Apply random float error #4156

LorrensP-2158466 · 2025-01-27T19:18:37Z

This partially tries to fix #3555 (does not include the pow operations)

this just does apply_random_float_error on the results of the intrinsics.
Things changed from original:

seperated the sqrtf* intrinsics to make the implementation cleaner, since applying the error was not required
changes the way we do approximation checks, instead of using the epsilon technique, we use ULP.
create the relative errors of 16ULP using ulp_err_scale, which does -(F::PRECISION - 1 - n for a 2^n ULP error.
adds a test test_non_determinism which tests a lot of non-deterministic operations with the same input to check if they are almost but not entirely equal.

RalfJung · 2025-01-29T22:53:36Z

Please also add a test (as a new function in tests/pass/float.rs) that checks for non-determinism by running the same operation multiple times and ensuring we get different (but similar) results.

src/intrinsics/mod.rs

LorrensP-2158466 · 2025-02-05T18:05:45Z

Almost done I think.

I have 1 TODO left, inside of the non-determinism test, it is about this:

Please also add a test (as a new function in tests/pass/float.rs) that checks for non-determinism by running the same operation multiple times and ensuring we get different (but similar) results.

Should we do an assert_ne! test on the results, because it is possible that the same error is applied to 2 separate operations that we then check. Maybe it is possible to count these exactly equal results and if it is the same as the amount of checks we can safely say it is deterministic and should fail the test?.

LorrensP-2158466 · 2025-02-05T18:52:12Z

@rustbot ready

RalfJung · 2025-02-05T21:19:27Z

Should we do an assert_ne! test on the results, because it is possible that the same error is applied to 2 separate operations that we then check. Maybe it is possible to count these exactly equal results and if it is the same as the amount of checks we can safely say it is deterministic and should fail the test?.

I think the easiest way is something like this; you can do that with a closure returning any PartialEq type instead of bool.

LorrensP-2158466 · 2025-02-06T17:42:42Z

@rustbot ready

RalfJung

Thanks a lot, this looks great! All comments are minor.

@rustbot author
Please post @rustbot ready when the PR is ready for the next round of review.

src/math.rs

src/intrinsics/mod.rs

RalfJung · 2025-02-08T08:27:32Z

tests/pass/float.rs

+            ulp diff: {ulp_difference} > {allowed_ulp}
+            ",
+            *a,
+            *b


This will print complete nonsense when the exponent is different, won't it?

I'm not convinced going via the hex/binary representation is the best option here.

f16 and f128 currently don't implement Display, so using debug was the only thing that worked (that i know of).

I don't understand your reply.

If the exponent is different, subtracting the two i128 representations will not be the difference in ULP, it will be complete junk. The entire logic falls apart.

So here's an alternative way to approximate the distance in ULP, but it may fail spectacularly due to rounding:

let ulp = (expected.next_up() - expected.next_down()) / 2; let ulp_diff = ((real - expected) / ulp).round();

I have to be honest, I don't see why we would use this way of calculating ULP. Isn't it unlikely to get approximately equal values with different exponents in our tests?

It's not a question of probability -- if we happen to write a test where the result is near the boundary between values represented with different exponents, we'll suddenly get completely nonsensical results. That's fragile and hard to debug. We shouldn't have surprising footguns in our code that can go off any day in the future that someone adds a new test.

tests/pass/float.rs

RalfJung · 2025-02-08T08:29:15Z

tests/pass/float.rs

+        frem_algebraic, frem_fast, fsub_algebraic, fsub_fast,
+    };
+    use std::{f32, f64};
+    // FIXME(powi_powf): add when supported


what is that about?

you mentioned in the zulip chat to not include the pow intrinsics in this pr. I should explain this better inside the comment.

Ah I see. The better place for the FIXME would be in the shim implementation.

tests/pass/float.rs

LorrensP-2158466 · 2025-02-08T13:36:39Z

tests/pass/float.rs

+/// Another form of checking if 2 floating point numbers are almost equal to eachother
+/// Using `a` and `b` as floating point numbers:
+///
+/// Instead of doing a simple EPSILON check (which we did at first):
+/// Absolute difference of `a` and `b` can't be greater than some E (10^-6, ...).
+///
+/// We look at ULP: `Units in the Last Place` or `Units of Least Precision`
+/// It is the absolute difference of the *unsigned integer* representation of `a` and `b`.
+/// For example for f32, which in IEEE format looks like this:
+///
+/// s: sign bit
+/// e: exponent bits
+/// m: significand bits
+/// f32: s | eeee eeee | mmm mmmm mmmm mmmm mmmm mmmm
+/// u32: seee eeee emmm mmmm mmmm mmmm mmmm mmmm
+///
+/// We see that checking `a` and `b` with different signs has no meaning, but we should not forget
+/// -0.0 and +0.0.
+/// Same with exponents but no zero checking.
+///
+/// So when Sign and Exponent are the same, we can get a reasonable ULP value
+/// by doing the operation explained above. And if this is less than or equal to our chosen upper bound
+/// we can say that `a` and `b` are approximately equal.
+///
+/// Basically ULP can be seen as a distance metric of floating point numbers, but having
+/// the same amount of "spacing" between consecutive numbers. So eventhough 2 very large floating point numbers
+/// have a large value difference, their ULP can still be 1, so they are still "approximatly equal",
+/// but the EPSILON check would have failed.


