Fixed NaN comparison #647
Fixed NaN comparison #647
Conversation
All comparisons involving at least one NaN now return false, regardless of units
|
One question is whether NaN should be comparable with other quantities regardless of units. I think yes, it is like 0 in this regard: the answer is known to be false regardless of units. |
numbat/src/quantity.rs
Outdated
| pub(crate) enum QuantityOrdering { | ||
| IncompatibleUnits, | ||
| NanOperand, | ||
| Less, | ||
| Equal, | ||
| Greater, | ||
| } |
There was a problem hiding this comment.
Would this lead to slightly less code?
| pub(crate) enum QuantityOrdering { | |
| IncompatibleUnits, | |
| NanOperand, | |
| Less, | |
| Equal, | |
| Greater, | |
| } | |
| pub(crate) enum QuantityOrderingResult { | |
| IncompatibleUnits, | |
| NanOperand, | |
| Ok(std::cmp::Ordering), | |
| } |
numbat/src/quantity.rs
Outdated
| std::cmp::Ordering::Equal => QuantityOrdering::Equal, | ||
| std::cmp::Ordering::Greater => QuantityOrdering::Greater, | ||
| }, | ||
| None => unreachable!("unexpectedly got a None partial_cmp from non-NaN arguments"), |
There was a problem hiding this comment.
Maybe catch this with a .expect(…) above, using the same message?
Yes, we want (ignoring associativity-violations in FP math for a second) If |
|
Not sure how I accidentally closed this or why I ended up force pushing, or why the macOS job didn't complete, but this PR should be good now. |
All comparisons involving at least one NaN now return false, regardless of units
Closes #644