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

Replace HasDomain to enable multi-argument edge case and domain tests #415

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jan 7, 2025

This also allows reusing the same generator logic between logspace tests and extensive tests, so comes with a nice bit of cleanup.

Changes:

  • Make the generator part of CheckCtx since a Generator and CheckCtx are almost always passed together.
  • Rename domain_logspace to spaced since this no longer only operates within a domain and we may want to handle integer spacing.
  • Domain is now calculated at runtime rather than using traits, which is much easier to work with.
  • With the above, domains for multidimensional functions are added.
  • The extensive test generator code tests has been combined with the domain_logspace generator code. With this, the domain tests have just become a subset of extensive tests. These were renamed to "quickspace" since, technically, the extensive tests are also "domain" or "domain logspace" tests.
  • Edge case generators now handle functions with multiple inputs.
  • The test runners can be significantly cleaned up and deduplicated.

@tgross35
Copy link
Contributor Author

tgross35 commented Jan 13, 2025

The improved edge case tests are showing some inputs where our results differ from spec, which is a great thing.

thread 'musl_edge_case_fmax' panicked at crates/libm-test/tests/compare_built_musl.rs:26:51:
called `Result::unwrap()` on an `Err` value: 
    input:    (-0.0, 0.0) (0x8000000000000000, 0x0000000000000000)
    expected: 0.0                    0x0000000000000000
    actual:   -0.0                   0x8000000000000000

Caused by:
    mismatched signs -1.0 1.0

---- musl_edge_case_fmaxf stdout ----
EdgeCases Musl fmaxf arg 1/2: 86 edge cases
EdgeCases Musl fmaxf arg 2/2: 86 edge cases

thread 'musl_edge_case_fmaxf' panicked at crates/libm-test/tests/compare_built_musl.rs:26:51:
called `Result::unwrap()` on an `Err` value: 
    input:    (-0.0, 0.0) (0x80000000, 0x00000000)
    expected: 0.0                    0x00000000
    actual:   -0.0                   0x80000000

Caused by:
    mismatched signs -1.0 1.0

---- musl_edge_case_fmin stdout ----
EdgeCases Musl fmin arg 1/2: 86 edge cases
EdgeCases Musl fmin arg 2/2: 86 edge cases

thread 'musl_edge_case_fmin' panicked at crates/libm-test/tests/compare_built_musl.rs:26:51:
called `Result::unwrap()` on an `Err` value: 
    input:    (-0.0, 0.0) (0x8000000000000000, 0x0000000000000000)
    expected: -0.0                   0x8000000000000000
    actual:   0.0                    0x0000000000000000

Caused by:
    mismatched signs 1.0 -1.0


---- musl_edge_case_fminf stdout ----
EdgeCases Musl fminf arg 1/2: 86 edge cases
EdgeCases Musl fminf arg 2/2: 86 edge cases

thread 'musl_edge_case_fminf' panicked at crates/libm-test/tests/compare_built_musl.rs:26:51:
called `Result::unwrap()` on an `Err` value: 
    input:    (-0.0, 0.0) (0x80000000, 0x00000000)
    expected: -0.0                   0x80000000
    actual:   0.0                    0x00000000

Caused by:
    mismatched signs 1.0 -1.0

The C spec has a footnote: "Ideally, fmax would be sensitive to the sign of zero, for example fmax(−0.0, +0.0) would return +0; however, implementation in software might be impractical.", but based on a comment in our source for these functions, it sounds like IEEE may say something slightly different. Something to look into more, I can xfail these for now.

Edit: opened #439 to track this.

@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch 4 times, most recently from daad206 to cf0d9a3 Compare January 13, 2025 21:55
@tgross35 tgross35 marked this pull request as ready for review January 13, 2025 21:55
@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch from cf0d9a3 to 7788a3d Compare January 13, 2025 22:01
@tgross35
Copy link
Contributor Author

This should be nearly done, I just need to double check the test count logic. Cc @beetrees since this PR interacts with your recent changes.

@tgross35
Copy link
Contributor Author

tgross35 commented Jan 13, 2025

Oh no, we have a buf in fma on i686? Happens on both i586 and i686:

---- mp_edge_case_fma stdout ----
EdgeCases Mpfr fma arg 1/3: 84 edge cases
EdgeCases Mpfr fma arg 2/3: 84 edge cases
EdgeCases Mpfr fma arg 3/3: 84 edge cases

thread 'mp_edge_case_fma' panicked at crates/libm-test/tests/multiprecision.rs:17:48:
called `Result::unwrap()` on an `Err` value: 
    input:    (0.999999999999999, 1.0000000000000013, 0.0) (0x3feffffffffffff7, 0x3ff0000000000006, 0x0000000000000000)
    expected: 1.0000000000000002     0x3ff0000000000001
    actual:   1.0000000000000004     0x3ff0000000000002

Caused by:
    ulp 1 > 0

@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch from 7788a3d to 7028a91 Compare January 13, 2025 22:16
@tgross35 tgross35 changed the title Replace HasDomain with a dynamic call that supports multiple arguments Replace HasDomain to enable multiple arguments for edge cases and domain tests Jan 13, 2025
@tgross35 tgross35 changed the title Replace HasDomain to enable multiple arguments for edge cases and domain tests Replace HasDomain to enable multi-argument edge case and domain tests Jan 13, 2025
@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch 3 times, most recently from 32c2652 to 8ba7388 Compare January 14, 2025 00:50
@tgross35
Copy link
Contributor Author

I need to cut down the CI runtime somehow. Time reporting from #441 should help figure out what is taking so long.

@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch 4 times, most recently from 2ba42ee to eacb9f5 Compare January 16, 2025 01:05
This also allows reusing the same generator logic between logspace tests
and extensive tests, so comes with a nice bit of cleanup.

Changes:

* Make the generator part of `CheckCtx` since a `Generator` and
  `CheckCtx` are almost always passed together.
* Rename `domain_logspace` to `spaced` since this no longer only
  operates within a domain and we may want to handle integer spacing.
* Domain is now calculated at runtime rather than using traits, which is
  much easier to work with.
* With the above, domains for multidimensional functions are added.
* The extensive test generator code tests has been combined with the
  domain_logspace generator code. With this, the domain tests have just
  become a subset of extensive tests. These were renamed to "quickspace"
  since, technically, the extensive tests are also "domain" or "domain
  logspace" tests.
* Edge case generators now handle functions with multiple inputs.
* The test runners can be significantly cleaned up and deduplicated.
@tgross35 tgross35 force-pushed the domain-multiple-dimensions branch from eacb9f5 to 2fab7e0 Compare January 16, 2025 01:10
@tgross35
Copy link
Contributor Author

tgross35 commented Jan 16, 2025

It would probably be nice to somehow reduce the number of iterations that run for bessel functions, since that is the biggest chunk of CI time. And we are still not testing jn/yn because MPFR crashes (I still need to look into it more, I could just be doing something incorrectly). But a ~seven minute increase in CI time here is tolerable for much better coverage of edge cases.

#446 should eventually win some time back.

@tgross35 tgross35 enabled auto-merge January 16, 2025 01:32
@tgross35 tgross35 merged commit 8ded576 into rust-lang:master Jan 16, 2025
35 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.

1 participant