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

Change standard name and unit of CCPP error flag variable in all metadata files #828

Merged
merged 2 commits into from
Jan 26, 2022

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Jan 16, 2022

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

[ errflg ]
  standard_name = ccpp_error_flag
  long_name = error flag for error handling in CCPP
  units = flag
  dimensions = ()
  type = integer
  intent = out

New

[ errflg ]
  standard_name = ccpp_error_code
  long_name = error code for error handling in CCPP
  units = 1
  dimensions = ()
  type = integer
  intent = out

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

@climbfuji
Copy link
Collaborator Author

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

@SamuelTrahanNOAA
Copy link
Collaborator

A unit of "1" is far less meaningful than a unit of "flag." Maybe the new code generator should be changed instead?

Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA left a 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?

@grantfirl
Copy link
Collaborator

grantfirl commented Jan 18, 2022

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

@grantfirl
Copy link
Collaborator

@climbfuji Since this won't affect UFS RTs, do you want to combine this for testing/merging with #786?

@climbfuji
Copy link
Collaborator Author

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

@climbfuji
Copy link
Collaborator Author

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

Agree!

@grantfirl
Copy link
Collaborator

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

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?

@SamuelTrahanNOAA
Copy link
Collaborator

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

Agree!

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.

@climbfuji
Copy link
Collaborator Author

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

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?

No hurry at all. Whenever it fits!

@grantfirl
Copy link
Collaborator

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.

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.

@SamuelTrahanNOAA
Copy link
Collaborator

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.

Copy link
Collaborator

@SamuelTrahanNOAA SamuelTrahanNOAA left a 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.

@climbfuji climbfuji merged commit 01f55a2 into NCAR:main Jan 26, 2022
@climbfuji climbfuji deleted the ccpp_error_code_in_prebuild branch June 27, 2022 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants