ESCOMP / CCPPStandardNames

Repository for community-accepted CCPP Standard Names and search tools
Other
3 stars 16 forks source link

Updated CCPP Standard Names #13

Closed bluefinweiwei closed 3 years ago

bluefinweiwei commented 3 years ago

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

dudhia commented 3 years ago

Not able to open standard_names.xml. Do we need to review this file too?

ligiabernardet commented 3 years ago

@dudhia It is sufficient to review the .md file since it is created from the .xml file.

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r637183364 :

 * `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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r637183364, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77FAZOSQ2OC5SFLEWG3TO2ZRPANCNFSM44IFYSVA .

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r637183364 :

 * `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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r637183364, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77FAZOSQ2OC5SFLEWG3TO2ZRPANCNFSM44IFYSVA .

grantfirl commented 3 years ago

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

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r638260953 :

  • 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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r638260953, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77BBEURANFB6BXWJDTDTPK3PBANCNFSM44IFYSVA .

grantfirl commented 3 years ago

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.

gold2718 commented 3 years ago

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

ligiabernardet commented 3 years ago

Should be timestep (not time step)

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

grantfirl commented 3 years ago

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

Yes. Unfortunately, we are doing this exercise with ~1/3 of names not having been "processed" for consistency, which makes this very messy.

grantfirl commented 3 years ago

During yesterday's ccpp-framework meeting, it was discussed to either 1) leave units off of the standard name list or 2) have a list of units. Regarding the first choice, even though CF includes "canonical units" for each name, it doesn't mean that we have to follow suit, since the CCPP should have unit conversion capability. Deciding on "canonical units" can be yet another conflict point among scientists engaging in this effort and is largely subjective preference, so NOT having units is a way of "punting" on this problem. @dudhia finds that having units can be helpful (at least for the initial standard name decision process), so it was also suggested potentially having a list. To be maximally useful, such a list should be the set of "permissible" units (i.e. those that have conversions defined in the framework).

Since the number of conversions defined is currently small and adds additional complexity, IMO, I would propose to remove units at this time and to add an issue in this repository to revisit the unit list concept when the names are further along. Any thoughts? @ligiabernardet @dudhia @gold2718 @cacraigucar @climbfuji

dudhia commented 3 years ago

Yes, units are very helpful for making sure you're on the same page with the verbal description. My example was that there have been mixed uses of heat flux which I noted in the standard names document. I agree that they should be more for informational purposes than some kind of enforcement.

On Wed, Jun 9, 2021 at 10:30 AM grantfirl @.***> wrote:

During yesterday's ccpp-framework meeting, it was discussed to either 1) leave units off of the standard name list or 2) have a list of units. Regarding the first choice, even though CF includes "canonical units" for each name, it doesn't mean that we have to follow suit, since the CCPP should have unit conversion capability. Deciding on "canonical units" can be yet another conflict point among scientists engaging in this effort and is largely subjective preference, so NOT having units is a way of "punting" on this problem. @dudhia https://github.com/dudhia finds that having units can be helpful (at least for the initial standard name decision process), so it was also suggested potentially having a list. To be maximally useful, such a list should be the set of "permissible" units (i.e. those that have conversions defined in the framework).

Since the number of conversions defined is currently small and adds additional complexity, IMO, I would propose to remove units at this time and to add an issue in this repository to revisit the unit list concept when the names are further along. Any thoughts? @ligiabernardet https://github.com/ligiabernardet @dudhia https://github.com/dudhia @gold2718 https://github.com/gold2718 @cacraigucar https://github.com/cacraigucar @climbfuji https://github.com/climbfuji

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-857851128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77DFO44PHBZQFPLH4FDTR6JKHANCNFSM44IFYSVA .

dudhia commented 3 years ago

I think that timestep is fine. It used to trigger spell checkers, but now it seems it doesn't.

On Wed, Jun 9, 2021 at 10:06 AM grantfirl @.***> wrote:

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

Yes. Unfortunately, we are doing this exercise with ~1/3 of names not having been "processed" for consistency, which makes this very messy.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-857834544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77DOPXH6M3FXPXRFPZDTR6GR5ANCNFSM44IFYSVA .

