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

Change matrix format to CSC in depletion #2764

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

eepeterson
Copy link
Contributor

@eepeterson eepeterson commented Nov 9, 2023

Description

This PR is related to #2703, but is just looking at changing the matrix format (no change to the solver method) since in my test case this reduces the time spent in CRAM by more than 25% (for IndependentOperator calculations). The majority of the speed up is actually just from setting the matrix format of ident = sp.eye(A.shape[0], format='csc') to be compatible with A prior to looping over the residues and also works for CSR format. The motivation to change to CSC is to avoid SparseEfficiencyWarnings and repeated conversions from CSR to CSC once we introduced explicit LU factorization as outlined in #2703. My idea here was to work through restructuring any necessary regression tests here to make sure we aren't making depletion less accurate. Interestingly enough the CSC format change results in fewer negative density warnings as shown in the files attached (it's not the addition of the factorization like I originally thought)

One test case didn't pass locally, although I did confirm that the resulting atom numbers coming out of CRAM agree to within a relative tolerance of 1e-14 between this patch and develop. I believe the error is resulting from the calculated reaction rates in the transport run done when final_step=True.

Below are the profile outputs for this problem:

no_patch.txt
csc_patch.txt

Update after merging #2771 and #2779: This PR offers negligible speed up (but at least no slow down) while reducing the number of negative density warnings, plus is more efficient for future work to be carried out in #2703 (see updated outputs below).

with_no_csc_patch.txt
with_csc_patch.txt

Checklist

  • I have performed a self-review of my own code
    - [ ] I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
    - [ ] I have added tests that prove my fix is effective or that my feature works (if applicable)

Copy link
Contributor

@drewejohnson drewejohnson left a comment

Choose a reason for hiding this comment

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

Thanks for breaking this out.

If the performance improvement is really due to having csc layout everywhere, then this feels better. But if we can get similar speedup by ensuring that all the matrices used are the same layout, even existing csr, and we don't get the same changes to the regression suite, that is preferable to me.

I know the change to csc also influences the LU factorization from #2703, so that's another thing to contend with. And I know there are potential some CI / local machine differences that are not easy to solve. And we have a whole bevy of other tests that pass without updates. I'm being kind of a wet blanket to try to understand sources of benefit and potential problems.

tests/regression_tests/deplete_with_transfer_rates/test.py Outdated Show resolved Hide resolved
openmc/deplete/cram.py Outdated Show resolved Hide resolved
one more change to CSC
@paulromano paulromano merged commit 8b2698f into openmc-dev:develop Nov 28, 2023
16 checks passed
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
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.

3 participants