rust-num / num-rational

Generic Rational numbers for Rust
Apache License 2.0
144 stars 50 forks source link

Rounding in implementation of `ToPrimitive` methods contradicts trait documentation #102

Open p-e-w opened 2 years ago

p-e-w commented 2 years ago

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.

MattX commented 2 years ago

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.

p-e-w commented 2 years ago

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.