num-rational
num-rational copied to clipboard
Rounding in implementation of `ToPrimitive` methods contradicts trait documentation
Ratio's implementation of ToPrimitive methods currently looks like this:
fn to_u64(&self) -> Option<u64> {
self.to_integer().to_u64()
}
But the documentation for to_u64 in the ToPrimitive trait states (emphasis added):
Converts the value of
selfto au64. If the value cannot be represented by au64, thenNoneis returned.
The current implementation doesn't match the documentation. Non-integral Ratio values cannot be represented as u64, so calling to_u64 on them should return None according to the documentation. Instead, it returns the rounded value, provided it is within the range of a u64.
Either the implementation or the documentation should be adjusted.
The trait documentation gives a more precise definition of being representable, which matches the current behavior:
A value can be represented by the target type when it lies within the range of scalars supported by the target type. For example, a negative integer cannot be represented by an unsigned integer type, and an
i64with a very high magnitude might not be convertible to ani32. On the other hand, conversions with possible precision loss or truncation are admitted, like anf32with a decimal part to an integer type, or even a largef64saturating tof32infinity.
It's true that it's easy to miss if just looking at individual function documentation, so maybe it needs to be made more explicit.
Thanks for pointing this out, indeed I had missed that part of the docs.
That being said, I still believe there is a problem here. Redefining terms that already have a commonly understood meaning is a bad idea. Under the definition from this crate, π "can be represented" by a single byte, since 0 <= π <= 255. The "representation" here implies π = 3. This just screams wrong. And the part where a large but finite floating point number might be "represented" as infinity sounds even worse.