Fix for reduce (min, max), see blitzpp/blitz#68

[AnderOne]

This patch allows to get the next behavior:

min([1 NaN 2]) -> NaN
max([1 NaN 2]) -> NaN

[citibeth]

See #68

[slayoo]

Thank you! Including a test case would be super valuable, see e.g.: https://github.com/blitzpp/blitz/commit/193935fd73e3261e9aaf9eb27a5dfb268042e77d

[citibeth]

I am not comfortable with this change because the IEEE 754 standard does not define any particular outcome for this function. C++ std::min and std::max work in "non-intuitive" ways for this function. And therefore, we should not try to define our own floating point standard.

The reason why... the way NaN works in IEEE arithmetic is not intuitive. Our job with Blitz++ should not be to try to make it more "intuitive"; but rather, to follow the standard. The standard is very clear that operator<, operator> and operator== all return false if one or more of its operands is NaN. This leads to some non-intuitive behavior:

There is another important difference. For x ! = NaN:
https://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c

std::max(Nan,x) = NaN std::max(x,NaN) = x std::min(Nan,x) = NaN std::min(x,NaN) = x

The IEEE754 standard has done nothing to discourage this behavior; in fact, it might become part of the standard.

Until and unless we can define (in a way 100% consistent with IEEE standard) the definition of min(vector) where vector has NaNs... then the result remains undefined.  And therefore, the result provided by Blitz++ is acceptable.  I do not want to box us into Blitz++ committing to specific results for things that are undefined; which could cause performance or semantic problems later.  This change in particular is case in point: it doubles the number of comparisons done in each loop, which certainly will not speed things up for uses of `min()` and `max()` where things ARE defined.

In fact... I like the way things were implemented before.  It allows us to define `blitz:min()` in terms of `reduce()` and `std::min()` --- both of which are well-defined functions (one in C++ and one in Blitz++)

[lutorm]

Yeah, I agree with Elizabeth here.

On Wed, Mar 20, 2019 at 2:41 AM Elizabeth Fischer notifications@github.com wrote:

I am not comfortable with this change because the IEEE 754 standard does not define any particular outcome for this function. C++ std::min and std::max work in "non-intuitive" ways for this function. And therefore, we should not try to define our own floating point standard.

The reason why... the way NaN works in IEEE arithmetic is not intuitive. Our job with Blitz++ should not be to try to make it more "intuitive"; but rather, to follow the standard. The standard is very clear that operator<, operator> and operator== all return false if one or more of its operands is NaN. This leads to some non-intuitive behavior:

There is another important difference. For x ! = NaN:https://stackoverflow.com/questions/1632145/use-of-min-and-max-functions-in-c

std::max(Nan,x) = NaN std::max(x,NaN) = x std::min(Nan,x) = NaN std::min(x,NaN) = x

The IEEE754 standard has done nothing to discourage this behavior; in fact, it might become part of the standard.

Until and unless we can define (in a way 100% consistent with IEEE standard) the definition of min(vector) where vector has NaNs... then the result remains undefined. And therefore, the result provided by Blitz++ is acceptable. I do not want to box us into Blitz++ committing to specific results for things that are undefined; which could cause performance or semantic problems later. This change in particular is case in point: it doubles the number of comparisons done in each loop, which certainly will not speed things up for uses of min() and max() where things ARE defined.

In fact... I like the way things were implemented before. It allows us to define blitz:min() in terms of reduce() and std::min() --- both of which are well-defined functions (one in C++ and one in Blitz++)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/blitzpp/blitz/pull/105#issuecomment-474811874, or mute the thread https://github.com/notifications/unsubscribe-auth/AFK8GMUneF13Bltn1Fp6nMtfQ_WYUnN0ks5vYixigaJpZM4b-d5l .