I only learned about this concept last week, but I tried my best. Let me know if i missed something or if something is explained badly.

Oh wow that's a lot more detailed than I expected, thanks!

LorrensP-2158466 · 2025-02-08T13:48:23Z

Fixed these CI checks in the last commit but CI never ran on that one...
Edit: it did, it still fails, but not locally
@rustbot ready

LorrensP-2158466 · 2025-02-08T14:27:17Z

@rustbot blocked

I don't know whats going on

saethlin · 2025-02-08T14:31:32Z

CI tests your branch merged with the target branch. The errors in CI look like you have a semantic merge conflict (GitHub can only automatically detect textual merge conflicts).

You should rebase onto the upstream target branch. The most foolproof way to do this is a rebase pull: git pull --rebase git@github.com:rust-lang/miri.

src/intrinsics/mod.rs

saethlin · 2025-02-08T16:55:33Z

If you still have a clean local build, run ./miri toolchain, which will probably update the toolchain override.

LorrensP-2158466 · 2025-02-08T20:20:37Z

@rustbot ready

src/intrinsics/mod.rs

src/math.rs

src/intrinsics/mod.rs

LorrensP-2158466 · 2025-02-15T13:23:11Z

Sorry for the delay, Uni started this week.

I extended the explanation of the Approx Check, let me know if this is correct and not too long?

@rustbot ready

RalfJung · 2025-02-15T17:09:14Z

tests/pass/float.rs

+fn approx_eq_check<F: Float>(
+    actual: F,
+    expected: F,
+    allowed_ulp: F::Int,
+) -> Result<(), NotApproxEq<F>>
+where
+    F::Int: PartialOrd,
+{
+    let actual_signum = actual.signum();
+    let expected_signum = expected.signum();
+
+    if actual_signum != expected_signum {
+        // Floats with different signs must both be 0.
+        if actual != expected {
+            return Err(NotApproxEq::SignsDiffer);
+        }
+    } else {
+        let ulp = (expected.next_up() - expected.next_down()).halve();
+        let ulp_diff = ((actual - expected) / ulp).round().as_int();
+
+        if ulp_diff > allowed_ulp {
+            return Err(NotApproxEq::UlpFail(ulp_diff));
+        }
+    }
+    Ok(())
+}


Now that this is just regular float arithmetic, why do we still need this trait and all the rest?

Also, most of the explanation is not relevant any more since we are not working on the bit representation any more.

Trait is still needed for other helper functions I didn't write, they were also used in macros and such so i followed that approach. It's still a lot of code and the macro will thus generate a lot of code (not sure if this matters), and there is no way to make a float generic function other than using the trait.

I'll update the explanation, let me know what you think about the rest.

Extra: Also the weird extra functions (halve, as_int, round) i had to add to the Trait were necessary to make the generic function work. Just putting in as F::int or / 2.0 did not work in any clean way other than this. It's no problem if i have to remove it if needed.

RalfJung · 2025-02-16T18:21:59Z

I found it easier to just do the remaining changes than to try to explain them, so I have updated the PR accordingly. I also rebased over the latest rustup which adds one more function that had to be tested for non-determinism, and squashed the commits. (If you want to see what I changed compared to your version, look at the 2nd commit.)

Turns out a macro-based solution still works just fine. Maybe a method is cleaner, but switching to that should not be done in the same PR as adding new functionality.

Thanks for your work!

…the float test

LorrensP-2158466 · 2025-02-16T19:41:27Z

Alright that's good. Thanks, learned a lot!

LorrensP-2158466 marked this pull request as draft January 27, 2025 19:51

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jan 29, 2025

RalfJung reviewed Jan 29, 2025

View reviewed changes

src/intrinsics/mod.rs Outdated Show resolved Hide resolved

src/intrinsics/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 5, 2025

Expose algebraic floating point intrinsics rust-lang/rust#136457

Open

LorrensP-2158466 marked this pull request as ready for review February 5, 2025 17:59

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the apply-random-float-error branch from a64db7e to b82c735 Compare February 5, 2025 18:21

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 5, 2025

RalfJung reviewed Feb 8, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Feb 8, 2025

LorrensP-2158466 commented Feb 8, 2025

View reviewed changes

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 8, 2025

rustbot added S-blocked Status: blocked on something happening somewhere else and removed S-waiting-on-review Status: Waiting for a review to complete labels Feb 8, 2025

RalfJung reviewed Feb 8, 2025

View reviewed changes

src/intrinsics/mod.rs Outdated Show resolved Hide resolved

LorrensP-2158466 force-pushed the apply-random-float-error branch from 312b028 to 25ecf26 Compare February 8, 2025 16:49

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-blocked Status: blocked on something happening somewhere else labels Feb 8, 2025

RalfJung reviewed Feb 10, 2025

View reviewed changes

src/intrinsics/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 10, 2025

View reviewed changes

src/intrinsics/mod.rs Show resolved Hide resolved

RalfJung reviewed Feb 10, 2025

View reviewed changes

src/math.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 10, 2025

View reviewed changes

src/intrinsics/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 15, 2025

View reviewed changes

RalfJung force-pushed the apply-random-float-error branch from 10ece47 to e095b40 Compare February 16, 2025 18:13

apply random float error to most floating-point operations

f5330d0

RalfJung force-pushed the apply-random-float-error branch from 1e977f7 to 5c09010 Compare February 16, 2025 18:19

RalfJung enabled auto-merge February 16, 2025 18:22

RalfJung force-pushed the apply-random-float-error branch from 5c09010 to 269a7f5 Compare February 16, 2025 18:23

add erf and erfc to nondet tests, and reduce how much we're changing …

5c9eaa3

…the float test

RalfJung force-pushed the apply-random-float-error branch from 269a7f5 to 5c9eaa3 Compare February 16, 2025 18:24

RalfJung added this pull request to the merge queue Feb 16, 2025

Merged via the queue into rust-lang:master with commit 826e72f Feb 16, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply random float error #4156

Apply random float error #4156

LorrensP-2158466 commented Jan 27, 2025 •

edited

Loading

RalfJung commented Jan 29, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

LorrensP-2158466 commented Feb 5, 2025

LorrensP-2158466 commented Feb 5, 2025

RalfJung commented Feb 5, 2025

LorrensP-2158466 commented Feb 6, 2025

RalfJung left a comment

RalfJung Feb 8, 2025 •

edited

Loading

LorrensP-2158466 Feb 8, 2025

RalfJung Feb 8, 2025 •

edited

Loading

RalfJung Feb 8, 2025

LorrensP-2158466 Feb 8, 2025

RalfJung Feb 10, 2025 •

edited

Loading

RalfJung Feb 8, 2025

LorrensP-2158466 Feb 8, 2025

RalfJung Feb 8, 2025

LorrensP-2158466 Feb 8, 2025

RalfJung Feb 8, 2025

LorrensP-2158466 commented Feb 8, 2025 •

edited

Loading

LorrensP-2158466 commented Feb 8, 2025

saethlin commented Feb 8, 2025

saethlin commented Feb 8, 2025

LorrensP-2158466 commented Feb 8, 2025

LorrensP-2158466 commented Feb 15, 2025

RalfJung Feb 15, 2025

LorrensP-2158466 Feb 15, 2025

LorrensP-2158466 Feb 15, 2025

RalfJung commented Feb 16, 2025

LorrensP-2158466 commented Feb 16, 2025

Apply random float error #4156

Apply random float error #4156

Conversation

LorrensP-2158466 commented Jan 27, 2025 • edited Loading

RalfJung commented Jan 29, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

LorrensP-2158466 commented Feb 5, 2025

LorrensP-2158466 commented Feb 5, 2025

RalfJung commented Feb 5, 2025

LorrensP-2158466 commented Feb 6, 2025

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorrensP-2158466 commented Feb 8, 2025 • edited Loading

LorrensP-2158466 commented Feb 8, 2025

saethlin commented Feb 8, 2025

saethlin commented Feb 8, 2025

LorrensP-2158466 commented Feb 8, 2025

LorrensP-2158466 commented Feb 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Feb 16, 2025

LorrensP-2158466 commented Feb 16, 2025

LorrensP-2158466 commented Jan 27, 2025 •

edited

Loading

RalfJung commented Jan 29, 2025 •

edited

Loading

RalfJung Feb 8, 2025 •

edited

Loading

RalfJung Feb 8, 2025 •

edited

Loading

RalfJung Feb 10, 2025 •

edited

Loading

LorrensP-2158466 commented Feb 8, 2025 •

edited

Loading