-
Notifications
You must be signed in to change notification settings - Fork 5
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
Backware Euler with vectorizable matrix types #596
Conversation
…senbrock-on-the-analytical-policy-tests' of https://github.com/NCAR/micm into 577-test-all-parameter-types-of-the-dense-matrix-cpu-rosenbrock-on-the-analytical-policy-tests
…senbrock-on-the-analytical-policy-tests' of https://github.com/NCAR/micm into 577-test-all-parameter-types-of-the-dense-matrix-cpu-rosenbrock-on-the-analytical-policy-tests
…senbrock-on-the-analytical-policy-tests' of github.com:NCAR/micm into 577-test-all-parameter-types-of-the-dense-matrix-cpu-rosenbrock-on-the-analytical-policy-tests
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! Only a few questions, but nothing that would hold this up
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.
Thanks @mattldawson for working on this giant PR and this functionality looks great. Just have one question as listed below.
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.
Looks good! I don't see anything other than what others have commented on. I left one comment but it shouldn't hold this PR getting merged.
Running on Derecho with NVHPC, I got the following failed tests:
Looking at the output log, the difference is slightly above the given tolerance. I wonder whether we can add a check to the relative error (like https://github.com/NCAR/micm/blob/main/test/unit/solver/test_rosenbrock.cpp#L138-L139) and pass the test if the relative error is small enough (say < 1.e-14?). |
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.
Looks great @mattldawson! Just one quick recommendation but nothing worth holding things up.
EXPECT_EQ(matrix[0][1][1], 8); | ||
EXPECT_EQ(matrix[1][1][1], 10); | ||
EXPECT_EQ(matrix[2][1][1], 12); | ||
|
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.
We might want to add some checks on the non-diagonal to make sure we are only incrementing the expected indices.
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.
added!
I modified the analytical policy tests to include a relative error check and the two CPU tests pass now. Still waiting on the CUDA analytical test to run. |
The CUDA tests now pass with some slightly adjusted tolerances. |
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.
Thanks @mattldawson for working on this and addressing all the comments. I could confirm that all the tests passed on Derecho's A100 GPU.
Adapts the Backward Euler solver to work with vectorizable matrices.
closes #500
Also: