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

572 check for singularity when the solver parameters flag is turned on #603

Conversation

K20shores
Copy link
Collaborator

@K20shores K20shores commented Jul 15, 2024

Closes #572

This PR fixes an unknown bug where a zero in the bottom right position of the U matrix (which leads to a singular matrix) was not detected by always checking for zero in the bottom right element. Additionally, two tests are added to check for singular matrices when running rosenbrock

I learned that to get a singular U matrix, at least for these simple 2 and 3 species reactions, we needed a negative reaction rate, which I think is not physically realistic. This, in addition to the knowledge that chemical simulations have been running for decades without a singularity check, seem to indicate that chemical systems are not structured in a way that would lead to singular systems.

Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

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

Thanks @K20shores for addressing this issue. I guess the CUDA version of LU decomposition (https://github.com/NCAR/micm/blob/main/src/solver/lu_decomposition.cu#L96-L102) needs to be revised accordingly as well? And update the test_cuda_rosenbrock.cpp?

include/micm/solver/lu_decomposition.inl Show resolved Hide resolved
include/micm/solver/lu_decomposition.hpp Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.47%. Comparing base (6a60a49) to head (03b15ef).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   93.17%   93.47%   +0.30%     
==========================================
  Files          49       49              
  Lines        3500     3494       -6     
==========================================
+ Hits         3261     3266       +5     
+ Misses        239      228      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@sjsprecious sjsprecious left a comment

Choose a reason for hiding this comment

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

Thanks @K20shores for working on this and figuring out this bug. I could confirm that all the tests passed on Derecho's A100 GPU.

I also talked with Allison today, and she told me that we could construct a singular matrix with linear dependence between two rows or two columns (which is the definition of "singularity"). Not sure whether this is easy to do but this may avoid the usage of negative reaction rates.

Copy link
Collaborator

@mattldawson mattldawson left a comment

Choose a reason for hiding this comment

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

looks great!

Copy link
Collaborator

@boulderdaze boulderdaze left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple questions

include/micm/solver/lu_decomposition.inl Outdated Show resolved Hide resolved
include/micm/solver/rosenbrock.inl Show resolved Hide resolved
Copy link
Collaborator

@mwaxmonsky mwaxmonsky left a comment

Choose a reason for hiding this comment

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

Looks great @K20shores!

@K20shores K20shores merged commit f8fc288 into main Jul 17, 2024
29 checks passed
@K20shores K20shores deleted the 572-check-for-singularity-when-the-solver-parameters-flag-is-turned-on branch July 17, 2024 15:00
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.

Check for singularity when the solver parameters flag is turned on
6 participants