grantfirl commented 3 years ago

@climbfuji @cacraigucar @dudhia @gold2718 @ligiabernardet @bluefinweiwei The standard_names.xml and associated markdown files have been changed pretty significantly again, although the new standard names and changes are mostly the same as the previous version, just with comments addressed. The big changes were in organization (rather than having one "uncategorized" section, for the purposes of this PR, I organized the names by GFS DDT and alphabetized them) and removal of standard names that have not been "processed" completely on our side (interstitial and diagnostic variables). IMO, this makes it more useful for the spreadsheet that we've been working off of and it makes it usable for a new script that I wrote (standard_name_helper.py -- which can be added in a new PR if desired). Since the XML/MD changed so much with the latest commit, previous comments in this PR will mostly be outdated. I'll do my best to respond to them one by one. Some comments will effectively be "abandoned" because they referred to standard names from the interstitial/diag DDTs that have been temporarily removed from the dictionary, but I took note of these (since Jimy kindly took the time to review) and they can be addressed in a future PR when those names are added.

grantfirl commented 3 years ago

Should be timestep (not time step)

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

Darn it, I missed this comment. There are still a couple variables with "time_step" instead of "timestep". Will change.

grantfirl commented 3 years ago

Should be timestep (not time step)

I see a lot of standard names with the phrase, timestep and also many with time_step. Should, could we standardize on one of these and use it consistently? The rules seem to indicate that we should be using timestep.

Darn it, I missed this comment. There are still a couple variables with "time_step" instead of "timestep". Will change.

time_step -> timestep has been made consistent in the XML/md with the latest commit.

grantfirl commented 3 years ago

I added an issue to revisit the reference pressure issue at some point in the future: https://github.com/ESCOMP/CCPPStandardNames/issues/15

dudhia commented 3 years ago

Regarding volumetric versus volume, we need to use what is in most use in those areas of science. Volumetric for sure is what the land people use for soil moisture, but might sound strange to radiation people.

On Wed, Jul 28, 2021 at 9:32 PM goldy @.***> wrote:

@.**** requested changes on this pull request.

I would still like to understand the rules around these standard names. For instance, these names begin with volume_mixingratio. However, other standard names use volumetric instead (e.g., volumetric_equilibrium_soil_moisture). Is there a reason to choose one over the other? Can we always use one? Please add one or more rules (to StandardNamesRules.rst) to clarify this usage (and that mixing ratios are mass mixing ratios by default or whatever the true default rule is). Also, I still think that we need to specify whether a mixing ratio is wet or dry. The mixing ratios in this PR do not specify either, that should be corrected. This is of critical importance for interoperability. Please add a rule for this and follow that rule for mixing ratios in this PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#pullrequestreview-717636995, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77GUPXCNZQM3IS3NEPDT2DDWTANCNFSM44IFYSVA .

gold2718 commented 3 years ago

Regarding volumetric versus volume, we need to use what is in most use in those areas of science. Volumetric for sure is what the land people use for soil moisture, but might sound strange to radiation people.

I see your point, however, one of the rules is to follow CF conventions and while the current uses of volumetric do not have analogs in the CF table (as far as I can tell), CF does not use the term, volumetric even for land terms (e.g., volume_fraction_of_condensed_water_in_soil_at_wilting_point).

dudhia commented 3 years ago

CF should have a description for soil moisture which is conventionally volumetric, but maybe they leave the word out.

On Thu, Jul 29, 2021 at 9:50 AM goldy @.***> wrote:

Regarding volumetric versus volume, we need to use what is in most use in those areas of science. Volumetric for sure is what the land people use for soil moisture, but might sound strange to radiation people.

I see your point, however, one of the rules is to follow CF conventions and while the current uses of volumetric do not have analogs in the CF table (as far as I can tell), CF does not use the term, volumetric even for land terms (e.g., volume_fraction_of_condensed_water_in_soil_at_wilting_point).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-889260939, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77HKCMX3K5YD5Q227U3T2F2ETANCNFSM44IFYSVA .

