search

rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
694 stars 131 forks source link

Implement NaN propagating minmax with TotalOrder and FloatCore #323

Open jdh8 opened 4 months ago

jdh8 commented 4 months ago

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.

cuviper commented 4 months ago

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.

jdh8 commented 4 months ago

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

cuviper commented 3 months ago

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.