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

Loosen numerically iffy test tolerance #1435

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

esseivaju
Copy link
Contributor

The InvoluteSolver test fails on Perlmutter because the precision is too strict.

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:392: Failure
Value of: dist_on[0]
  Actual: 0.0017713715293239856
Expected: 0.0017713715293786088
Which is: 0.0017713715293786088
(Relative error -3.0836664554021502e-11 exceeds tolerance 9.9999999999999998e-13)

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:409: Failure
Value of: dist_off[0]
  Actual: 0.0017713715293239856
Expected: 0.0017713715293786088
Which is: 0.0017713715293786088
(Relative error -3.0836664554021502e-11 exceeds tolerance 9.9999999999999998e-13)

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:432: Failure
Value of: dist_on[0]
  Actual: 0.00053216327768802003
Expected: 0.00053216327674743909
Which is: 0.00053216327674743909
(Relative error 1.7674668425285303e-09 exceeds tolerance 9.9999999999999998e-13)

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:449: Failure
Value of: dist_off[0]
  Actual: 0.00053216327768802003
Expected: 0.00053216327674743909
Which is: 0.00053216327674743909
(Relative error 1.7674668425285303e-09 exceeds tolerance 9.9999999999999998e-13)

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:573: Failure
Value of: dist_on[0]
  Actual: 0.00195043766381975
Expected: 0.0019504376639951655
Which is: 0.0019504376639951655
(Relative error -8.9936457836800689e-11 exceeds tolerance 9.9999999999999998e-13)

/global/cfs/cdirs/atlas/esseivaj/devel/celeritas/test/orange/surf/detail/InvoluteSolver.test.cc:590: Failure
Value of: dist_off[0]
  Actual: 0.00195043766381975
Expected: 0.0019504376639951655
Which is: 0.0019504376639951655
(Relative error -8.9936457836800689e-11 exceeds tolerance 9.9999999999999998e-13)

I have to significantly reduce the precision for the test to pass. Maybe we can refactor the computation so that we don't lose that much precision?

@esseivaju esseivaju added bug Something isn't working minor Minor internal changes or fixes labels Sep 30, 2024
@esseivaju esseivaju requested a review from sethrj September 30, 2024 18:52
@@ -44,7 +44,7 @@ struct SoftEqualTraits<double>
{
using value_type = double;
static CELER_CONSTEXPR_FUNCTION value_type sqrt_prec() { return 1.0e-6; }
static CELER_CONSTEXPR_FUNCTION value_type rel_prec() { return 1.0e-12; }
static CELER_CONSTEXPR_FUNCTION value_type rel_prec() { return 1.0e-8; }
Copy link
Member

Choose a reason for hiding this comment

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

Oh dear this is far too wide ranging a change. We need to drill down to this one test. It looks like the failing tests may be in a part of the solver that's numerically unstable or suffers from catastrophic rounding or something, because it's producing very different answers for single precision. I saw something similar in the nuclear form factor PR: 664ac38 .

The best solution here might just be to change EXPECT_SOFT_EQ to EXPECT_SOFT_NEAR to add the third tolerance option (and make that one 1e-8).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the tolerance would be too wide. For now, let's use EXPECT_SOFT_NEAR.

Copy link

github-actions bot commented Sep 30, 2024

Test summary

 3 249 files   5 047 suites   3m 42s ⏱️
 1 520 tests  1 493 ✅ 27 💤 0 ❌
16 838 runs  16 776 ✅ 62 💤 0 ❌

Results for commit c8e7404.

♻️ This comment has been updated with latest results.

@sethrj sethrj merged commit 09e5eae into celeritas-project:develop Oct 1, 2024
34 checks passed
@sethrj sethrj changed the title Loosen double equivalence precision requirement Loosen numerically iffy test tolerance Oct 1, 2024
@sethrj sethrj added documentation Documentation, examples, tests, and CI and removed bug Something isn't working labels Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation, examples, tests, and CI minor Minor internal changes or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants