[Draft] Fix #5647, difference in max/min in debug with floats and nans

[abelbraaksma]

Fixes #5647

As mentioned in the issue, the Core min and max functions behave incorrect when used with float/float32 types in relation to nan/nanf. The result should always be nan/nanf, but this isn't the case in Debug mode, because optimizations aren't applied and instead, the default comparer is being used (that is, the line if e1 < e2 then e2 else e1 from the implementation is executed, as opposed to the specialized paths).

Since I can't run the tests at the moment (see #13557), I use CI to check if the tests properly fail indeed.

EDIT: this is a precursor to #13207, which deals with a slightly different bug related to Seg.min/max etc.

[abelbraaksma]

Weird errors in fsharpqa build:

setup\Swix\Microsoft.FSharp.SDK\Microsoft.FSharp.SDK.csproj(0,0): error NU1301: (NETCORE_ENGINEERING_TELEMETRY=Restore) Failed to verify the root directory of local source 'https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5-transport/nuget/v3/index.json'.

[tannergooding]

You should also validate the behavior against negative numbers (e.g -1.0) and positive nan (well technically a nan with the inverse sign of Double.NaN/Single.NaN, but those are typically a negative NaN).

[abelbraaksma]

@tannergooding, tx, I’ll do that. This will ultimately use Math.Min, for which I’ve seen that it does this correctly already.

[abelbraaksma]

Hmm, CI seems broken, same error as above in the job that runs tests. 16 tests do not show up in the result.

Should be this, I think (from other, green PR):

Failed: 0 (0.00%) Passed: 37,032 (99.07%) Other: 348 (0.93%) Total: 37,380

But I got 37,016

[abelbraaksma]

well technically a nan with the inverse sign of Double.NaN/Single.NaN, but those are typically a negative NaN

@tannergooding, adding those tests revealed a weird little bug in Math.Max. While the IEEE 754 spec allows you return either of the arguments (unless you'd support "totalOrder" definition of that spec), it looks like Math.Min returns a negative NaN (that is +Double.NaN) when there is one, Math.Max does not:

// in .NET, the BCL NaN is negative, so -NaN gives positive 
> Math.Max(nan, -nan) |> Double.IsNegative;;  // expected
val it: bool = true

> Math.Max(-nan, nan) |> Double.IsNegative;;  // not expected
val it: bool = false

> Math.Max(-nan, -nan) |> Double.IsNegative;;  // expected
val it: bool = false

> Math.Max(nan, nan) |> Double.IsNegative;;   // expected
val it: bool = true

You could argue that the code is shortcut, but then you'd expect a similar shortcut with Math.Min, but it behaves stable:

> Math.Min(nan, -nan) |> Double.IsNegative;;  // expected
val it: bool = true

> Math.Min(-nan, nan) |> Double.IsNegative;;  // expected
val it: bool = true

> Math.Min(-nan, -nan) |> Double.IsNegative;;  // expected
val it: bool = false

> Math.Min(nan, nan) |> Double.IsNegative;;  // expected
val it: bool = true

[abelbraaksma]

From CI:

The type 'Double' does not define the field, constructor or member 'IsNegative'.

I should’ve seen that coming… these weren’t available at the time (is this Core 3.0 or Framework?). Either way, I’ll add bit test for proper comparison.

[tannergooding]

adding those tests revealed a weird little bug in Math.Max.

That is possibly related to a bug that is already fixed and will be in the next patch release of .NET 6: https://github.com/dotnet/runtime/pull/70865

It's already in the latest .NET 7 public preview.

While the IEEE 754 spec allows you return either of the arguments (unless you'd support "totalOrder" definition of that spec), it looks like Math.Min returns a negative NaN (that is +Double.NaN) when there is one, Math.Max does not

Right, which of two NaNs is returned is undefined and the IEEE 754 spec even allows that NaN to be normalized (and so it might differ from either NaN input, as long as some NaN is returned). The test is more important in that Min(+NaN, -1) and Max(-NaN, +1) should each return some NaN value and shouldn't accidentally treat -1 < +NaN or +1 > -NaN due to how it checks for signs/etc.

Even with totalOrder, the IEEE 754 spec allows NaN normalization as a legal "optimization". From 10.4 Literal meaning and value-changing optimizations:

The following value-changing transformations, among others, preserve the literal meaning of the source code: ... Changing the payload or sign bit of a quiet NaN.

Ideally, most operations won't however.

Max and Min map to the IEEE 754 maximum and minimum APIs, respectively (returning the NaN one input is NaN and the other is not. It also treats -0 as less than +0) MaxNumber and MinNumber map to the IEEE 754 maximumNumber and minimumNumber APIs, respectively (returning the number one input is NaN and the other is not. It also treats -0 as less than +0)

We don't currently have any API that exposes totalOrder and takes things like NaN sign/payload into account. If we did, it would likely just be updating CompareTo, but that is "more expensive" for little practical benefit.

[abelbraaksma]

@tannergooding, thanks for the explanation. I looked at the IEEE 754:2008 version, not the 2019 version, the latter which makes the behavior you mention explicit. I looked at the PR (I searched, but searching for Max/Min in Git is rather fruitless) and that boils down to: if both are NaN, return first arg, if either is NaN, return that value, if neither is, just compare.

I should update the tests to cover for this change in behavior, otherwise, as soon as this is patched, the current tests will fail.

My wrong assumption was that negative NaN would be ordered before positive NaN, but that isn't the case. They are treated according to the unordered sections in the spec, i.e., it shouldn't matter which is returned, as long as it is some NaN.