num-traits icon indicating copy to clipboard operation
num-traits copied to clipboard

Implement NaN propagating minmax with TotalOrder and FloatCore

Open jdh8 opened this issue 1 year ago • 3 comments

Given the IEEE total-ordering predicate and .is_nan(), we can implement NaN propagating minmax, cf. f32::maximum.

I'm not sure if using .is_nan() with trait TotalOrder: FloatCore is the best idea, but I can't think of a better one for now.

jdh8 avatar May 11 '24 21:05 jdh8

Adding FloatCore is a breaking change, at least -- that was considered in #295, but didn't seem important.

Thematically though, I would expect them to be called total_min and total_max, and follow the full rules of total_cmp. Is there precedent for total-ordered floats that propagate NaN as you have? This is also the opposite of the builtin min/max on floats, which ignore NaN.

cuviper avatar May 12 '24 00:05 cuviper

No, they do not fully follow the rules of total_cmp. The total_cmp predicate is simply a sign-magnitude comparison, where -NaN < -Inf < -finite < -0.0 < +0.0 < +finite < +Inf < +NaN.

The nightly builtin minimum/maximum (not min/max) propagate NaN first (not necessarily one of the inputs) and then compare with total_cmp

jdh8 avatar May 12 '24 08:05 jdh8

I see -- I wasn't aware of those unstable methods. However, the tracking issue still has a number of unresolved questions, and I'd rather wait to see that stabilized before we try to match their name and behavior.

Even then, a breaking change is still a problem, but you could put where Self: FloatCore on those particular methods instead of the whole trait.

cuviper avatar May 12 '24 19:05 cuviper