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

Number density bug fix #804

Merged
merged 8 commits into from
Aug 3, 2022
Merged

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Aug 1, 2022

Description

PR #657 introduced a bug into how number densities are initialized for a component.

The bug:

  • when detailedAxialExpansion: True, number densities were being initialized with input temperatures.

This is incorrect behavior as number densities should be initialized with hot temperatures to account for radial expansion of components. Otherwise, number densities are never adjusted for radial expansion and masses are incorrect.

This PR fixes this by initializing the number densities with hot temperatures. Accounting for axial expansion via inputHeightsConsideredHot is then accounted for in adjustNDensForHotHeight.

  • In addition to this fix, I've changed method names and improved docstrings in the interest clarity.
  • Also cleaned up the code within adjustNDensForHotHeight to be simpler and more efficient (instead of completely recalculating the number densities previously calculated in setNDensFromMassFracsAtTempInC, it now just scales them accordingly.

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 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.

- The number densities in `applyMaterialMassFracsToNumberDensities` should be evaluated at hot temperatures to account for radial expansion of the components.
- instead of completely recalculating the number densities calculated by setNDensFromMassFracsAtTempInC, this now just scales the number densities accordingly. is an efficiency and clarity improvement
@albeanth albeanth added the bug Something is wrong: Highest Priority label Aug 1, 2022
@albeanth albeanth requested a review from onufer August 1, 2022 23:08
@albeanth
Copy link
Member Author

albeanth commented Aug 1, 2022

@mgjarrett

armi/reactor/blueprints/blockBlueprint.py Show resolved Hide resolved
armi/reactor/tests/test_components.py Outdated Show resolved Hide resolved
@jakehader jakehader removed the request for review from onufer August 3, 2022 21:58
@jakehader jakehader merged commit 22cadd2 into terrapower:main Aug 3, 2022
@albeanth albeanth deleted the numberDensityBugFix branch August 3, 2022 22:40
@john-science john-science mentioned this pull request Aug 5, 2022
7 tasks
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
PR terrapower#657 introduced a bug into how number densities are initialized for a component.

The bug:

When `detailedAxialExpansion: True`, number densities were being initialized with input temperatures.
This is incorrect behavior as number densities should be initialized with hot temperatures to account for radial expansion of components. Otherwise, number densities are never adjusted for radial expansion and masses are incorrect.

This PR fixes this by initializing the number densities with hot temperatures. Accounting for axial expansion via inputHeightsConsideredHot is then accounted for in adjustNDensForHotHeight.

In addition to this fix, I've changed method names and improved docstrings in the interest clarity.
Also cleaned up the code within `adjustNDensForHotHeight` to be simpler and more efficient (instead of completely recalculating the number densities previously calculated in `setNDensFromMassFracsAtTempInC`, it now just scales them accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants