armi
armi copied to clipboard
terrapower
Force Parameters to have descriptions
When constructing a Parameter object, we already have multiple checks to ensure the Parameter is valid:
https://github.com/terrapower/armi/blob/59388d29f8929178e0e1aa4d8f0f160095a2b739/armi/reactor/parameters/parameterDefinitions.py#L250-L253
Why not add some to ensure there is a valid description field? I want to say the same for units, but many parameters are unitless, so we would have to change "unitless" to be a non-empty string.
assert description is not None and len(description)
After all, what good is a
Parameterif no one can tell what it is?
@keckler FYI
@sombrereau @drewj-usnctech Would this be a problem for either of you?
I like the idea of enforcing that these are set. Could be a breaking change, but probably a good one? Maybe we can start implementing warnings to fix before we enforce so that folks have a chance to change? I also like the idea of enforcing locations would be good too.
On board for this. Parameters should have descriptions. I think setting units=None should still be supported as there are some cases where parameters really are unitless. Like number of things? It would be a small sample because the vast majority of things have units.
Maybe a non-None field like units=UNITLESS? None seems simpler tbh
I am changing this ticket to only FORCE Parameter descriptions, not units.
And I already have a DeprecationWarning in place:
https://github.com/terrapower/armi/blob/59388d29f8929178e0e1aa4d8f0f160095a2b739/armi/reactor/parameters/parameterDefinitions.py#L255-L259
The plan is:
- This version of ARMI will be released with the
DeprecationWarning. - The very next version of ARMI will change that to a hard
Error. - Then I will close this ticket.
Estimated time to close this ticket: 2 months (of waiting).
I have put this on the ARMI 0.4.0 Roadmap.
Really, the change in ARMI is no work at all. The time here will be spent on testing downstream projects to see if they will break.