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

Regression: Difference in float values #93535

Closed
jnqnfe opened this issue Feb 1, 2022 · 4 comments
Closed

Regression: Difference in float values #93535

jnqnfe opened this issue Feb 1, 2022 · 4 comments
Labels
A-floating-point Area: Floating point numbers and arithmetic A-time Area: Time T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@jnqnfe
Copy link
Contributor

jnqnfe commented Feb 1, 2022

My libpulse-binding crate has a set of time handling functionality with some conversion based tests. Today's monthly automated CI run has alerted me that some of the tests are failing with the current nightly compiler. Specifically assertions are failing with slightly different floating point results than expected. It seems that a presumed regression/breaking-change/unexpected-change has snuck into the nightlies.

I've determined that the nightly that introduced this difference is nightly-2022-01-28.

Details here:

@SNCPlay42
Copy link
Contributor

Bisection points to the rollup #93352, which contains PR #90247.

This looks like an issue of floating point precision, e.g. the decimal 2.3 isn't exactly representable as an f32 and now Duration::from_secs_f32 is picking a different rounding. Although this is evidently technically a breaking change, I'm not sure what guarantees are provided here.

@rustbot label regression-from-stable-to-nightly T-libs A-time A-floating-point


searched nightlies: from nightly-2022-01-26 to nightly-2022-02-01
regressed nightly: nightly-2022-01-28
searched commit range: 6abb638...21b4a9c
regressed commit: 009c1d0

bisected with cargo-bisect-rustc v0.6.1

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --preserve -- test 

@rustbot rustbot added A-floating-point Area: Floating point numbers and arithmetic A-time Area: Time regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 1, 2022
@jnqnfe
Copy link
Contributor Author

jnqnfe commented Feb 1, 2022

@SNCPlay42 Thank you for identifying the relevant PR. If this is intentional and improves accuracy, as seems to be the case, then I have no wish personally for this to be "fixed", though being a breaking change it should be documented in the release notes when the time comes.

I'll have to go away and pick different values or something to work around it.

@Urgau
Copy link
Member

Urgau commented Feb 1, 2022

Floating points numbers are very finicky. You should never try to compare them directly, you should instead use comparison with some tolerance. See the book of the float_eq crate for more information's: https://jtempest.github.io/float_eq-rs/book/introduction.html

@nagisa
Copy link
Member

nagisa commented Feb 1, 2022

This looks like an issue of floating point precision, e.g. the decimal 2.3 isn't exactly representable as an f32 and now Duration::from_secs_f32 is picking a different rounding.

Precision is exactly the reason this issue arises. The previous implementation was discarding a significant amount of precision, and the new one is to the best of my knowledge lossless. While rounding mode did indeed change from round-to-nearest to truncating, it is important to note that:

  1. truncation matches the rest of the Duration APIs (such as Duration::subsec_micros); and
  2. in the previous implementation rounding was occurring as a result of an intermediate floating point multiplication with a 1e9 before another operation would truncate the result anyway.

These are actual values stored in the f32 for two of the failing tests (https://www.h-schmidt.net/FloatConverter/IEEE754.html is a nice way to verify this):

  • 2.3f32 is actually 2.299_999_952_316...f32
  • 3.14f32 is actually 3.140_000_104_904...f32

And these are exactly the values the new implementation produces after truncating conversion.

@jnqnfe jnqnfe closed this as completed Feb 2, 2022
@apiraino apiraino removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-floating-point Area: Floating point numbers and arithmetic A-time Area: Time T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants