-
Notifications
You must be signed in to change notification settings - Fork 147
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
Change standard name and unit of CCPP error flag variable in all metadata files #828
Conversation
@junwang-noaa @yangfanglin @ligiabernardet @mkavulich Please see the suggested change of the metadata for the CCPP error flag/code variable. If this PR is approved as is, we need to update the CCPP technical documentation and should consider sending out an email to the physics developers. I can provide a draft at that time, if needed. |
A unit of "1" is far less meaningful than a unit of "flag." Maybe the new code generator should be changed instead? |
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 unit of "1" is far less meaningful than a unit of "flag." Maybe the new code generator should be changed instead?
The "flag" unit is reserved for logical. This is a change to be consistent with our own set of rules that we've agreed upon (https://github.com/ESCOMP/CCPPStandardNames/blob/main/StandardNamesRules.rst at the bottom). Plus, in the future, although this variable is used in the binary sense, there is no reason why it must remain so in the future (to accommodate more thorough error reporting/handling). |
@climbfuji Since this won't affect UFS RTs, do you want to combine this for testing/merging with #786? |
I'd prefer to keep them separate, because this PR is already modifying 115 metadata files. If that's ok with you. |
Agree! |
That's fine. Is it OK with you to schedule this behind #786 and #820 that have already been scheduled with UFS code managers or is it higher priority than these? |
I had forgotten that the "error flag" variable errflg is not in fact a flag, but an integer error code. Indeed, the change in the "unit=" lines make sense. It is the original variable name that is wrong. If that is going to be corrected, it belongs in another PR. |
No hurry at all. Whenever it fits! |
Sam, I don't understand why you would want to split these metadata changes into 2 different pull requests. All of it is related to correcting past metadata errors and preparing for the transition to the new CCPP code generator. Splitting this seems like unnecessary work to me. |
I misunderstood the change. This highly questionable change (using "1" as the units for an integer flag) is required by established rules. Dom's changes are required, whether they're beneficial or not. I had suggested changing "errflg" in the code to actually be a logical, but apparently the errflg may one day be treated as an error code, with more than two possible values. Hence, the units of "1" make even less sense. Regardless, the established rules require units of "1" so the PR is acceptable as is. |
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.
I misunderstood the change. This highly questionable change (using "1" as the units for an integer flag) is required by established rules. Dom's changes are required, whether they're beneficial or not.
I had suggested changing "errflg" in the code to actually be a logical, but apparently the errflg (error flag) may one day be treated as an error code, with more than two possible values. Hence, the units of "1" make even less sense. Regardless, the established rules require units of "1" so the PR is acceptable as is.
I reluctantly approve.
…_error_code_in_prebuild
Description
Update standard name and unit of CCPP error flag variable in all metadata files in preparation for the new CCPP framework code generator:
Old
New
Testing
See ufs-community/ufs-weather-model#1013
Associated PRs:
NCAR/ccpp-framework#428
#828
NOAA-EMC/fv3atm#467
ufs-community/ufs-weather-model#1013