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

Test against MPFR #311

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Test against MPFR #311

merged 3 commits into from
Oct 29, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Oct 18, 2024

Test against MPFR via rug for infinite precision results in cases where musl might not be accurate.

@tgross35 tgross35 marked this pull request as draft October 18, 2024 21:47
@tgross35 tgross35 force-pushed the mpfr-test branch 8 times, most recently from e386722 to ded5a3e Compare October 21, 2024 22:19
@tgross35 tgross35 changed the base branch from master to tgross35/test-refactoring October 21, 2024 22:26
@tgross35 tgross35 force-pushed the mpfr-test branch 2 times, most recently from 2bd6dbb to 4d51e47 Compare October 21, 2024 22:43
@tgross35 tgross35 changed the title [wip] test against MPFR Test against MPFR Oct 21, 2024
@tgross35
Copy link
Contributor Author

I think this is ready for a review too @Amanieu. This adds tests against MPFR for almost all functions, which gives us infinite precision results in cases where we want to be more accurate than musl.

The few untested functions are at

skip: [
// FIXME: MPFR tests needed
frexp,
frexpf,
ilogb,
ilogbf,
ldexp,
ldexpf,
modf,
modff,
remquo,
remquof,
scalbn,
scalbnf,
],
.

This targets my test-refactoring branch so it can't be merged directly (requires #300).

@tgross35 tgross35 marked this pull request as ready for review October 21, 2024 22:49
@tgross35 tgross35 force-pushed the tgross35/test-refactoring branch 2 times, most recently from 09d82a7 to 2722bde Compare October 22, 2024 00:06
@tgross35 tgross35 force-pushed the mpfr-test branch 4 times, most recently from a20b0c8 to c76cece Compare October 22, 2024 00:16
@tgross35 tgross35 force-pushed the tgross35/test-refactoring branch 3 times, most recently from 6bf4102 to f778935 Compare October 22, 2024 01:02
@tgross35 tgross35 force-pushed the mpfr-test branch 2 times, most recently from b33d482 to e661694 Compare October 22, 2024 10:10
@tgross35
Copy link
Contributor Author

Cc also other @tspiteri (rug maintainer) if you get a chance to take a look at this.

self.0.assign(input.0);
self.1.assign(input.1);
self.0.next_toward(&self.1);
prep_retval::<Self::Output>(&mut self.0, Ordering::Equal)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think subnormalize_ieee_round with Round::Nearest will work correctly for next_toward: e.g. for an f32 with bit repr 0x1 (the smallest representable positive non-zero f32), the correct result for next_toward positive infinity would be the f32 with bit repr 0x2. However, the rug next_toward method will calculate the next value as if the input is a normal number, meaning that subnormalize will round away the difference, meaning the MPFR result will be incorrect. The easiest way to fix this that I can think of is to have subnormalize_ieee_round always round the result in the direction next_toward is trying to go in (so use Round::Up if self.1 > self.0 and Round::Down if self.1 < self.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch. That sounds reasonable but I think I should probably add a unit test that the MPFR behavior is correct. I think maybe it is better to have that as a followup, so I just removed this test and left a FIXME.

crates/libm-test/build.rs Outdated Show resolved Hide resolved
}
}
CheckBasis::MultiPrecision => {
if ctx.canonical_name == "copysign" && input.1.is_nan() {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, since MPFR discards NaN bits, this will also fail when input.0 is NaN (and similarly for fabs). This won't have been causing any tests to fail yet due to #300 (comment), but will do once that is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even testing locally with a much higher iteration count, I haven't seen this fail at all. I guess I will just leave it as-is, unless it turns out to be a problem later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs say this works "even when op1 or op2 is a NaN" https://www.mpfr.org/mpfr-current/mpfr.html and the code says:

   The computation of z with magnitude of x and sign of y:
   z = (-1)^signbit(y) * abs(x), i.e. with the same sign bit as y,
   even if z is a NaN.

The code does say https://gitlab.inria.fr/mpfr/mpfr/-/blob/0b0ce34f301bf75f0a4160350a179de19d9a5146/src/copysign.c#L24-30

The result does still seem surprising.

@tgross35
Copy link
Contributor Author

tgross35 commented Oct 28, 2024

The MPFR jn implementation on i686 seems to be hitting this assertion https://gitlab.inria.fr/mpfr/mpfr/-/blob/0b0ce34f301bf75f0a4160350a179de19d9a5146/src/jn.c#L250-251

@tgross35 tgross35 force-pushed the mpfr-test branch 2 times, most recently from 903ff5b to cff809e Compare October 28, 2024 06:03
@tgross35
Copy link
Contributor Author

tgross35 commented Oct 28, 2024

That assertion might actually be the better behavior, seems like large numbers of iterations can't complete in reasonable time with MPFR's jn. I assume carrying full precision through large numbers of iterations is significantly costlier than whatever approximation we are doing.

Will skip these for now.

@tgross35 tgross35 force-pushed the mpfr-test branch 2 times, most recently from a998a4e to e7ce344 Compare October 28, 2024 18:18
@tgross35 tgross35 changed the base branch from tgross35/test-refactoring to master October 28, 2024 18:19
@tgross35 tgross35 force-pushed the mpfr-test branch 2 times, most recently from eaf3ee4 to 7d89436 Compare October 28, 2024 18:43
Add a way to call MPFR versions of functions in a predictable way, using
the `MpOp` trait.

Everything new here is guarded by the feature `test-multiprecision`
since MPFR cannot easily build on Windows or any cross compiled targets.
This effectively gives us tests against infinite-precision results on
MacOS and x86+sse Linux.
@tgross35
Copy link
Contributor Author

I think the current state of this PR should be a good enough start to getting a better test basis, even if there are some rough edges still. I am going to merge in order to unblock any refactoring that builds on it, as well as to help out with the implementation changes that would benefit from the testing.

Please feel free to add further reviews here, or open issues/PRs if there are problems.

@tgross35 tgross35 merged commit 47bfc9b into rust-lang:master Oct 29, 2024
28 checks passed
@tgross35 tgross35 deleted the mpfr-test branch October 29, 2024 02:38
@tgross35 tgross35 mentioned this pull request Oct 29, 2024
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.

4 participants