-
Notifications
You must be signed in to change notification settings - Fork 9
Introduce global test tolerance #382
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
base: main
Are you sure you want to change the base?
Introduce global test tolerance #382
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes tolerance settings for result comparisons by introducing global defaults and removes individual tolerance parameters from all tests.
- Introduce
RELATIVE_TOLERANCE
andABSOLUTE_TOLERANCE
constants inconftest_result_comparison.py
. - Update
assert_results_equal
andcompare_vtk_files
to use these defaults. - Remove hard-coded
rtol
/atol
arguments and theASSERT_RESULTS_EQUAL_TOL
constant from all test files.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/conftest_result_comparison.py | Added global tolerance constants and updated fixtures |
tests/test_mesh_creation_functions.py | Removed explicit rtol arguments from assertions |
tests/test_four_c_simulation.py | Ditto |
tests/test_create_cubit_input.py | Removed local tolerance constant and explicit args |
tests/test_cosserat_curve.py | Removed explicit rtol /atol from assertions |
tests/reference-files/test_four_c_beam_to_solid.4C.yaml | Reformatted beam triads floats to full precision |
Comments suppressed due to low confidence (3)
tests/conftest_result_comparison.py:41
- [nitpick] Add a module-level docstring or comment explaining that
RELATIVE_TOLERANCE
andABSOLUTE_TOLERANCE
are the defaults used byassert_results_equal
so contributors understand why they exist and how changing them affects all tests.
RELATIVE_TOLERANCE = 1e-13
tests/reference-files/test_four_c_beam_to_solid.4C.yaml:134
- [nitpick] Consider standardizing float formatting for readability (e.g., using
0.0
instead of-0.0
and rounding to a consistent number of decimal places) to keep reference files concise.
- 9 BEAM3R HERM2LINE3 28 30 29 MAT 1 TRIADS -0.0 -1.5707963267948968 -0.0 -0.0 -1.5707963267948968 -0.0 -0.0 -1.5707963267948968 -0.0
tests/conftest_result_comparison.py:40
- Add a note or link here (e.g., in the project’s CONTRIBUTING or changelog) referencing issue Possibly rethink / discuss handling of tolerance in new testing framework #192 so that the rationale for introducing global tolerances is recorded for future maintainers.
# GLOBAL DEFAULT TEST TOLERANCES
@isteinbrecher very interesting! The pipeline fails on macOS (especially ARM based OSs). Should we increase the tolerance for this one test or the global test tolerance? I am curious on your opinion:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that file makes the issues (I created it on macOS). I had a quick look and could not figure out what is going wrong, where the tolerances can be increased - I suspect different versions of the scipy interpolate functionality, but I am not sure. Anyway, this should not concern us, let's simply put both tolerances for this test to 1e-12
, then everything should pass. I would then also not adapt this result file.
- 9 BEAM3R HERM2LINE3 28 30 29 MAT 1 TRIADS 0 -1.57079632679 0 0 -1.57079632679 0 0 -1.57079632679 0 | ||
- 10 BEAM3R HERM2LINE3 31 33 32 MAT 1 TRIADS 0 -1.57079632679 0 0 -1.57079632679 0 0 -1.57079632679 0 | ||
- 11 BEAM3R HERM2LINE3 33 35 34 MAT 1 TRIADS 0 -1.57079632679 0 0 -1.57079632679 0 0 -1.57079632679 0 | ||
- 9 BEAM3R HERM2LINE3 28 30 29 MAT 1 TRIADS -0.0 -1.5707963267948968 -0.0 -0.0 -1.5707963267948968 -0.0 -0.0 -1.5707963267948968 -0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I marked this file as ported to double precision in #356
Closes #192
This PR introduces a global testing tolerance and removes all hard coded tolerances from all tests (which use the assert_results_equal functionality)
In another PR the array comparison should be also centralized, but this is #184
I needed to update 2 reference files so everything works locally, let's see how the pipelines are doing