-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add method for setting input parameter unit for purposes of documentation #27352
Conversation
@bwspenc Here's a candidate implementation for you to try. This PR still needs finalization with respect to API and output metadata naming, as well as tests and some documentation, but I wanted to get it in your hands to try out. An example of usage for
which then results in I am considering changing the metadata label to "Assumed Units", but would welcome feedback. |
If we do this, I think that we also need to use the MooseUnit system somehow to also keep track of this unit so that it can properly be converted to other units if the primary units are declared in another way. |
I have gone back and forth on this, and @dschwen and I spoke about it previously. He can chime in with thoughts now that MooseUnit is in the framework. I am open to a re-write to integrate this into MooseUnit, but then that would open up the door to propagating units throughout the framework, which we've always stepped up to the line for but never quite committed to. |
Making sure that otherwise declared units are consistent (for error-checking if nothing else) would be a good thing here, I think, at minimum. |
I agree. I just want to make sure that we design it s.t. it has the unit integration in mind for the future, hopefully not backing us out |
Job Precheck on 053a050 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
053a050
to
6afa1c7
Compare
Job Documentation on 6afa1c7 wanted to post the following: View the site here This comment will be updated on new commits. |
Job Coverage on 6afa1c7 wanted to post the following: Framework coverage
Modules coverageCoverage did not change Full coverage reportsReports
Warnings
This comment will be updated on new commits. |
Job Documentation, step Docs: sync website on 3d98b17 wanted to post the following: View the site here This comment will be updated on new commits. |
Job Coverage, step Generate coverage on 3d98b17 wanted to post the following: Framework coverage
Modules coverageCoverage did not change Full coverage reportsReports
Warnings
This comment will be updated on new commits. |
Our internal discussions on this current design suggest there is a lot more to be done here in order to integrate this system into that of Tagging @bwspenc for a review. |
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.
Looks good to me. I made a couple of comments on really minor stuff that you can leave alone if you prefer.
Reason
This change is intended to address corrective action CA 2021-1199 for BlackBear/Grizzly (and other MOOSE-based application to which a similar CA applies) and provide a new capability to framework and application developers who with to provide more context regarding intended units for a given parameter. This is not intended to provide automatic unit conversion for input parameters (but could likely be used as the API to tweak further in the future).
Design
Create
setDocoUnit
andgetDocoUnit
methods for setting a unit in the object source code and getting it for use in other code or, in this case, in the JSON output for use in MooseDocs. MooseDocs infrastructure to display this new parameter metadata should also be constructed in accordance with the other parameter metadata already presented.Impact
New documentation for intended units in calculations will improve software quality. This is not a simulation enhancement but a user/developer quality-of-life one.