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

Make PartialOrd implementation canonical for NonNAN<T> #222

Merged

Conversation

FreezyLemon
Copy link
Contributor

@FreezyLemon FreezyLemon commented Apr 28, 2024

Fixes clippy error:

warning: non-canonical implementation of `partial_cmp` on an `Ord` type
  --> src/distribution/empirical.rs:14:1
   |
14 | /  impl<T: PartialOrd> PartialOrd for NonNAN<T> {
15 | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
   | | _____________________________________________________________-
16 | ||         self.0.partial_cmp(&other.0)
17 | ||     }
   | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
18 | |  }
   | |__^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_partial_ord_impl
   = note: `#[warn(clippy::non_canonical_partial_ord_impl)]` on by default

This is the normal way of implementing PartialOrd when a manual Ord implementation exists because it ensures that they will return the same result.

This is technically a change in behaviour:

let ordering = NonNAN(f64::NAN).partial_cmp(NonNAN(1.0));

// before: None
// after: panic

But IMHO this is fine because NonNAN(f64::NAN) is unsupported anyways, right?

@FreezyLemon FreezyLemon force-pushed the fix-non-canonical-partialord-impl branch from 239baf4 to 52a104c Compare April 28, 2024 16:35
@YeungOnion
Copy link
Contributor

Might be better to derive PartialOrd instead, but the behavior change is good by me, it should not be constructed and it's a lot lighter than ordered_float::NotNan.

Since you're messing with it, would you rename to NonNan or NotNan to match PascalCase? It's not public, so this is just preference.

@YeungOnion YeungOnion merged commit ccf8ec8 into statrs-dev:master Apr 29, 2024
2 checks passed
@FreezyLemon FreezyLemon deleted the fix-non-canonical-partialord-impl branch April 29, 2024 11:20
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 this pull request may close these issues.

2 participants