max and min of NaN return NaN

[stevengj]

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 #12552, although #12564 fixes that anyway.

[StefanKarpinski]

💯

[stevengj]

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

[iamed2]

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]

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]

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

[StefanKarpinski]

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

[stevengj]

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

[ufechner7]

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]

NaNOps.jl ?

[tkelman]

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]

MissingNaN.jl?

[KristofferC]

@stevengj Haha that's a great name!

[stevengj]

@iamed2, I fixed NEWS.md

[nalimilan]

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

[pkofod]

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]

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

[stevengj]

Rebased.

[StefanKarpinski]

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

[tkelman]

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

[tkelman]

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

[stevengj]

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

[nanosoldier]

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

[StefanKarpinski]

Lacking objections, I'm merging this :)