TraversableOnce.max with NaN should return NaN

[scabug]

scala> Seq(1.0, Double.NaN, 2.5E-4).max
res2: Double = 2.5E-4

From a user point of view it would be great if this operation could
return NaN.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-9637?orig=1
Reporter: Romain Reuillonr (romain)
Affected Versions: 2.11.7
See #5104

[scabug]

@ruippeixotog said:
I consider this a bug indeed, as for any collection seq, seq.max should be always equal to seq.reduce(max). Unfortunately, this happens because the ordering for Float and Double values follow the IEEE convention where floating-point values are never smaller, larger or equal to NaN. Therefore, implementing ordering-based operations on sequences with comparison operators would always lead to inconsistencies.

Implementing max via reduce(ordering.max) would solve the problem, as the Ordering instances of floating-point values already consider this special case. However, the current signature (min[B >: A](implicit cmp: Ordering[B]): A) doesn't allow this, as cmp.max returns a B, not an A as required. We can't rely on checking which argument the result is equal to, as NaN is not equal to NaN.

The only way I see to fix this without changing the method signature is explicitly checking if we are dealing with Float or Double and find the min/max value accordingly. It's very hacky and won't work on user-defined types with the same semantics, but I can't think of other way right now. @Ichoran, what do you think?

[scabug]

@Ichoran said:
@ruippeixotog - I think under the circumstances special-casing max to check whether it's got an instance of Ordering.Double or Ordering.Float is a reasonable solution.

You could also explicitly check the semantics of NaN values: a NaN is not even equal to itself, which lets you detect it. But I'm not sure it's worth the extra runtime.

[scabug]

@szeiger said:
Assigning to 2.13. We should consider the required signature changes for the Collections overhaul in 2.13.

[scabug]

Chase Bradford (cbradford) said (edited on Apr 23, 2016 4:36:35 AM UTC):
I just hit this bug, and I find it particularly bad because when NaN is encountered, the current implementation effectively starts over.

Seq( 0.0, NaN, 1.0 ).min == 1.0
Seq( NaN, 1.0, 0.0).min == 0.0
Seq( 1.0, 0.0, NaN).min == NaN
Seq( 1.0, NaN, 0.0).min == 0.0

I'd argue that NaN is only a valid response if NaN is the only value in the collection. Otherwise, it should effectively be ignored.

If the fold replaced the accumulator only when the condition is true (ie. the subject under test is less than), then we'd get stable behavior regardless of the order of the elements. Instead, the logic replaces the accumulator whenever the comparison is false, which is why NaN is so disruptive.

[som-snytt]

scala 2.13.0-M5> Seq(1.0, Double.NaN, 2.5E-4).max
                                              ^
                 warning: object DeprecatedDoubleOrdering in object Ordering is deprecated (since 2.13.0): There are multiple ways to order Doubles (Ordering.Double.TotalOrdering, Ordering.Double.IeeeOrdering). Specify one by using a local import, assigning an implicit val, or passing it explicitly. See the documentation for details.
res0: Double = NaN

[NthPortal]

Fixed by scala/scala#6410