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

Tracking Issue for sort_floats #93396

Open
1 of 3 tasks
joshtriplett opened this issue Jan 27, 2022 · 5 comments
Open
1 of 3 tasks

Tracking Issue for sort_floats #93396

joshtriplett opened this issue Jan 27, 2022 · 5 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jan 27, 2022

Feature gate: #![feature(sort_floats)]

This is a tracking issue for the sort_floats method on [f32] and [f64], a convenience method to sort a slice of floats by calling sort_unstable_by using total_cmp.

Public API

impl [f32] {
    pub fn sort_floats(&mut self);
}

impl [f64] {
    pub fn sort_floats(&mut self);
}

Steps / History

Open questions

  • Should we rename this to sort, which would require some additional steps (possibly negative impls for Ord on f32 and f64) to avoid a name conflict?
@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 27, 2022
@clarfonthey
Copy link
Contributor

I feel like total_sort would be a better name, since it matches the total_cmp naming and makes it clear what it does.

@Riolku
Copy link
Contributor

Riolku commented Oct 19, 2022

I don't think these are useful. Surely it's not very hard to use sort_unstable_by(f64::total_cmp).

I think, although longer, forcing the long usage makes users aware of the possible issues, instead of sweeping them under the rug.

As for name, I think total_float_sort would be more sensible maybe?

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

As mentioned in rust-lang/unsafe-code-guidelines#237 (comment), we probably shouldn't stabilize the current implementation of this at the moment -- it looks increasingly likely that we're going to have to say that the sign bit of NaN is fundamentally non-deterministic, at least in the short term.

A sorting function that puts NaNs non-deterministically at the start or end of the slice is probably far too error-prone for us to want to stabilize. (I probably should have pushed back harder than I did on total_cmp too, but I expected the non-determinism around NaN sign bit to be a bug we eventually fix, rather than something we have to live with indefinitely).

@shssoichiro
Copy link
Contributor

I just learned the existence of this... and while I like the idea of having this capability, just tacking on a separate method called "sort_floats" feels very unwieldy. It feels like it would be much more ergonomic to do something like adding an implementation of sort for PartialOrd. As a developer, the idea that I have to be aware of a separate method specifically for sorting floats is quite unintuitive.

@kornelski
Copy link
Contributor

kornelski commented Apr 9, 2024

I don't like that it uses total_cmp. This is already easy to do with the existing sort_by.

If there's going to be a dedicated method, I'd like one that is optimized for finite floats which can use the cheapest comparison available, ignoring what that means for NaN and other float edge cases. As long as it beats hacks like .sort_by(|a, b| a.partial_cmp(b).unwrap_or(Equal)) or .sort_by(|a, b| if a < b { Less } else { Greater }) on speed, correctness or stability.

Also it should be .sort_floats_by_key() to be able to handle arbitrary structs. I rarely have [f32], but very often [(f32, str)] or .sort_by_key(|item| item.score()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants