-
-
Notifications
You must be signed in to change notification settings - Fork 487
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 tolerance in a few expm()
tests
#38685
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change 1r-14 to 5e-14 in the following two examples. This is likely required because expm() in scipy was rewritten in C. Example 1: File "src/sage/matrix/matrix_double_dense.pyx", line 3666, in sage.matrix.matrix_double_dense.Matrix_double_dense.exp Failed example: A.exp() # tol 1e-14 Expected: [51.968956198705044 74.73656456700327] [112.10484685050491 164.07380304920997] Got: [51.968956198707595 74.736564567007] [112.10484685051048 164.07380304921807] Tolerance exceeded in 4 of 4: 51.968956198705044 vs 51.968956198707595, tolerance 5e-14 > 1e-14 74.73656456700327 vs 74.736564567007, tolerance 5e-14 > 1e-14 112.10484685050491 vs 112.10484685051048, tolerance 5e-14 > 1e-14 164.07380304920997 vs 164.07380304921807, tolerance 5e-14 > 1e-14 Example 2: File "src/sage/matrix/matrix_double_dense.pyx", line 3679, in sage.matrix.matrix_double_dense.Matrix_double_dense.exp Failed example: A.exp() # tol 1e-14 Expected: [51.968956198705044 74.73656456700327] [112.10484685050491 164.07380304920997] Got: [51.968956198707595 74.736564567007] [112.10484685051048 164.07380304921807] Tolerance exceeded in 4 of 4: 51.968956198705044 vs 51.968956198707595, tolerance 5e-14 > 1e-14 74.73656456700327 vs 74.736564567007, tolerance 5e-14 > 1e-14 112.10484685050491 vs 112.10484685051048, tolerance 5e-14 > 1e-14 164.07380304920997 vs 164.07380304921807, tolerance 5e-14 > 1e-14
Change 1e-14 to 6e-14 in the following example. This is likely necessary because expm() in scipy was rewritten in C. File "src/sage/matrix/matrix2.pyx", line 15903, in sage.matrix.matrix2.Matrix.exp Failed example: a.change_ring(RDF).exp() # rel tol 1e-14 Expected: [42748127.31532951 7368259.244159399] [234538976.1381042 40426191.45156228] Got: [ 42748127.31533201 7368259.244159831] [234538976.13811788 40426191.45156465] Tolerance exceeded in 4 of 4: 42748127.31532951 vs 42748127.31533201, tolerance 6e-14 > 1e-14 7368259.244159399 vs 7368259.244159831, tolerance 6e-14 > 1e-14 234538976.1381042 vs 234538976.13811788, tolerance 6e-14 > 1e-14 40426191.45156228 vs 40426191.45156465, tolerance 6e-14 > 1e-14
In the following example, we now use 5e-14 instead of 1e-14. This is likely due to scipy's expm() being rewritten in C: File "src/sage/symbolic/constants_c_impl.pxi", line 162, in sage.symbolic.constants_c_impl.pxi.E.__pow__ Failed example: e^A # rel tol 1e-14 Expected: [51.968956198705044 74.73656456700327] [112.10484685050491 164.07380304920997] Got: [51.968956198707595 74.736564567007] [112.10484685051048 164.07380304921807] Tolerance exceeded in 4 of 4: 51.968956198705044 vs 51.968956198707595, tolerance 5e-14 > 1e-14 74.73656456700327 vs 74.736564567007, tolerance 5e-14 > 1e-14 112.10484685050491 vs 112.10484685051048, tolerance 5e-14 > 1e-14 164.07380304920997 vs 164.07380304921807, tolerance 5e-14 > 1e-14
Documentation preview for this PR (built with commit a727c17; changes) is ready! 🎉 |
kwankyu
approved these changes
Sep 26, 2024
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.
OK. reasonable.
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Sep 27, 2024
SciPy upstream just merged scipy/scipy#21553 which rewrites the matrix exponential in C. I merged this PR to work around some other bug, and in the process noticed that a few tolerances will need to be loosened in our test suite. URL: sagemath#38685 Reported by: Michael Orlitzky Reviewer(s): Kwankyu Lee
thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SciPy upstream just merged scipy/scipy#21553 which rewrites the matrix exponential in C. I merged this PR to work around some other bug, and in the process noticed that a few tolerances will need to be loosened in our test suite.