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

Force Parameters to have descriptions #1348

Open
john-science opened this issue Jul 17, 2023 · 5 comments
Open

Force Parameters to have descriptions #1348

john-science opened this issue Jul 17, 2023 · 5 comments
Assignees
Labels
architecture Issues related to big picture system architecture documentation Improvements or additions to documentation enhancement New feature or request

Comments

@john-science
Copy link
Member

john-science commented Jul 17, 2023

When constructing a Parameter object, we already have multiple checks to ensure the Parameter is valid:

assert self._validName.match(name), "{} is not a valid param name".format(name)
# nonsensical to have a serializer with no intention of saving to DB
assert not (serializer is not None and not saveToDB)
assert serializer is None or saveToDB

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 Parameter if no one can tell what it is?

@john-science john-science added the enhancement New feature or request label Jul 17, 2023
@john-science john-science self-assigned this Jul 17, 2023
@john-science
Copy link
Member Author

@keckler FYI

@sombrereau @drewj-usnctech Would this be a problem for either of you?

@jakehader
Copy link
Member

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.

@drewj-usnctech
Copy link
Contributor

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

@john-science john-science changed the title Force Parameters to have units and descriptions Force Parameters to have descriptions Aug 16, 2023
@john-science
Copy link
Member Author

I am changing this ticket to only FORCE Parameter descriptions, not units.

And I already have a DeprecationWarning in place:

if not len(description):
runLog.warning(
f"DeprecationWarning: Parameter {name} defined without description.",
single=True,
)

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

@john-science john-science added the good first issue Good for newcomers label Oct 3, 2023
@albeanth albeanth mentioned this issue Oct 11, 2023
7 tasks
@john-science
Copy link
Member Author

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.

@john-science john-science added documentation Improvements or additions to documentation architecture Issues related to big picture system architecture and removed good first issue Good for newcomers labels Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Issues related to big picture system architecture documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants