Bug 10448 – min and max are not NaN aware

(and so are probably minCount, minPos, or mostly everything else that is related to opCmp). From the discussion https://github.com/D-Programming-Language/phobos/pull/1360 : min(0, float.nan); //Yields 0 max(0, float.nan); //Yields 0 min(float.nan, 0); //Yields float.nan max(float.nan, 0); //Yields float.nan Not sure what the correct solution is: Throwing an exception on NaN might be too expensive? How about an assert, combined with an explicit: "Passing nan is an ERROR", and let the user decide if/ifnot to pay for the check?

(In reply to comment #0) > (and so are probably minCount, minPos, or mostly everything else that is > related to opCmp). > > From the discussion https://github.com/D-Programming-Language/phobos/pull/1360 > : > min(0, float.nan); //Yields 0 > max(0, float.nan); //Yields 0 > min(float.nan, 0); //Yields float.nan > max(float.nan, 0); //Yields float.nan > > Not sure what the correct solution is: Throwing an exception on NaN might be > too expensive? How about an assert, combined with an explicit: "Passing nan is > an ERROR", and let the user decide if/ifnot to pay for the check? I don't see why one would throw on nan, and it would be expensive. I believe the accepted convention is that nan is treated as missing data for min/max, so that min(0, float.nan); //Yields 0 max(0, float.nan); //Yields 0 min(float.nan, 0); //Yields 0 max(float.nan, 0); //Yields 0 related discussion: http://bytes.com/topic/c/answers/528404-max-nan-0-should-nan matlab does it that way btw.

Exactly. See documentation of fmax at http://man7.org/linux/man-pages/man3/fmax.3.html Specifically: If one argument is a NaN, the other argument is returned. If both arguments are NaN, a NaN is returned.

Then it needs to be documented in Phobos, and this fixed: min(0, float.nan); //Yields 0 min(float.nan, 0); //Yields float.nan (should be 0 then?)

(In reply to comment #2) > Exactly. See documentation of fmax at > http://man7.org/linux/man-pages/man3/fmax.3.html > > Specifically: > If one argument is a NaN, the other argument is returned. > > If both arguments are NaN, a NaN is returned. Isn't it better for min(0, float.nan) to be NaN, just as max(0, float.nan) ?

(In reply to comment #4) > (In reply to comment #2) > > Exactly. See documentation of fmax at > > http://man7.org/linux/man-pages/man3/fmax.3.html > > > > Specifically: > > If one argument is a NaN, the other argument is returned. > > > > If both arguments are NaN, a NaN is returned. > > Isn't it better for min(0, float.nan) to be NaN, just as max(0, float.nan) ? Yeah, that sounds like the better behavior: *anything* and nan is always nan.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > Exactly. See documentation of fmax at > > > http://man7.org/linux/man-pages/man3/fmax.3.html > > > > > > Specifically: > > > If one argument is a NaN, the other argument is returned. > > > > > > If both arguments are NaN, a NaN is returned. > > > > Isn't it better for min(0, float.nan) to be NaN, just as max(0, float.nan) ? > > Yeah, that sounds like the better behavior: *anything* and nan is always nan. that would indeed seem more logical, although: * it differs from standard practice * it incurs additional cost, compared to : return a<b?a:b; because you'd have to check for isNan

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #2) > > > > Exactly. See documentation of fmax at > > > > http://man7.org/linux/man-pages/man3/fmax.3.html > > > > > > > > Specifically: > > > > If one argument is a NaN, the other argument is returned. > > > > > > > > If both arguments are NaN, a NaN is returned. > > > > > > Isn't it better for min(0, float.nan) to be NaN, just as max(0, float.nan) ? > > > > Yeah, that sounds like the better behavior: *anything* and nan is always nan. > > that would indeed seem more logical, although: > > * it differs from standard practice > * it incurs additional cost, compared to : return a<b?a:b; because you'd have > to check for isNan "return a<b?a:b;" doesn't work: you'd have to check for nan regardless of which you return.

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > (In reply to comment #2) > > > > > Exactly. See documentation of fmax at > > > > > http://man7.org/linux/man-pages/man3/fmax.3.html > > > > > > > > > > Specifically: > > > > > If one argument is a NaN, the other argument is returned. > > > > > > > > > > If both arguments are NaN, a NaN is returned. > > > > > > > > Isn't it better for min(0, float.nan) to be NaN, just as max(0, float.nan) ? > > > > > > Yeah, that sounds like the better behavior: *anything* and nan is always nan. > > > > that would indeed seem more logical, although: > > > > * it differs from standard practice > > * it incurs additional cost, compared to : return a<b?a:b; because you'd have > > to check for isNan > > "return a<b?a:b;" doesn't work: you'd have to check for nan regardless of which > you return. my bad, which is probably why the current min is buggy. In that case since there's no penalty might as well do the safest thing, ie returning nan, and provide a minIgnoresNan to do the 'standard behavior', which will return 0 instead of nan.

There is no clearly "correct" way here, see the PR discussion on GitHub: https://github.com/dlang/phobos/pull/4316#issuecomment-219301946

Default behavior should be that min/max be translated to the single assembler instruction, which can be vectorized. Compiler may have intrinsics for min/max and we should just use them. See also https://llvm.org/bugs/show_bug.cgi?id=24314

FWIW, after https://github.com/dlang/phobos/pull/2198, the output changes to: nanF 0.00000F 0.00000F nanF Still looks broken, but in a different way.

@crocopaw created dlang/phobos pull request #7181 "Fix Issue 10448 - min and max are not NaN aware" fixing this issue: - Fix Issue 10448 - min and max are not NaN aware https://github.com/dlang/phobos/pull/7181

As experts do not agree on the prefered result I think it's best to document, that NaNs will result in undefined behaviour. See https://github.com/dlang/phobos/pull/7181 for more details.

@berni44 created dlang/phobos pull request #7240 "Fix Issue 10448 - min and max are not NaN aware" fixing this issue: - Fix Issue 10448 - min and max are not NaN aware https://github.com/dlang/phobos/pull/7240

dlang/phobos pull request #7240 "Fix Issue 10448 - min and max are not NaN aware" was merged into master: - 0320d88c3aecb5af38b68fc2efcbf661d07190a1 by Bernhard Seckinger: Fix Issue 10448 - min and max are not NaN aware https://github.com/dlang/phobos/pull/7240