Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xlim and ylim defaults are incorrect on 0.6 #796

Closed
dpsanders opened this issue May 4, 2017 · 22 comments
Closed

xlim and ylim defaults are incorrect on 0.6 #796

dpsanders opened this issue May 4, 2017 · 22 comments
Labels

Comments

@dpsanders
Copy link
Contributor

On 0.5, the defaults for xlim and ylim take into account all elements of the plot and try to accommodate them all; on 0.6, they seem to default to (0, 1).

@mkborregaard
Copy link
Member

I can't replicate this on 0.6, could you post an MWE? Are you on Plots master?

@mkborregaard
Copy link
Member

@dpsanders Do you still have this issue or can it be closed?

@daschw
Copy link
Member

daschw commented May 22, 2017

I could replicate this with histogram(randn(1000))on 0.6 yesterday

@daschw
Copy link
Member

daschw commented May 22, 2017

(currently not at my workstation... thus 'yesterday' )

@mkborregaard
Copy link
Member

Yup, me too. Ugh.

@mkborregaard
Copy link
Member

This only affects the :bar seriestype, and thus, by extension, histogram.

@dpsanders
Copy link
Contributor Author

Also the :shape seriestype (which I guess is used for :bar).

@mkborregaard
Copy link
Member

It appears to be the extrema function pulling tricks

@mkborregaard
Copy link
Member

mkborregaard commented May 22, 2017

In 0.5:

julia> Base.extrema([1,2,3,NaN])
(1.0,3.0)

In 0.6:

julia> Base.extrema([2,3,4, NaN])
(NaN, NaN)

@dpsanders
Copy link
Contributor Author

dpsanders commented May 22, 2017

That in turn is due to

julia> max(3, NaN)
NaN

which, as far as I remember, is a documented change in behavior in 0.6.

Probably best to just write Plots' own extrema function:

julia> my_extrema(x) = extrema(filter(!isnan, x))
my_extrema (generic function with 1 method)

julia> my_extrema([1,2,3,NaN])
(1.0, 3.0)

@dpsanders
Copy link
Contributor Author

JuliaLang/julia#12563

@mkborregaard
Copy link
Member

There's apparently a super-lightweight package to deal with such things. I've put in a PR with extrema JuliaMath/NaNMath.jl#16 - I am guessing we'll need to depend on that package anyway.

@mkborregaard
Copy link
Member

This turns out to be a longer ordeal - there is #856 but there are so many instances of maximum and minimum in the code, not all compatible with the NaNMath versions.

@mkborregaard
Copy link
Member

It's also zlim and clim, meaning that a lot of functionality is currently lost:

z = randn(10,10)
z[2,2] = NaN
heatmap(z)

This is a major issue, as NaNMath has no methods for arrays of higher dimension (e.g. matrices). I don't know if they plan to implement them.
This is very likely the biggest hurdle towards becoming 0.6 compliant on time.

@mkborregaard
Copy link
Member

It affects at least minimum, maximum, extrema and mean, perhaps also sum.

@dpsanders
Copy link
Contributor Author

Maybe there could be an ignore_nan keyword argument in all of those functions? Or just define Plots.jl's own, non-exported versions, and write Plots.extrema etc everywhere?

@dpsanders
Copy link
Contributor Author

If you just change the type annotation for the argument of NaNmath.extrema to AbstractArray, it will work for any dimension.

@mkborregaard
Copy link
Member

mkborregaard commented May 25, 2017

Yeah I don't think the want an ignore_nan keyword (I think it would lead to dynamic dispatch of functionality?), but instead e.g. NaNMath.mean – the issue is that these functions are only defined for a narrow subset of types.

If you just change the type annotation for the argument of NaNmath.extrema to AbstractArray, it will work for any dimension.

It's that simple, huh? I thought they'd need generated functions and everything. I'll suggest that to the NaNMath maintainers.

So far I am experimenting with your suggestion above (defining _extrema etc everywhere and filtering with isnan)

@dpsanders
Copy link
Contributor Author

As far as I can see, it should be that simple, at least for anything that has indexing for which "for i in x" works.

@mkborregaard
Copy link
Member

mkborregaard commented May 25, 2017

There is a new version of the PR here #866 that uses your suggestion above of just defining _ versions locally and using filter(!isnan(.

for fun in (:extrema, :minimum, :maximum, :mean)
       @eval $(Symbol(string("_",fun))){F<:AbstractFloat, N<:Integer}(x::AbstractArray{F, N}) = Base.$(fun)(filter(!isnan(x)))
       @eval $(Symbol(string("_",fun)))(x) = Base.$(fun)(x)
end

Here's the bad news - it hasn't solved the (0,1) limits issue. I will revisit this later.

@mkborregaard
Copy link
Member

After working an inordinate number of hours(!) on this, including 3 PRs on the NaNMath package, I know have a solution to this that works: #876 . I am not a big fan of JuliaLang/julia#12563 .
Comments are requested on the PR.

@dpsanders
Copy link
Contributor Author

I can confirm that the original bug is now fixed on Plots.jl master, many thanks!

daschw pushed a commit to daschw/Plots.jl that referenced this issue Jun 1, 2017
…ts#876)

Replaces min, max, minimum, mean, maximum and extrema with NaNMath versions in places where NaNs can occur.

To avoid returning NaN when there are NaNs in the Vector

* Also add maximum and minimum

* define _-prefaced versions of mean, maximum, minimum, extrema

* variable arg numbers for Base methods

* Different implementation of the override

* remove underscore from 2-arg versions of maximum

* some forgotten extrema -> _extrema

* Fix bug in _extrema definition

* edit comment

* replace min and max with _min and _max

* Base NaN-compliant functions on NaNMath


replace _min and _max with NaNMath versions

* Use NaNMath explicitly everywhere

* remove unneccesary NaNMath calls

* Ensure ceil does not error on NaN

* Added one more maximum in gr
@t-bltg t-bltg removed the priority label Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants