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

Implement ties-to-even for Big[U]Int::to_{f32,f64} #271

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

dramforever
Copy link
Contributor

This makes the rounding consistent with primitive types, which, per IEEE-754, uses nearest-ties-to-even rounding.

See: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a6ab9b18a4982f881cd594061f096f8c

#[test]
fn test() {
    use num::{BigInt, ToPrimitive};
    let a: i128 = (1 << 120) + (1 << (120 - 53));
    let b: i128 = 1 << (120 - 60);
    assert_ne!(a as f64, (a + 1) as f64);
    assert_eq!((a + b) as f64, (a + 1) as f64);

    assert_eq!(BigInt::from(a).to_f64().unwrap(), a as f64);
    
    // This works because the b is among the high 64 bits
    assert_eq!(BigInt::from(a + b).to_f64().unwrap(), (a + b) as f64);

    // This fails because 1 is just thrown away
    assert_eq!(BigInt::from(a + 1).to_f64().unwrap(), (a + 1) as f64);
}

@dramforever dramforever marked this pull request as ready for review April 27, 2023 14:15
@dramforever
Copy link
Contributor Author

I'm not quite sure how to characterize this.

  • Is this a bug? It's certainly pretty surprising.
  • Should I add this to the changelog? Is this a breaking change?

@cuviper
Copy link
Member

cuviper commented Apr 27, 2023

In general, the goal is to match the behavior of primitive types, at least so far as that behavior does not depend on limited bit width. So yes, having an example of different i128 behavior makes this look like a bug we should fix.

Breaking changes are fuzzy when it comes to behavior -- I think this is a justified change, but yes we should mention it in release notes. There's semi-related precedent in how Rust 1.67 fixed rounding in rust-lang/rust#102935.

tests/bigint.rs Outdated Show resolved Hide resolved
@dramforever

This comment was marked as outdated.

@dramforever dramforever force-pushed the to-float-ties-to-even branch 2 times, most recently from 2bc992c to c75c117 Compare May 2, 2023 22:26
BigUInt::to_{f32,f64} uses only the highest 64 bits as the mantissa,
discarding the rest, which gives an incorrect result when later
converted to float using IEEE-754's default nearest-ties-to-even
rounding.

Fixed to use correct round-to-odd for taking the mantissa, which means
setting the LSB to 1 if any of the lower (i.e. rounded away) bits are 1.

This also fixes the problem for BigInt since it really just uses BigUInt
under the hood.
@cuviper
Copy link
Member

cuviper commented Aug 22, 2023

Thanks! I just rebased this without changes to get CI running.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 22, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9dbf8d6 into rust-num:master Aug 22, 2023
3 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.

2 participants