-
Notifications
You must be signed in to change notification settings - Fork 92
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
Filling in empty Parameter units and descriptions #1345
Conversation
Code coverage went down because I mostly deleted code. That's fine. |
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.
Some various comments/musings.
I also want to highlight that the Torus
component shape has been removed entirely by this PR. From my read of the PR description, I was expecting that just some parameters of the Torus
were removed. I don't have a complaint about removing Torus
, because it appears to be possible to do the same things via RadialSegment
.
@@ -723,23 +724,23 @@ def _getNeutronicsCoreParams(): | |||
with pDefs.createBuilder(categories=[parameters.Category.neutronics]) as pb: | |||
pb.defParam( | |||
"eigenvalues", | |||
units=None, | |||
units=units.UNITLESS, | |||
description="All available lambda-eigenvalues of reactor.", |
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.
Unrelated to your changes, but this parameter can almost certainly be removed.
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.
That will require a ticket though, because the name of this parameter is so generic that it makes it hard to grep through downstream repos to find it.
It might be easier to just remove the Parameter, and see if anything breaks downstream.
Co-authored-by: Chris Keckler <ckeckler@terrapower.com>
Co-authored-by: Chris Keckler <ckeckler@terrapower.com>
Co-authored-by: Michael Jarrett <mjarrett@terrapower.com>
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.
A handful of small suggestions, but otherwise looks good.
Thanks for your work on this, it's an obvious improvement to the codebase!
Co-authored-by: Chris Keckler <ckeckler@terrapower.com>
Thanks, Chris! The original goal of this PR was just to fix missing/wrong units/descriptions. I see what you're doing with the units there, and I think at some point I will either do that with ALL the units, or we will use But this PR is already so hefty, I'll leave that for the next one (and #352). |
What is the change?
We are finally filling in all the missing values in the
units
anddescription
fields of our ARMIParameter
s.I have also completely removed 16
Parameter
s, because they are unused:NOTE: I should note all the above "torus" parameters were removed, and in the end we removed the entire Torus geometry. It was entirely unused, and in the process of reviewing these Parameters I found a few bugs in our Torus geometry.
NOTE: As part of #1348, we are also adding a
DeprecationWarning
that at some pointParameter
s will demand non-empty descriptions.Why is the change being made?
What is the good of having "units" and "description" fields on important and heavily-used ARMI Parameters if they are left blank?
These should have been filled in years ago.
Checklist
doc/release/0.X.rst
) are up-to-date with any important changes.doc
folder.setup.py
.