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

max and min of NaN return NaN #12563

Merged
merged 1 commit into from
Sep 16, 2016
Merged

Conversation

stevengj
Copy link
Member

As an experiment, this patch changes max and min (and related functions) to propagate NaN values, as discussed in #7866. (I'm not sure whether #7866 was resolved in favor of this or not, but I thought I would see what it looked like.)

While I agree with Kahan that, in principle, it makes some sense for max(NaN,Inf) to return Inf, I agree with @danluu that it's not worth adding an extra branch to an inner-loop function like this for a "corner case of a corner case" in which the correct behavior is ambiguous.

As a side-effect, this fixes JuliaLang/LinearAlgebra.jl#236, although #12564 fixes that anyway.

@stevengj stevengj added needs decision A decision on this change is needed maths Mathematical functions labels Aug 11, 2015
@StefanKarpinski
Copy link
Member

💯

@stevengj
Copy link
Member Author

Hitting the sqrt(big(NaN)) bug ... this should wait for a rebase when #12564 is merged. Update: rebased.

@stevengj stevengj force-pushed the maxnan branch 2 times, most recently from 50515b4 to 0129abb Compare August 11, 2015 20:53
@iamed2
Copy link
Contributor

iamed2 commented Aug 11, 2015

I had no idea this was not already the case. Glad it is becoming so. Also glad isnan(max(NaN, Inf)), as !isless(NaN, Inf). In my interpretation, the NaN return from max is not saying NaN > Inf, but that there is no numeric max of NaN and Inf, as NaN is not comparable.

Small note: maxmin should be minmax in News.md.

@ufechner7
Copy link

If this is merged, is there a function nanmax and nanmin that ignores nan values (like http://docs.scipy.org/doc/numpy/reference/generated/numpy.nanmax.html )? I would need this to ignore invalid or missing values in measurement data.

@ScottPJones
Copy link
Contributor

Yes, this surprised me very much also that it wasn't already always promoting the NaN. 💯

@StefanKarpinski
Copy link
Member

@ufechner7, can't you just filter out NaNs first?

@stevengj
Copy link
Member Author

@ufechner, wouldn't you also need nanmaximum and nanminimum and nanextrema and nanminmax?

@ufechner7
Copy link

Well, numpy defines the following functions, that ignore nan's:
nanargmax nanargmin nanmax nanmin
nansum nanmean nanstd nanvar
It would be nice, if similar functions could be available in Julia.
I could open a separate feature request issue to implement these functions.
Should this be implemented in base, or rather in a package? If so, in which
package?

@KristofferC
Copy link
Member

NaNOps.jl ?

@tkelman
Copy link
Contributor

tkelman commented Aug 13, 2015

There's already a registered https://github.com/mlubin/NaNMath.jl but that kind of serves an alternate purpose (returning NaN instead of DomainError from NaNMath.log(-1)). Might make for an appropriately-named home at least?

@stevengj
Copy link
Member Author

MissingNaN.jl?

@KristofferC
Copy link
Member

@stevengj Haha that's a great name!

@stevengj
Copy link
Member Author

@iamed2, I fixed NEWS.md

@nalimilan
Copy link
Member

Using DataArrays.jl could also make sense, depending on .your use case.

@pkofod
Copy link
Contributor

pkofod commented Jun 22, 2016

Just because I recently ran into this, is there a reason this is not merged? Seems like the discussion got off track, and then nothing happened.

@StefanKarpinski
Copy link
Member

Now that it's open season for breaking changes on master, let's do this.

@stevengj stevengj force-pushed the maxnan branch 2 times, most recently from afe7b8e to b1ecd56 Compare September 14, 2016 02:10
@stevengj
Copy link
Member Author

Rebased.

@StefanKarpinski
Copy link
Member

Excellent. Once CI passes, let's give this a day and then merge it unless there are objections.

@tkelman
Copy link
Contributor

tkelman commented Sep 14, 2016

osx travis failure was a timeout, log backed up to https://gist.github.com/fa25ebd3a383d4bb64025e34b95d5328 and restarted

@tkelman
Copy link
Contributor

tkelman commented Sep 15, 2016

@nanosoldier runbenchmarks(ALL, vs = ":master")

@stevengj
Copy link
Member Author

I don't think BaseBenchmarks.jl has much coverage of this, e.g. there is no benchmark of just maximum(array).

@tkelman tkelman added the potential benchmark Could make a good benchmark in BaseBenchmarks label Sep 15, 2016
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@StefanKarpinski
Copy link
Member

Lacking objections, I'm merging this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions needs decision A decision on this change is needed potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

norm(Float64[0,NaN]) == 0
10 participants