Clojure

core min and max should behave predictably when args include NaN

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

The min and max functions in clojure.core behave unpredictably when one or more of their arguments is Float/NaN or Double/NaN. This is because the current implementation assumes that > provides a total ordering, but this is not the case when NaN is added to the mix. This is an unfortunate fact of life when dealing with IEEE floating point numbers.

See also the recent mailing list thread "clojure.core/max and NaN".

May be related to issue CLJ-738.

Attachments

  1. Text File
    CLJ-868-contagious-NaN.patch
    01/Nov/11 11:05 AM
    8 kB
    Ben Smith-Mannschott
  2. Text File
    CLJ-868-ignore-NaN.patch
    01/Nov/11 1:12 PM
    9 kB
    Ben Smith-Mannschott
  3. Text File
    CLJ-868-throw-exception-on-NaN.patch
    01/Nov/11 12:15 PM
    8 kB
    Ben Smith-Mannschott

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Ben Smith-Mannschott added a comment -

It seems to me that there are four approaches one might take to address this.

  1. Document the current undefined behavior of min and max in the presence of NaN.
  2. Define that min and max will return NaN if any argument is NaN.
  3. Define that min and max will ignore any NaN arguments.
  4. Define that min and max will throw an exception if any argument is NaN.

1 Document current behavior as undefined

This requires no changes in the implementation, but it doesn't strike me as a desirable resolution. Why unnecessarily codify clearly confusing behavior?

2 Make NaN contagious

Define min and max to return NaN if and only if at least one of their arguments is NaN. This seems most in keeping with the (admittedly perverse) behavior of NaN as specified.

See JLS 4.2.4 Floating Point Operations:

An operation that overflows produces a signed infinity, an operation that underflows produces a denormalized value or a signed zero, and an operation that has no mathematically definite result produces NaN. All numeric operations with NaN as an operand produce NaN as a result. As has already been described, NaN is unordered, so a numeric comparison operation involving one or two NaNs returns false and any != comparison involving NaN returns true, including x!=x when x is NaN.

3 Let min and max ignore NaN arguments

This means that (min a NaN b) would be exactly equivalent to (min a b). It would further imply that (min NaN) would be equivalent to (min), which currently throws an exception.

4 Let NaN cause min and max to throw an exception

Currently min and max throw an exception if given arguments that are not Numeric. One might plausibly argue that NaN is not numeric.

Show
Ben Smith-Mannschott added a comment - It seems to me that there are four approaches one might take to address this.
  1. Document the current undefined behavior of min and max in the presence of NaN.
  2. Define that min and max will return NaN if any argument is NaN.
  3. Define that min and max will ignore any NaN arguments.
  4. Define that min and max will throw an exception if any argument is NaN.

1 Document current behavior as undefined

This requires no changes in the implementation, but it doesn't strike me as a desirable resolution. Why unnecessarily codify clearly confusing behavior?

2 Make NaN contagious

Define min and max to return NaN if and only if at least one of their arguments is NaN. This seems most in keeping with the (admittedly perverse) behavior of NaN as specified. See JLS 4.2.4 Floating Point Operations:
An operation that overflows produces a signed infinity, an operation that underflows produces a denormalized value or a signed zero, and an operation that has no mathematically definite result produces NaN. All numeric operations with NaN as an operand produce NaN as a result. As has already been described, NaN is unordered, so a numeric comparison operation involving one or two NaNs returns false and any != comparison involving NaN returns true, including x!=x when x is NaN.

3 Let min and max ignore NaN arguments

This means that (min a NaN b) would be exactly equivalent to (min a b). It would further imply that (min NaN) would be equivalent to (min), which currently throws an exception.

4 Let NaN cause min and max to throw an exception

Currently min and max throw an exception if given arguments that are not Numeric. One might plausibly argue that NaN is not numeric.
Hide
Permalink
Ben Smith-Mannschott added a comment -

CLJ-868-contagious-NaN.patch

(Implementation corresponds to variant 2 from my earlier comment.)

Show
Ben Smith-Mannschott added a comment - CLJ-868-contagious-NaN.patch (Implementation corresponds to variant 2 from my earlier comment.)
Hide
Permalink
Ben Smith-Mannschott added a comment -

This implements the third variant described above with one difference:

When when all arguments are NaN we don't throw a exception, but return NaN instead.

(min) and (max) still throw an exception, however.

This fell out of the implementation naturally and I couldn't see a good reason to prefer one behavior to another.

Show
Ben Smith-Mannschott added a comment - This implements the third variant described above with one difference: When when all arguments are NaN we don't throw a exception, but return NaN instead. (min) and (max) still throw an exception, however. This fell out of the implementation naturally and I couldn't see a good reason to prefer one behavior to another.
Hide
Permalink
Stuart Halloway added a comment -

The contagious version of the patch seems to me the right approach, assuming fidelity to Java (e.g. Math/max and Math/min) is the standard.

Show
Stuart Halloway added a comment - The contagious version of the patch seems to me the right approach, assuming fidelity to Java (e.g. Math/max and Math/min) is the standard.
Hide
Permalink
Rich Hickey added a comment -

contagious ok

Show
Rich Hickey added a comment - contagious ok

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.