dudhia commented 3 years ago

so alternative names here might be dry_mass_mixing_ratio and moist_mass_mixing_ratio

On Thu, Jul 29, 2021 at 9:51 AM goldy @.***> wrote:

@.**** commented on this pull request.

In standard_names.xml https://github.com/ESCOMP/CCPPStandardNames/pull/13#discussion_r679278116 :

@@ -362,48 +362,48 @@ long_name="Ratio of the mass of ice to the mass of dry air">

real

I would prefer the term, mass included here (e.g., mass_mixing_ratio_of_rain, which is consistent with the way the volume mixing ratio standard names are constructed). However, even if it is decided that mass mixing ratio is the default, please include the term, 'mass mixing ratio' in a custom long_name property for these quantities.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#pullrequestreview-718264080, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77A4CRF2QPJF66WR3P3T2F2HFANCNFSM44IFYSVA .

gold2718 commented 3 years ago

CF should have a description for soil moisture which is conventionally volumetric, but maybe they leave the word out.

Searching for the combination of "soil" and "moisture" does not turn up any volumetric terms, only mass fraction terms and "moisture_content" terms which are kg m-2.

gold2718 commented 3 years ago

so alternative names here might be dry_mass_mixing_ratio and moist_mass_mixing_ratio

Or mass_mixing_ratio_of_<x>_dry_air (similar to surface_pressure_of_dry_air which is already in the dictionary) and mass_mixing_ratio_of_<x>_of_moist_air.

climbfuji commented 3 years ago

so alternative names here might be dry_mass_mixing_ratio and moist_mass_mixing_ratio

It's more complicated with that, because moist_mass_mixing_ratio will require additional qualifiers like with_respect_to. Moist mixing ratios can be with respect to just specific humidity, or all hydrometeors, ...

dudhia commented 3 years ago

maybe mass_mixing_ratioof*_with_respect_to_moist_mass ? Is mass_mixing_ratio an accepted term? Maybe the first mass is redundant because it is implied in mixing_ratio.

On Thu, Jul 29, 2021 at 10:50 AM Dom Heinzeller @.***> wrote:

so alternative names here might be dry_mass_mixing_ratio and moist_mass_mixing_ratio … <#m6197343537365292180>

It's more complicated with that, because moist_mass_mixing_ratio will require additional qualifiers like with_respect_to. Moist mixing ratios can be with respect to just specific humidity, or all hydrometeors, ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-889303893, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIZ77CEACMWJ7UMQH4TWV3T2GBDVANCNFSM44IFYSVA .

ligiabernardet commented 3 years ago

Pls see proposed rules clarification in https://github.com/ESCOMP/CCPPStandardNames/pull/16. Once we agree on the rules, we can make changes in the names listed in this PR.

grantfirl commented 3 years ago

@gold2718 @cacraigucar Since "constituent_mixing_ratio" originally came from you all, and according to the latest rule change regarding "mixing_ratio", should we add ...wrt_dry_air, wrt_moist_air, wrt_total_mass to this standard name?

cacraigucar commented 3 years ago

@gold2718 @cacraigucar Since "constituent_mixing_ratio" originally came from you all, and according to the latest rule change regarding "mixing_ratio", should we add ...wrt_dry_air, wrt_moist_air, wrt_total_mass to this standard name?

I will defer to @gold2718 on this one.

grantfirl commented 3 years ago

@ligiabernardet @bluefinweiwei The e97a517 commit from @bluefinweiwei changed the volume_mixing_ratio units from kg kg-1 to m3 m-3. The rules now state that they should be in mol mol-1, so this needs to be reflected in this PR (undoing commit e97a517), right?

ligiabernardet commented 3 years ago

Yes. We had switched to m3 m-3, but now have settled on mol mol-1.

On Fri, Aug 6, 2021 at 10:23 AM grantfirl @.***> wrote:

