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

Relative Error Checking for Floating Point Tests. Fixes #172. #181

Merged

Conversation

insertinterestingnamehere
Copy link
Collaborator

If a compiler is optimizing overly aggressively it may make assumptions about commutativity/associativity of floating point numbers that don't actually hold perfectly in practice. That's a likely cause of the current test failure with icx. The result in the test is just slightly off from the expected value. While this could arguably be a bug upstream, it's generally a bad idea to use exact float equality for tests. Relative error is a more reliable measure. This change makes the float-related tests check relative error rather than actual equality. The thresholds are very conservative, but checking this way should insulate our test suite from failures resulting in small differences in floating point handling at the compiler and/or hardware level.

…ating point results.

If a compiler is optimizing overly aggressively it may make assumptions about
commutativity/associativity of floating point numbers that don't actually hold
perfectly in practice. That's a likely cause of the current test failure with
icx. The result in the test is just slightly off from the expected value.
While this could arguably be a bug upstream, it's generally a bad idea to use
exact float equality for tests. Relative error is a more reliable measure.
This change makes the float-related tests check relative error rather than
actual equality. The thresholds are very conservative, but checking this way
should insulate our test suite from failures resulting in small differences
in floating point handling at the compiler and/or hardware level.
@janciesko
Copy link
Collaborator

LGTM

@janciesko janciesko self-requested a review November 28, 2023 21:36
@janciesko
Copy link
Collaborator

I see some of the CI tests failing - is that fixed in #182 ?

@insertinterestingnamehere
Copy link
Collaborator Author

insertinterestingnamehere commented Nov 28, 2023

The CI failures include documented but not yet fixed errors like #145, #150, and various others. Icx in particular is based on clang, so it's susceptible to most of the same bugs. This PR covers the only icx-specific issue. Any remaining icx failures also apply to clang. Generally, all the CI failures should be documented and I'm currently working through fixing them all. I figured it would be better to have a thorough but partially passing CI suite because it still lets me see regressions in partially-working configs while debugging.

#182 just covers all errors from the undefined behavior sanitizer. I also happened to pick up part of #150 while resolving those. The lingering issues in the undefined sanitizer builds there are either specific to ARM or specific to clang instead of being specific to the undefined behavior sanitizer.

@insertinterestingnamehere insertinterestingnamehere merged commit a1dcb48 into sandialabs:main Nov 29, 2023
120 of 216 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