Updated CCPP Standard Names #13
Conversation
modified: standard_names.xml modified: Metadata-standard-names.md modified: standard_names.xml
bluefinweiwei
changed the title
|
Not able to open standard_names.xml. Do we need to review this file too? |
|
@dudhia It is sufficient to review the .md file since it is created from the .xml file. |
|
I saw many duplications in there so I wondered if it was generated
correctly.
…On Fri, May 21, 2021 at 1:42 PM ligiabernardet ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Metadata-standard-names.md
<#13 (comment)>
:
> * `real(kind=kind_phys)`: units = kg kg-1
## standard_variables
Standard / required CCPP variables
* `ccpp_error_message`: Error message for error handling in CCPP
* `character(kind=len=512)`: units = 1
* `ccpp_error_flag`: Error flag for error handling in CCPP
* `integer(kind=)`: units = flag
+## non-category
@dudhia <https://github.com/dudhia> Yes, please review the
non-categorized names since they are an integral part of the dictionary of
standard names.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77FAZOSQ2OC5SFLEWG3TO2ZRPANCNFSM44IFYSVA>
.
|
|
I believe there are four full repeats of over 2000 lines after about 6000
lines
…On Fri, May 21, 2021 at 1:50 PM Jimy Dudhia ***@***.***> wrote:
I saw many duplications in there so I wondered if it was generated
correctly.
On Fri, May 21, 2021 at 1:42 PM ligiabernardet ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In Metadata-standard-names.md
> <#13 (comment)>
> :
>
> > * `real(kind=kind_phys)`: units = kg kg-1
> ## standard_variables
> Standard / required CCPP variables
> * `ccpp_error_message`: Error message for error handling in CCPP
> * `character(kind=len=512)`: units = 1
> * `ccpp_error_flag`: Error flag for error handling in CCPP
> * `integer(kind=)`: units = flag
> +## non-category
>
> @dudhia <https://github.com/dudhia> Yes, please review the
> non-categorized names since they are an integral part of the dictionary of
> standard names.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#13 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEIZ77FAZOSQ2OC5SFLEWG3TO2ZRPANCNFSM44IFYSVA>
> .
>
|
|
@dudhia and @cacraigucar We're pushing a commit sans-duplication in a bit. Thanks for IDing the 4X duplication @dudhia. This should make it (slightly) easier to review. |
remove duplication in UFS-based XML additions
|
It was to avoid references to things that needed units like 0 deg C or 20
deg C in response to earlier comments on the thread.
…On Mon, May 24, 2021 at 2:47 PM grantfirl ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Metadata-standard-names.md
<#13 (comment)>
:
> * `latitude`: Latitude
- * `real(kind=kind_phys)`: units = radians
+ * `real(kind=kind_phys)`: units = degree_north
degree_north and degree_east are the "canonical" units in CF for lat/lon.
I agree that radians makes more sense for scientific programming, but
either we stick with existing standards or we don't. We have a tab on our
spreadsheet to try to capture disagreements with CF standards, so we could
put this in there.
We actually have variables/std names for latitude/longitude_in_radians
below, although they should be superfluous with unit conversion. This one
might be a bit trickier than most because the conversion may need to put
bounds on the data, right? (latitude degree between -90-90, radian between
-pi/2,pi/2 ...)
@dudhia <https://github.com/dudhia>, I'm not understanding standard
temperature/freezing point's relation to lat/lon here. Was this comment
supposed to refer to other variables?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77BBEURANFB6BXWJDTDTPK3PBANCNFSM44IFYSVA>
.
|
|
Once @bluefinweiwei accepts/merges a PR into her branch, there is a bit more duplication removal and modifications to the XML/md to address several comments. I also added a simple script to report on duplicates (but not remove -- still need to finish that code). Note that the XML file in this PR does not pass the XML validation due to a bunch of capital letters (if not other things as well). We can fix that if necessary. |
|
@ligiabernardet Thanks for the confirmation. I'm working on updating this PR as discussed yesterday now. There will be a PR into @bluefinweiwei's fork to inspect at some point, whose changes will get transferred to this PR once approved/merged. As far as updating ccpp-physics and hosts with these further changes, do we want to do that right away? Pro: can maintain consistency with the CCPPStandardNames repo; Con: more PRs to have to test and potentially annoy reviewers. |
|
My recommendation is to wait until we reach agreement wrt this PR13 and later proceed to updating ccpp-physics and fv3atm. We do need to reduce the number of standard-name related updates to ccpp-physics and fv3atm to reduce the burden on reviewers. |
Another wrinkle is that NOAA models have "tracer_concentration" to refer to the same array as the current "constituent_mixing_ratio". These should probably be combined for better portability between hosts. CF doesn't mention "constituent" but does "tracer". As you mentioned, I don't think that we can just get rid of the array since hosts/physics will want to use something like this. "Array" in "constituent_array" is redundant since dimensions are included in the metadata. I see a couple of options:
1 is probably simpler in terms of coding, but makes the code function more ambiguous IMO (actual denominators in the variables are effectively hidden unless documented elsewhere). 2 is probably more work but is more explicit. It also has the benefit of making it obvious when a physics scheme expects one denominator and the host is providing something else. |
I have a strong preference for 2 as that is the only way we will ever be able to provide checking and (possibly automated) translation. We have had errors in simulations traced to a misunderstanding of whether a particular field was 'wet' or 'dry'. |
|
Yes, this was discussed earlier and the wrt qualifiers are needed and hence
the rule was made explicit about that (Grant was not here for that).
Regarding tracer versus constituent. I think of tracers as passive and
constituents as possibly chemical species or something that can change.
So in my thinking constituents would always have species names
associated with them.
…On Fri, Aug 6, 2021 at 11:17 AM goldy ***@***.***> wrote:
Use an umbrella term like "constituent_concentration" or "tracer_concentration" as a catch-all for whatever the host/physics is using.
Have several different standard names/variables for different denominators, e.g. "tracer_concentration_wrt_moist_air" + "tracer_concentration_wrt_dry_air" + "tracer_concentration_wrt_total_mass".
I have a strong preference for 2 as that is the only way we will ever be
able to provide checking and (possibly automated) translation. We have had
errors in simulations traced to a misunderstanding of whether a particular
field was 'wet' or 'dry'.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77B4IOL4V7BBGN2PAO3T3QKKZANCNFSM44IFYSVA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
I also agree with the connotations of tracer vs. constituent. The problem on the NOAA side is that one array is used for both what we would consider atmospheric constituents (which can be described as proper concentrations with common denominators) and passive tracers that are either advected or not by the dycore depending on flags (e.g. cloud fraction, TKE). One could argue about whether the non-constituent components belong in this array, but we cannot necessarily dictate this by considering what would make our CCPP standard names more self-consistent. I believe that for the "tracer_concentration" variable that NOAA models are using, the only "true" relationship amongst the components is that they are potentially advected by the dycore. @goldy For the constituent_mixing_ratio, I'm guessing that this isn't necessarily the case (and that advected variables are handled differently)? That is, we could not both agree to use something like "potentially_advected_species"? It still seems like there should be space to combine these into one for better portability, especially since >90% of the variables in NOAA's "tracer_concentration" array are likely actual atmospheric constituents. I don't know that we would be any worse off (in terms of code understandability) if we did the following:
Any better ideas? I can't finish updating this PR with respect to mixing ratios until this is decided. |
|
@grantfirl I appreciate the challenge here. I have not come up with a reasonable suggestion yet. This topic require discussion in person, perhaps on Tuesday's CCPP Framework meeting.It would be good to get an opinion from @climbfuji when he is back. The tracer concentration is already problematic as is, since it has units of kg kg-1 but some of the contents of the array are cloud fraction and TKE. However, calling it mixing_ratio_of and keeping the TKE and cloud fraction in it is hard to justify. |
… ratios to be specific about denominators, constituent_mixing_ratio names
Update flags and mixing ratios
As I said in #20, I am fine with this not being units=index since the values are floating point numbers. I think index should just be for integers. Does that mean that the few standard names with have "_real" attached to them should also have their units changed? See soil_type_classification_real for an example of one of these variables. |
|
Yes, I agree, these are really categories not indices and perhaps the unit
should be category.
The code is unusual in expecting the category to be real, but that is a
separate issue.
Index should be used only for integers that can be an index in an array.
…On Fri, Aug 20, 2021 at 4:29 PM cacraigucar ***@***.***> wrote:
Why units of none instead of index like the other control variables?
As I said in #20 <#20>, I
am fine with this not being units=index since the values are floating point
numbers. I think index should just be for integers.
Does that mean that the few standard names with have "_real" attached to
them should also have their units changed? See
soil_type_classification_real for an example of one of these variables.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77DC5WSLUMUVPUV3REDT53JNBANCNFSM44IFYSVA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
I like this statement, it should be in the rules (do we need a separate document for rules on units?). |
There was a problem hiding this comment.
Thank you @grantfirl for addressing several of the previous comments and concerns including:
- Come up with a better name (potentially_advected_quantities) for tracer_concentration since a) it contains cloud fraction and TKE and b) it can contain non-advected quantities.
- Improve flag names because "flag_for_PBL" is not descriptive enough.
- Change mixing ratios to say wrt what.
- Change volume mix ratio to units of mol mol-1.
- Change xyz_number_concentration to number_concentration_of_xyz.
|
If there is still a chance, the *classification_real unit can be changed to
something else like 'category' to comply with stricter rules for the use of
'index'.
This applies to lines 1507, 1585 and 1615 of the *.md file.
…On Mon, Aug 23, 2021 at 8:09 PM ligiabernardet ***@***.***> wrote:
***@***.**** approved this pull request.
Thank you @grantfirl <https://github.com/grantfirl> for addressing
several of the previous comments and concerns including:
- Come up with a better name (potentially_advected_quantities) for
tracer_concentration since a) it contains cloud fraction and TKE and b) it
can contain non-advected quantities.
- Improve flag names because "flag_for_PBL" is not descriptive enough.
- Change mixing ratios to say wrt what.
- Change volume mix ratio to units of mol mol-1.
- Change xyz_number_concentration to number_concentration_of_xyz.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77GXUQ345ARPUVQ3BILT6L5PBANCNFSM44IFYSVA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
20210825 update to reflect latest discussion RE: units
|
It looks like we are very close to merging this PR. @grantfirl @gold2718 can you please re-review and approve if satisfied? |
| <standard_name name="ccpp_error_flag" | ||
| long_name="Error flag for error handling in CCPP"> | ||
| <type kind="" units="flag">integer</type> | ||
| <standard_name name="ccpp_error_code" |
There was a problem hiding this comment.
@climbfuji In order to use "flag" consistently in the standard names, I changed this from ccpp_error_flag to ccpp_error_code. I also changed the units of this standard name to 1 and the units of ccpp_error_message to none in accordance with PR #21 . If this is not acceptable, or will otherwise be a pain to have to implement, I can revert it and just add an issue to do this later.
There was a problem hiding this comment.
BTW, if this change is OK, I don't think the other changes in a18b0e6 should be controversial, and I will approve the PR for merging. Otherwise, I'll wait to approve until after reverting the ccpp_error* entries.
There was a problem hiding this comment.
Changing the standard name for ccpp_error_flag or its units requires a ton of changes in ccpp-physics, fv3atm, scm, and all CCPP documentation. This variable is per requirement used by every scheme and in the host model to catch errors in the physics. Your change makes sense, but I am not sure when there is a good time to change this in the model and ccpp-physics, if at all.
There was a problem hiding this comment.
Changing the standard name for
ccpp_error_flagor its units requires a ton of changes in ccpp-physics, fv3atm, scm, and all CCPP documentation. This variable is per requirement used by every scheme and in the host model to catch errors in the physics. Your change makes sense, but I am not sure when there is a good time to change this in the model and ccpp-physics, if at all.
It will also require updates to ccpp_prebuild.py and the metadata parser of capgen.py, since these variables are hardcoded in several places.
There was a problem hiding this comment.
Ya, that's what I thought, and why I pointed it out. If we're being consistent, these changes reflect how they should be, but it's probably best to live with the inconsistency until it is more convenient to change this. I'll go ahead and revert this and then add an issue to reference if/when there is time to address it.
There was a problem hiding this comment.
I think a good time would be when we update the error handling in the framework and move from the error flag/code and error message to something more sophisticated.
I am a fan of fixing this now. I do not see a good reason to document incorrect usage just because it was done wrong in the past. This just marks a place where the framework needs to catch up to best practice in standard name usage (i.e., open issues in the relevant repositories). |
If you mean fixing it in ESCOMP and then create an issue to at some point later fix it in the actual code, then that is ok of course. |
As far as I see it, either we have the inconsistency with the stated rules in N places or N+1 places. Keeping the change in ESCOMP reduces the inconsistency to N places. Progress! I will start an issue in ccpp-framework. IMO, it can be fixed later there (and in hosts + physics) when there is a good opportunity. |
The following two files were changed with the CCPP standard names that are contained under [non-category] section.
modified: standard_names.xml
modified: Metadata-standard-names.md
fixes #12