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

Fix integerish conversions #757

Merged

Conversation

Ilia-Kosenkov
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov commented May 12, 2024

Fixes #720.

  • Rework scalar f64 (Rfloat) to integer type conversions (both FromRobj and TryFrom) by extracting all logic into a single FloatToInt trait.
  • Properly handle all edge cases like NA, over-/under-flow, subnormality
  • Disallow rounding casts that result in loss of accuracy (e.g., 4.2f64 no longer maps to 4i32)
  • Allow only f64 values that are exactly representable by target integer type

Also update xtask:

  • cargo extendr devtools-test now gained -f/--filter option that controls which tests to execute. Value is passed verbatim to filter parameter of devtools::test()

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@CGMossa CGMossa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm.

Personally, I didn't like the way it was done before.

We should strive for a performant conversion, if you ask me, but that's another discussion altogether.
I believe if the num crate does this in a more clever way, we ought to consider it.

Bytemuck does not do conversions. I checked. It merely reinterprets bytes.

@CGMossa
Copy link
Member

CGMossa commented May 12, 2024

Please consider this https://doc.rust-lang.org/std/primitive.f64.html#method.to_int_unchecked
Before merging.

@Ilia-Kosenkov
Copy link
Member Author

It practically does what f64 as i32 does, with perhaps tiny variations. It still will overflow/underflow to max/min value (what we observed in the original issue), it will break on various NaN, it will not propagate NA, which we definitely want. So it is practically useless to us. We are looking for a _checked version, which is not yet present.

Since I extracted the logic as much as I could, if we find a better solution to map an f64 into i32 | ConversionError, it is now easy to implement, replacing code in one place.

@Ilia-Kosenkov Ilia-Kosenkov merged commit 24c25ad into extendr:master May 12, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

Parsing f64 to i32 where the number overflows i32 causes to get the max value instead NA or overflow
2 participants