@ligiabernardet https://github.com/ligiabernardet @bluefinweiwei https://github.com/bluefinweiwei The e97a517 commit from @bluefinweiwei https://github.com/bluefinweiwei changed the volume_mixing_ratio units from kg kg-1 to m3 m-3. The rules now state that they should be in mol mol-1, so this needs to be reflected in this PR (undoing commit e97a517), right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-894372009, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE7WQAVZSVPRBJDQ43CVUZTT3QEAPANCNFSM44IFYSVA . 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 .

gold2718 commented 3 years ago

@grantfirl, @cacraigucar,

Since "constituent_mixing_ratio" originally came from you all, and according to the latest rule change regarding "mixing_ratio", should we add ...wrt_dry_air, wrt_moist_air, wrt_total_mass to this standard name?

Since the array can contain a mix of all these plus number concentration fields, how about just constituents_array? Or, maybe we should take this out entirely since there is no way it will ever be fixed (e.g., different chemistry suite will have different constituents). Given the CCPP principle that two objects with the same standard name represent the same physical quantity, this feels like something that should not be listed.

Since we do need something to transport arrays of these items around, maybe we should have constituent_array with all the usual rules for mixing ratios. I feel this would work because chemistry suites normally keep all of their constituents in the same form (e.g., volume_mixing_ratio_wrt_total_mass) so the only thing that would vary is the number (and type) of species.

grantfirl commented 3 years ago

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

ligiabernardet commented 3 years ago

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.

grantfirl commented 3 years ago

@grantfirl, @cacraigucar,

Since "constituent_mixing_ratio" originally came from you all, and according to the latest rule change regarding "mixing_ratio", should we add ...wrt_dry_air, wrt_moist_air, wrt_total_mass to this standard name?

Since the array can contain a mix of all these plus number concentration fields, how about just constituents_array? Or, maybe we should take this out entirely since there is no way it will ever be fixed (e.g., different chemistry suite will have different constituents). Given the CCPP principle that two objects with the same standard name represent the same physical quantity, this feels like something that should not be listed.

Since we do need something to transport arrays of these items around, maybe we should have constituent_array with all the usual rules for mixing ratios. I feel this would work because chemistry suites normally keep all of their constituents in the same form (e.g., volume_mixing_ratio_wrt_total_mass) so the only thing that would vary is the number (and type) of species.

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. Use an umbrella term like "constituent_concentration" or "tracer_concentration" as a catch-all for whatever the host/physics is using.
  2. 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".

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.

gold2718 commented 3 years ago

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

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-894401435, 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 .

grantfirl commented 3 years ago

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:

  1. Create 3 constituent variables/standard names to represent arrays with common denominators: constituent_mixing_ratio becomes one of constituent_mixing_ratio_wrt_moist_air/constituent_mixing_ratio_wrt_dry_air/constituent_mixing_ratio_wrt_total mass
  2. Change NOAA's "tracer_concentration" variable to use the new constituent_mixing_ratio_wrt_moist_air, but it will just have stuff like TKE and cloud_fraction still tacked on to the end?

Any better ideas? I can't finish updating this PR with respect to mixing ratios until this is decided.

@ligiabernardet @dudhia @gold2718

ligiabernardet commented 3 years ago

@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_ratioof and keeping the TKE and cloud fraction in it is hard to justify.

cacraigucar commented 3 years ago

Why units of none instead of index like the other control variables?

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.

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#issuecomment-902989666, 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 .

gold2718 commented 3 years ago

Index should be used only for integers that can be an index in an array.

I like this statement, it should be in the rules (do we need a separate document for rules on units?).

climbfuji commented 3 years ago

Index should be used only for integers that can be an index in an array.

I like this statement, it should be in the rules (do we need a separate document for rules on units?).

@gold2718 how about this: https://github.com/ESCOMP/CCPPStandardNames/pull/21

dudhia commented 3 years ago

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 https://github.com/ESCOMP/CCPPStandardNames/pull/13#pullrequestreview-736716850, 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 .

climbfuji commented 3 years ago

It looks like we are very close to merging this PR. @grantfirl @gold2718 can you please re-review and approve if satisfied?

gold2718 commented 3 years ago

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 .

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

climbfuji commented 3 years ago

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 .

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.

grantfirl commented 3 years ago

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 .

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.