num-rational icon indicating copy to clipboard operation
num-rational copied to clipboard

Rounding in implementation of `ToPrimitive` methods contradicts trait documentation

Open p-e-w opened this issue 3 years ago • 2 comments

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 self to a u64. If the value cannot be represented by a u64, then None is 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.

p-e-w avatar Apr 04 '22 06:04 p-e-w

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 i64 with a very high magnitude might not be convertible to an i32. On the other hand, conversions with possible precision loss or truncation are admitted, like an f32 with a decimal part to an integer type, or even a large f64 saturating to f32 infinity.

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.

MattX avatar Apr 13 '22 15:04 MattX

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.

p-e-w avatar May 09 '22 04:05 p-e-w