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

Point out that total_cmp is no strict superset of partial comparison #78627

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

est31
Copy link
Member

@est31 est31 commented Nov 1, 2020

Partial comparison and total_cmp are not equal. This helps
preventing the mistake of creating float wrappers that
base their Ord impl on total_cmp and their PartialOrd impl on
the PartialOrd impl of the float type. PartialOrd and Ord
are required to agree with each other.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 1, 2020
@est31
Copy link
Member Author

est31 commented Nov 1, 2020

Got the idea for this while looking at #78618.

@tavianator
Copy link
Contributor

I'm not sure "superset" is the right word -- what sets are being compared exactly? Maybe something like "this ordering is not compatible with the default ordering" or "this ordering does not always agree with the default ordering"

@workingjubilee
Copy link
Member

Considering a poset, there are items which are comparable and items which are not (this is the definition of a partially ordered set), thus there is a subset of the poset which could be considered to be a properly totally ordered set if the other items were ignored.

Usually it is imagined that one could impose this on floats if one just pretended the NaNs did not exist, but this is a flawed consideration for floats due to -0 and 0. -0 <= 0 and 0 <= -0 in the "float order", thus -0 == 0, but that means two members partake of the reflexive relation when they are not actually the same. So in total_cmp that symmetry is deliberately broken as -0 is then < 0. It produces a proper linear order not just by ordering the remaining possible float values and in doing so making all members of the set of floating point values orderable, but by altering all comparisons, including comparisons that had previously flattened differences between members. So the language is arguably correct not because the terms are correct to use, but rather because the mental notion of floats having a proper poset within the set of numbers is what needs to be broken.

So the goal of any wording here is naturally to be disruptive: "Remember, This Is Probably Not What You Think It Is", which means it will contain the linguistic oddity of referencing an idea which does not apply.

@est31
Copy link
Member Author

est31 commented Nov 1, 2020

@tavianator I'm using the definition that defines partial functions as sets, specifically defining functions f : X → Y as subsets f ⊆X ⨯ Y for which holds that if (x, y) ∈ f and (x, z) ∈ f then y == z (image of each element is unique). Notation then writes (x,y) ∈ f as f(x) = y. If there is y so that f(x) = y, we call f defined at x. If there is no such y, then we call f not defined at x. A total function is a partial function which is defined at all x ∈ X.

In this instance, X is the set of all (f64, f64) tuples (or (f32, f32) tuples), and Ordering := { Less, Greater, Equal } is the three element enum. Then partial_cmp is a partial function partial_cmp : X → Ordering, while total_cmp is a total function total_cmp : X → Ordering.

The concept of subsets of functions then just maps down to the concept of subsets of sets.

@est31
Copy link
Member Author

est31 commented Nov 1, 2020

I think I'll just change the text to forestall further confusion...

@m-ou-se
Copy link
Member

m-ou-se commented Nov 1, 2020

It might be good to specifically mention the PartialOrd trait. Maybe something like 'the comparison provided by the PartialOrd implementation'.

@tavianator
Copy link
Contributor

I like the new wording. I think it's important that documentation like this is understandable without a mathematical background.

It's interesting that the old wording has (at least) two natural interpretations of what the "set" in question is, but "superset" means the right thing in either case.

Partial comparison and total_cmp are not equal. This helps
preventing the mistake of creating float wrappers that
base their Ord impl on total_cmp and their PartialOrd impl on
the PartialOrd impl of the float type. PartialOrd and Ord
are required to agree with each other.
@m-ou-se
Copy link
Member

m-ou-se commented Nov 1, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 1, 2020

📌 Commit a79059d has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 1, 2020
Point out that total_cmp is no strict superset of partial comparison

Partial comparison and total_cmp are not equal. This helps
preventing the mistake of creating float wrappers that
base their Ord impl on total_cmp and their PartialOrd impl on
the PartialOrd impl of the float type. PartialOrd and Ord
[are required to agree with each other](https://doc.rust-lang.org/std/cmp/trait.Ord.html#how-can-i-implement-ord).
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#78606 (Clarify handling of final line ending in str::lines())
 - rust-lang#78610 (Do not remove tokens before AST json serialization)
 - rust-lang#78620 (Trivial fixes to bitwise operator documentation)
 - rust-lang#78627 (Point out that total_cmp is no strict superset of partial comparison)
 - rust-lang#78637 (Add fetch_update methods to AtomicBool and AtomicPtr)

Failed merges:

r? `@ghost`
@bors bors merged commit fb7948e into rust-lang:master Nov 2, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants