List/Array/Seq.max/min/maxBy/minBy may return nan if the sequence starts with nan

[Happypig375]

The position where NaN appears changes what Seq.max returns.

Repro steps

printfn $"{Seq.max [nan; 2.; 1.; 4.]}"
printfn $"{Seq.max [3.; nan; 1.; 4.]}"
printfn $"{Seq.max [3.; 2.; nan; 4.]}"
printfn $"{Seq.max [3.; 2.; 1.; nan]}"

Expected behavior

4
4
4
3

Actual behavior

NaN
4
4
3

Known workarounds

Filter away all NaNs first.

Related information

SharpLab
https://sharplab.io/#v2:DYLgZgzgNALiBOBXAdgHwA7wJbJmZABACQBEA3gMoCmAjgHQC2AhgB4EDayTyA3AQEx0+ARiEEALHQC6AXxIBYAFCYceQqUq1GrDgGYxXXgVF9Jshcuy58xctXrM27fX0F9Dp6XKUrr6u1qOemJuxgbc5kA=

[vzarytovskii]

FYI, Seq.min behaves the same, which, technically, makes sense, since:

> 4. > nan;;
val it: bool = false

> 4. < nan;;
val it: bool = false

So, current will remain nan forever, since it was initially bound as first element.

[dsyme]

Tricky problem, cf. min/max:

> max nan 1.0;;
val it: float = nan

> max 1.0 nan;;
val it: float = nan

> min 1.0 nan;;
val it: float = nan

> min nan 1.0;;
val it: float = nan

Code here:

        let mutable acc = e.Current

        while e.MoveNext() do
            let curr = e.Current
            if curr > acc then acc <- curr

        acc

could be

        let mutable acc = e.Current

        while e.MoveNext() do
            acc <- max acc e.Current

        acc

Fixing it does seem to make sense. Might be slower

[dsyme]

Noting List, Array, maxBy, minBy also have this problem

@vzarytovskii I do think this should be fixed. Labelling as good-first-issue

[abonie]

Just to clarify, for the example in the original post:

Repro steps

printfn $"{Seq.max [nan; 2.; 1.; 4.]}"
printfn $"{Seq.max [3.; nan; 1.; 4.]}"
printfn $"{Seq.max [3.; 2.; nan; 4.]}"
printfn $"{Seq.max [3.; 2.; 1.; nan]}"

Expected behavior

4
4
4
3

based on how min/max works currently, the actual expected behavior is

NaN
NaN
NaN
NaN

correct?

edit: maybe I shouldn't have used the phrase "the actual expected behavior" when trying to clarify things 😄

[vzarytovskii]

Expected behavior

4
4
4
3

This is how @Happypig375 expected Seq.max to behave.

based on how min/max works currently, the actual expected behavior is

NaN
NaN
NaN
NaN

This is how it behaves currently.

[abonie]

based on how min/max works currently, the actual expected behavior is

NaN
NaN
NaN
NaN

This is how it behaves currently.

The above is not quite correct and that's why I am trying to clarify. The actual observed behavior, both by @Happypig375 based on their original post and by me is in fact:

Actual behavior

NaN
4
4
3

edit: ...and fix suggested by @dsyme:

could be

        let mutable acc = e.Current

        while e.MoveNext() do
            acc <- max acc e.Current

        acc

as far as I can tell would result in 4x NaN, as opposed to the behavior expected by @Happypig375. So it would be best to explicitly agree on the expected behavior.

[vzarytovskii]

based on how min/max works currently, the actual expected behavior is

NaN
NaN
NaN
NaN

This is how it behaves currently.

The above is not quite correct and that's why I am trying to clarify. The actual observed behavior, both by @Happypig375 based on their original post and by me is in fact:

Actual behavior

NaN
4
4
3

edit: ...and fix suggested by @dsyme:

could be

        let mutable acc = e.Current

        while e.MoveNext() do
            acc <- max acc e.Current

        acc

as far as I can tell would result in 4x NaN, as opposed to the behavior expected by @Happypig375. So it would be best to explicitly agree on the expected behavior.

Sorry, I misread your comment. I personally would expect that Seq.max and Seq.min would never return nan unless it is the only element in the array.
The proposed change to replace inner implementation to use built-in function will result in the opposite behaviour.

Not sure which default behaviour @dsyme has in mind.