Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature request] total_cmp for num_traits::Float #256

Closed
felix-clark opened this issue Feb 9, 2023 · 8 comments · Fixed by #295
Closed

[feature request] total_cmp for num_traits::Float #256

felix-clark opened this issue Feb 9, 2023 · 8 comments · Fixed by #295

Comments

@felix-clark
Copy link

With total_cmp() being stabilized for f32 and f64, could this be included in the floating point trait? It's quite a convenient method for sorting generic floating point types.

@cuviper
Copy link
Member

cuviper commented Feb 10, 2023

Is there a generic way to write a default implementation? Otherwise, adding a trait method is a breaking change.

@felix-clark
Copy link
Author

To my knowledge there is no trivial built-in generic way.

@cuviper
Copy link
Member

cuviper commented Feb 10, 2023

Then I suppose this will need to be a standalone trait. Something like:

pub trait TotalOrd {
    fn total_cmp(&self, other: &Self) -> Ordering;
}

That could even have impls for integers. In theory, we could have a blanket impl for all T: Ord, but I think coherence would then disallow impls for f32 and f64 in case they add Ord in the future, even though we know they won't.

@andrewjradcliffe
Copy link
Contributor

Incidentally, half provides implementations of total_cmp for both f16 and bf16 which match the signature suggested by @cuviper. Likewise, rug's wrapper around MPFR provides a total_cmp implementation. In other words, it's little more than 5 lines of code for the respective maintainers.

As a separate constraint which could be imposed upon generic types, it would be useful in the rare numerical algorithms which necessitate sorting of floating point numbers (e.g. isotonic regression). NaN in such algorithms will of course produce garbage results, but using total_cmp makes the behavior reasonably transparent to the user -- in contrast to library writers making arbitrary decisions for how NaNs are handled (or not).

The inevitable bikeshedding: TotalOrd feels not quite as clean as one would like. Ord is already total. We could get around this by invoking the source of the ordering, i.e. name the trait IeeeOrd (or IEEEOrd?). While ugly to look at, it has the advantage of clarity.

@cuviper
Copy link
Member

cuviper commented Sep 5, 2023

We could further distinguish this from PartialOrd/Ord by spelling it out like trait TotalOrder, and include the standard library's documentation that this is "in accordance to the totalOrder predicate as defined in the IEEE 754 (2008 revision) floating point standard." (Or an approximation thereof for 3rd-party types.) We could also keep this in the num_traits::float module and not implement for integers as I had suggested before, so it remains scoped to float-like comparisons.

@andrewjradcliffe
Copy link
Contributor

trait TotalOrder is preferable to IeeeOrd, as the latter is rather generic. Placing it within the num_traits::float module and including "in accordance to the totalOrder predicate..." should be sufficient to disambiguate that the trait is strictly for floating point types.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2023

@andrewjradcliffe Would you be interested in writing that pull request?

@andrewjradcliffe
Copy link
Contributor

Just saw this today -- I'm not great about checking notifications ._.
Give me a day or so and I'll have the PR ready.

bors bot added a commit that referenced this issue Oct 27, 2023
295: `TotalOrder` trait for floating point numbers r=cuviper a=andrewjradcliffe

Define an orthogonal trait which corresponds to the `totalOrder` predicate the IEEE 754 (2008 revision) floating point standard.

In order to maintain coherence, the bounds on `TotalOrder` should most likely be `TotalOrder: Float` (or `TotalOrder: FloatCore`).  Without type constraints, `TotalOrder` could be defined on, well, anything. Though slightly ugly, one way to deal with this is to define two traits, `TotalOrderCore: FloatCore` and `TotalOrder: Float`.  On the other hand, `Inv` has no such constraints (due to the possibility of a meaningful implementation on rational numbers). If the crate designers could weigh in on whether to introduce constraints, that would be helpful.

Resolves: [issue#256](#256)

Co-authored-by: Andrew Radcliffe <andrewjradcliffe@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
bors bot added a commit that referenced this issue Oct 27, 2023
295: `TotalOrder` trait for floating point numbers r=cuviper a=andrewjradcliffe

Define an orthogonal trait which corresponds to the `totalOrder` predicate the IEEE 754 (2008 revision) floating point standard.

In order to maintain coherence, the bounds on `TotalOrder` should most likely be `TotalOrder: Float` (or `TotalOrder: FloatCore`).  Without type constraints, `TotalOrder` could be defined on, well, anything. Though slightly ugly, one way to deal with this is to define two traits, `TotalOrderCore: FloatCore` and `TotalOrder: Float`.  On the other hand, `Inv` has no such constraints (due to the possibility of a meaningful implementation on rational numbers). If the crate designers could weigh in on whether to introduce constraints, that would be helpful.

Resolves: [issue#256](#256)

Co-authored-by: Andrew Radcliffe <andrewjradcliffe@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors bors bot closed this as completed in #295 Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants