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

Define consistent float ordering #808

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Define consistent float ordering #808

merged 2 commits into from
Sep 16, 2024

Conversation

robert3005
Copy link
Member

@robert3005 robert3005 commented Sep 15, 2024

We use total_cmp instead of partial_cmp to compare floating point numbers and use bit patern to determine float equality

@lwwmanning
Copy link
Member

The downside though is that sorting an array of floats won't give what users probably expect?

@robert3005
Copy link
Member Author

I don’t think anyone has expectation how nans sort. There’s no way to sort arbitrary float arrays without this

@lwwmanning
Copy link
Member

Right, but the bit wise ordering of non-NaN floats isn't the same as the natural ordering?

@robert3005
Copy link
Member Author

Hm, you're right. I think I went too far. You can define equality using bit equality

@robert3005 robert3005 changed the title Define consistent NaN ordering Define consistent float ordering Sep 15, 2024
Comment on lines +52 to +54
fn compare(self, other: Self) -> Ordering;

fn is_eq(self, other: Self) -> bool;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't these two be expressed with additional trait bounds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can define an additional traits but afaik there isn't one that has the behaviour as implemented here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just Ord?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I got it, its about the custom implementation

Base automatically changed from rk/fuzz to develop September 16, 2024 10:11
@robert3005 robert3005 enabled auto-merge (squash) September 16, 2024 10:16
@robert3005 robert3005 merged commit 02b752c into develop Sep 16, 2024
5 checks passed
@robert3005 robert3005 deleted the rk/nanordering branch September 16, 2024 10:33
@@ -30,7 +31,6 @@ pub enum PType {
pub trait NativePType:
Send
+ Sync
+ Sized
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no longer Sized?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

3 participants