Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rounding in implementation of ToPrimitive methods contradicts trait documentation #102

Open
p-e-w opened this issue Apr 4, 2022 · 2 comments

Comments

@p-e-w
Copy link

p-e-w commented Apr 4, 2022

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
Copy link
Contributor

MattX commented Apr 13, 2022

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
Copy link
Author

p-e-w commented May 9, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants