-
Notifications
You must be signed in to change notification settings - Fork 90
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
Updates for pin-related functionality #1990
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 76e36f3.
…u frac comp params
@zachmprince a heads up |
@albeanth This PR needs several release notes:
|
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.
Changes seem to be in order. Requesting release notes according to @john-science comment - #1990 (comment)
Some softer, non-blocking changes that I'd consider good to haves but not strictly needed at this time.
armi/reactor/components/component.py
Outdated
# Return pin fluxes | ||
return self.parent.p.get(param)[indexMap] |
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.
If the parameter param
is not defined on the parent, or is None
, there could be a not great exception raised here. I would recommend some error helping, maybe a try/except/raise pattern like
try:
return self.parent.p.get(param)[indexMap]
except Exception as ee:
msg = f"Failure getting {param} from {self} via parent {self.parent}"
runLog.error(msg)
runLog.error(ee)
raise ValueError(msg) from ee
or just allowing the raised exception to pass up, replacing the raise ValueError(msg) from ee
with raise
@@ -120,6 +124,8 @@ class HistoryTrackerInterface(interfaces.Interface): | |||
|
|||
name = "history" | |||
|
|||
DETAILED_ASSEMBLY_FLAGS = [Flags.FUEL, Flags.CONTROL] |
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.
Did we check with the users of this to ensure that this doesn't need to be configurable? Specifically for pin performance evaluations? @sammiller11235?
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.
Yes. We were told that, for now, we are only concerned with fuel and control assemblies.
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.
prior to this we only got fuel.
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.
Changes from @zachmprince look good. Just need release notes and I feel okay with this PR
What is the change?
These changes are to support upgrades in pin-related functionality. The changes in this PR can be lumped into the following buckets:
blocks.py::Block::breakFuelComponentsIntoIndividuals
. This is removed because it is a very memory expensive way of storing pin level characteristics. Storing data as numpy arrays is much more manageable.getPuMoles
from blocks.py up to composites.pymolesHmBOL
andpuFrac
on components duringcompleteInitialLoading
.getPinMgFluxes
to retrieve the pin-wise multigroup fluxes from aBlock
.Why is the change being made?
To support downstream pin-related functionality upgrades.
close #791
Checklist
This PR has only one purpose or idea.Sorry 😞doc
folder.pyproject.toml
.