Extend the documentation of PartialOrd #53386
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I tried to merge all the comments of https://github.com/LukasKalbertodt/partial-cmp-docs/pull/1/files , but there is still one unresolved issue open by @RalfJung :
Also, i'm not a native speaker, and this is a tricky topic, so it would be cool if someone from the docs team would take a look at this and further process it for easy consumption by users (cc @steveklabnik ) |
| @@ -8,17 +8,16 @@ | |||
| // option. This file may not be copied, modified, or distributed | |||
| // except according to those terms. | |||
|
|
|||
| //! Functionality for ordering and comparison. | |||
| //! Functionality for ordering and relational operations. | |||
There was a problem hiding this comment.
Should this be just relational operations ? This is what comparisons are.
There was a problem hiding this comment.
I wouldn't mind keeping the old version ("ordering and comparisons"). I don't think we gain a lot from being precise here.
| //! | ||
| //! The distinction between these traits is important: it tell us what it _means_ for a | ||
| //! sequence of values to be ordered. |
There was a problem hiding this comment.
This is the pragmatic way I choose to put it, might not be the best one.
@LukasKalbertodt I basically decided to put the "intro docs" to PartialOrd at the top level of the module. Otherwise the docs of PatialOrd and other traits end up restating the same things over and over again and is hard to keep them in sync.
There was a problem hiding this comment.
Typo: "it tellS us"
I think putting the text describing the differences in the mod documentation is a good idea.
There was a problem hiding this comment.
The second part of this sentence does not talk about a distinction, so I am confused what this is even telling us.
There was a problem hiding this comment.
Actually I am not even sure which distinction you mean: PartialEq vs Eq, or Eq vs Ord...
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| //! | ||
| //! Two commonly encountered examples are integers and floating-point numbers. For example, every [`i32`] | ||
| //! value is comparable to every other. If you can tell that two `i32` values are distinc, then these two | ||
| //! values never compare equal, and if you sorte a sequence of `i32` values like `[1, 1, 0, -5]` using `<=`, |
There was a problem hiding this comment.
Just a small nit while reading the new docs: sort
There was a problem hiding this comment.
Typo: "distinc" => "distinct"
| //! order, and denote this by implementing `Ord` for `i32`. | ||
| //! | ||
| //! However, none of this holds for IEEE-754 floating-point numbers like `f32`. There is a special `f32` value | ||
| //! called "Not a Number" (`NaN`) for which any relational operation alwys return false, that is: |
|
Ping from triage @steveklabnik! This PR needs your review. |
There was a problem hiding this comment.
I finally managed to review this. Sorry for the delay!
And thanks a lot for pushing this topic.
I left a few comments. Many are just typos or minor things. However, as you can see below, I'm not quite happy with changes in the module docs -- but maybe it's just me.
Also note that all "I would..." comments should be taken with a grain of salt since I'm not a native speaker.
Oh and maybe you could summarize the prior discussion and what this PR mainly wants to achieve so that @steveklabnik can start reviewing this more quickly.
| @@ -8,17 +8,16 @@ | |||
| // option. This file may not be copied, modified, or distributed | |||
| // except according to those terms. | |||
|
|
|||
| //! Functionality for ordering and comparison. | |||
| //! Functionality for ordering and relational operations. | |||
There was a problem hiding this comment.
I wouldn't mind keeping the old version ("ordering and comparisons"). I don't think we gain a lot from being precise here.
| //! | ||
| //! # Examples | ||
| //! state that values of a type can be compared for equality, or ordered, but that | ||
| //! not all values of the type can be compared to all other values (more on this below). |
There was a problem hiding this comment.
I guess this can be removed as it appears below already?
| //! This module also provides the following two traits: | ||
| //! | ||
| //! * [`Eq`]: which requires `PartialEq`, states that all values of a type can be | ||
| //! compared to all others, |
There was a problem hiding this comment.
Alternative:
Eq: marker trait which states that all values of a type can be compared to all other values of the same type (requiresPartialEq<Self>)
| //! | ||
| //! The distinction between these traits is important: it tell us what it _means_ for a | ||
| //! sequence of values to be ordered. |
There was a problem hiding this comment.
Typo: "it tellS us"
I think putting the text describing the differences in the mod documentation is a good idea.
| //! | ||
| //! Two commonly encountered examples are integers and floating-point numbers. For example, every [`i32`] | ||
| //! value is comparable to every other. If you can tell that two `i32` values are distinc, then these two | ||
| //! values never compare equal, and if you sorte a sequence of `i32` values like `[1, 1, 0, -5]` using `<=`, |
There was a problem hiding this comment.
Typo: "distinc" => "distinct"
| //! the types they accept with the [`Eq`] and [`Ord`] traits, while those for which a partial order or partial equality | ||
| //! is enough can use the `PartialEq` and `PartialOrd` traits. | ||
| //! | ||
| //! This leads us to the final question: which traits should a type implement ? There is no easy answer: a type should only |
There was a problem hiding this comment.
Nit: space before ?. Also: the lines are pretty long here.
| /// Trait for values that can be partially ordered. | ||
| /// | ||
| /// This traits defines the operators `<`, `>`, `<=`, and `>=` which compare two values and return `bool`, | ||
| /// where `<` and `>` form [_strict_ partial ordering relations][wiki_porder]. |
There was a problem hiding this comment.
I would split this sentence. "[...] return bool. The operators < and > form [...]"
There was a problem hiding this comment.
I'd move the remark about strict ordering relations far down to all the corollaries.
| /// This traits defines the operators `<`, `>`, `<=`, and `>=` which compare two values and return `bool`, | ||
| /// where `<` and `>` form [_strict_ partial ordering relations][wiki_porder]. | ||
| /// | ||
| /// The `PartialOrd` trait has fewer requirements of the ordering relation than Ord. In particular, `partial_cmp` |
There was a problem hiding this comment.
Nits: two spaces after trait and Ord not in monofont
There was a problem hiding this comment.
Also "requirements on the ordering". I think.
| /// From these rules it follows that `PartialOrd` must be implemented symmetrically and transitively. | ||
| /// That is, for two distinct types `T` and `U`, `T: PartialOrd<U>` requires the following trait | ||
| /// implementations to exist and satisfy these rules: `T: PartialOrd<T>`, `U: PartialOrd<T>`, | ||
| /// and `U: PartialOrd<U>`. If for a third distinct type `V` `T: PartialOrd<V>` is provided, then |
There was a problem hiding this comment.
Mhhh I don't quite get why T: PartialOrd<T> and U: PartialOrd<U> are required from the rules above. I am probably just missing something obvious...
There was a problem hiding this comment.
This is required by transitivity (if a < b and b < c then a < c).
Suppose that a: T, b: U, and c: T., and that a < b and b < c are both true. For a < c to also be true it would need to be well-typed, and that requires T: PartialOrd<T>.
There was a problem hiding this comment.
Ah right, that makes sense. Thanks :)
| /// Stronger ordering relations can be expressed by using the `Eq` and `Ord` traits, where the `PartialOrd` | ||
| /// methods provide: | ||
| /// | ||
| /// * a [_non-strict_ partial ordering relation](https://en.wikipedia.org/wiki/Partially_ordered_set#Strict_and_non-strict_partial_orders) |
There was a problem hiding this comment.
Did you mean to use [wiki_porder] here? Since you added the link at the bottom...
|
cc @RalfJung could you comment on these changes? I will then go and update the PR with both your feedback, if you agree with @LukasKalbertodt feedback a 👍 on their comments would be great! |
| //! | ||
| //! # Examples | ||
| //! state that values of a type can be compared for equality, or ordered, but that |
There was a problem hiding this comment.
I find this hard to parse, where does the sentence even begin and what is the subject matching this verb?
| //! * [`Eq`]: which requires `PartialEq`, states that all values of a type can be | ||
| //! compared to all others, | ||
| //! * [`Ord`]: which requires `PartialOrd` and `Eq`, states that the ordering | ||
| //! relations expressed by the comparison operators form all total ordering relations |
There was a problem hiding this comment.
What does it mean to form "all total ordering relations"?
Or do you mean, "all the comparison operators form total ordering relations"?
| //! `a == NaN` is always `false`, even when `a` is `NaN`. For this reason `f32` only implements `PartialOrd`, and it | ||
| //! does not implement neither `Eq` nor `Ord`. What does this mean in practice? It means two things: | ||
| //! | ||
| //! * there are sequences of `f32`s that we can order using `<=` and will always produce the same unique |
| //! * there are sequences of `f32`s that we can order using `<=` and will always produce the same unique | ||
| //! result: sorting `[3.0, 5.0, 1.0]` into `[1.0, 3.0, 5.0]`. | ||
| //! | ||
| //! * there are also sequences of `f32`s that when ordered using `<=` will produce a different result depending |
| //! | ||
| //! Another example is the "subset relation" on sets in which, for sets `a` and `b`, `a < b` is true if and only | ||
| //! if `a` is a proper subset of `b` (e.g. `{4, 9} < {1, 4, 5, 9}` is true). The problem is that when neither | ||
| //! `a` is a subset of `b` nor `b` is a subset of `a` nor `a = b` (e.g. `{1, 3}` and `{1, 4}`), we cannot sensible |
| //! `a` is a subset of `b` nor `b` is a subset of `a` nor `a = b` (e.g. `{1, 3}` and `{1, 4}`), we cannot sensible | ||
| //! compare `a` and `b`, so that `a < b`, `a == b`, and `a > b` all return false. | ||
| //! | ||
| //! The distinction between a _total_ order and a _partial_ order is important, and impacts what it means for |
There was a problem hiding this comment.
This refers to the distinction made above, right? It should be "This distinction" then.
Generally, I feel this needs more structure. All the text explaining partial vs total order should be in some kind of appropriately titeled subsection.
| //! total order for floating point numbers that is more expensive to compute. So which ordering should they implement? Arguably, | ||
| //! they should offer both orders and let the user choose, but they can only implement the trait once. For these kind of types, | ||
| /// for which multiple orderings exist, it might make sense to not implement any order at all, but use another mechanism | ||
| //! (e.g. a wrapper type) to allow users to select the order they want. |
There was a problem hiding this comment.
I don't see what's so hard about this TBH. Once you have a type and a relation (or two, = and <) fixed, there is no doubt any more about which traits to implement.
| /// Trait for values that can be compared for a sort-order. | ||
| /// Trait for values that can be partially ordered. | ||
| /// | ||
| /// This traits defines the operators `<`, `>`, `<=`, and `>=` which compare two values and return `bool`, |
| /// transitively: if `T: PartialOrd<U>` and `U: PartialOrd<V>` then `U: PartialOrd<T>` and `T: | ||
| /// PartialOrd<V>`. | ||
| /// * _transitivity_: if `a < b` and `b < c` then `a < c` | ||
| /// * _duality_: if `a < b` then `b > a` |
There was a problem hiding this comment.
Shouldn't it say somewhere that consistency with == from PartialEq is required?
Basically, this trait contains strictly more information than PartialEq, and it must be the case that == behaves exactly as if implemented via partial_cmp == Some(Equal).
|
I feel like the module introduction spends lots of text explaining things most users will not find relevant... all this array sorting and so on. I would start by introducing the 4 traits, and then explain that the For
[transitivity and duality go here]
Then go on about various corollaries (my open issue would be resolved by remarking that transitivity and duality still hold after swapping |
TimNN
added
S-waiting-on-author
|
Ping from triage, @gnzlbg: Skimming the comments above, I believe this PR requires an update from you. |
|
Ping from triage @gnzlbg: it looks like this PR needs to be updated. |
|
Ping from triage @gnzlbg: I'm closing this due to inactivity per the triage procedure. Thank you for your contribution and please feel free to (re-)open this or another PR in the future. |
TimNN
added
S-inactive
Closes #50230 .
cc @ExpHP @RalfJung @LukasKalbertodt @fbstj