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

Fix burnup-dependent reaction rates #911

Merged
merged 15 commits into from
Oct 1, 2022
Merged

Conversation

keckler
Copy link
Member

@keckler keckler commented Sep 29, 2022

Description

The primary purpose of this commit is to fix the getReactionRates() methods
on the Block and Component classes to account for burnup-dependent cross-sections, as was brought up in #813 .

This is completed primarily by changing the underlying getReactionRateDict()
method to accept the entire library suffix instead of just the first letter of
the suffix (i.e. "AA" or "AB" instead of just "A").

In the process, it was seen that a refactor could be achieved where the two classes could share the method of the parent instead of implementing their own, since they effectively do the same thing. This was accomplished by generalizing the calls in the method and moving that method into the Composite class.

Unit tests have also been updated.

I looked around, and do not believe that the change in the getReactionRateDict() signature should impact anybody. And even if it does, this is a necessary change because this was previously a bug.

In terms of impact of this bug-fix on physics results, I only know of two potential impacts, both on internal projects. I have already reached out to both of those projects individually.


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

keckler and others added 11 commits August 3, 2022 20:41
…refactor

The primary purpose of this commit is to fix the `getReactionRates()` methods
on the `Block` and `Component` classes to account for burnup-dependent cross-sections.
This is completed primarily by changing the underlying `getReactionRateDict()`
method to accept the entire library suffix instead of just the first letter of
the suffix (i.e. "AA" or "AB" instead of just "A").

In the process, it was seen that a refactor could be achieved where the two classes
could share the method of the parent instead of implementing their own, since they
effectively do the same thing. This was accomplished by generalizing the calls
in the method and moving that method into the `Composite` class.
@keckler keckler marked this pull request as ready for review September 30, 2022 16:34
@john-science john-science added the feature request Smaller user request label Sep 30, 2022
Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

As we discussed, I think getReactionRateDict() started in kind of a strange place before you got here. But that doesn't change the value of this PR.

Thanks!

@keckler
Copy link
Member Author

keckler commented Sep 30, 2022

Alright @john-science , I moved getReactionRateDict() into composites where it belongs.

@keckler keckler merged commit 6557412 into terrapower:main Oct 1, 2022
@keckler keckler deleted the reactionRates branch October 1, 2022 00:02
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants