Update several units in CCPP metadata, contains "Expanded format fields for percentages ..." (#777), update CODEOWNERS #775
Conversation
… non-physical quantities, fix units of humidity diagnostic variables
…ect_units_due_to_feature_capgen
…into correct_units_due_to_feature_capgen
climbfuji
changed the title
…ect_units_due_to_feature_capgen
climbfuji
changed the title
|
@grantfirl for some reason I can't add you as reviewer to this PR. Maybe you didn't exist or were caught in a space-time bubble while transferring from NCAR to NOAA. |
There was a problem hiding this comment.
I have mixed thoughts about replacing things like Pa**kappa and log(Pa) with 1. According to https://math.stackexchange.com/questions/238390/units-of-a-log-of-a-physical-quantity, it would probably make more sense to replace log(Pa) with Pa. As for Pa**kappa, we (CCPP) may need to support exponents of units that are non-integers, because that does come up occasionally. Although, to be consistent with our unit conventions, it would then need to be Pa0.2858 or some ugly-looking thing. Thoughts?
@gold2718 @dudhia @ligiabernardet @grantfirl What are your thoughts? The PR is scheduled for commit tomorrow or the day after. I would prefer to use |
I looked through |
|
I move that we merge the PR as is (subject to regression tests passing) and change any units later after a broader discussion of how to handle this problem. Otherwise, we'll just hold up the PR, and more pressing matters won't be resolved. |
|
My thought is to leave it as log(Pa) which is a lot more informational than
1.
As for Pa^kappa, there will be things with "special powers" that may be
fractional or another parameter.
This is the only current example, but I know some of those exist within
schemes. A convention is needed.
…On Mon, Nov 22, 2021 at 3:51 PM Dom Heinzeller ***@***.***> wrote:
I have mixed thoughts about replacing things like Pa*kappa and log(Pa)
with 1. According to
https://math.stackexchange.com/questions/238390/units-of-a-log-of-a-physical-quantity
<https://math.stackexchange.com/questions/238390/units-of-a-log-of-a-physical-quantity>,
it would probably make more sense to replace log(Pa) with Pa. As for Pa*kappa,
we (CCPP) may need to support exponents of units that are non-integers,
because that does come up occasionally. Although, to be consistent with our
unit conventions, it would then need to be Pa0.2858 or some ugly-looking
thing. Thoughts?
@gold2718 <https://github.com/gold2718> @dudhia
<https://github.com/dudhia> @ligiabernardet
<https://github.com/ligiabernardet> @grantfirl
<https://github.com/grantfirl> What are your thoughts? The PR is
scheduled for commit tomorrow or the day after. I would prefer to use
fraction or frac for the units of relative humidity when they are not
percentages instead of 1. I have no strong preference for the units of
log(Pa) and Pa**kappa.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77BQOKFZGFC7UFN5BNTUNLCNJANCNFSM5HX53CUQ>
.
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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
What is the purpose of the units? Is it to be parseable by automated tools like udunits? Or is it just for people to read? For automated tools, we have to follow an existing convention. People are usually a bit more flexible. |
The units must be parseable, not only for matching but also for automated unit conversions. Yes, we try to follow the udunits standard, and |
|
If the |
physics/h2o_def.meta
Outdated
| @@ -22,7 +22,7 @@ | |||
| [h2o_pres] | |||
| standard_name = natural_log_of_h2o_forcing_data_pressure_levels | |||
| long_name = natural log of h2o forcing data pressure levels | |||
There was a problem hiding this comment.
See here @SamuelTrahanNOAA - both standard name and long name have the log of ... pressure
| @@ -74,7 +74,7 @@ | |||
| [ph2o] | |||
| standard_name = natural_log_of_h2o_forcing_data_pressure_levels | |||
| long_name = natural log of h2o forcing data pressure levels | |||
| units = log(Pa) | |||
| units = 1 | |||
There was a problem hiding this comment.
Also here @SamuelTrahanNOAA - both standard name and long name have the log of ... pressure
physics/gfdl_fv_sat_adj.meta
Outdated
| @@ -341,7 +341,7 @@ | |||
| [pkz] | |||
| standard_name = finite_volume_mean_edge_pressure_raised_to_the_power_of_kappa | |||
| long_name = finite-volume mean edge pressure raised to the power of kappa | |||
There was a problem hiding this comment.
And here @SamuelTrahanNOAA - both standard name and long name have "finite-volume mean edge pressure raised to the power of kappa"
There was a problem hiding this comment.
The only dispute I've heard about this is removing log(Pa) and similar in the units. It seems to me that @climbfuji has come up with a reasonable solution to the problem, given the constraints on the contents of the units variable. I see no reason to delay a commit.
|
The long name should include the Pa information in this case as it is not
in the unit itself.
…On Tue, Nov 23, 2021 at 12:50 PM Samuel Trahan (NOAA contractor) < ***@***.***> wrote:
***@***.**** approved this pull request.
The only dispute I've heard about this is removing log(Pa) and similar in
the units. It seems to me that @climbfuji <https://github.com/climbfuji>
has come up with a reasonable solution to the problem, given the
constraints on the contents of the units variable. I see no reason to
delay a commit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#775 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77HYY64OK67K6KQFSBDUNPV77ANCNFSM5HX53CUQ>
.
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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Yes, this is what Sam also asked for. They do, see my comments in |
|
I mean Pascals should be explicitly in the long name as the magnitude
depends on it.
…On Tue, Nov 23, 2021 at 12:56 PM Dom Heinzeller ***@***.***> wrote:
The long name should include the Pa information in this case as it is not
in the unit itself.
… <#m_-8180411912721675403_>
Yes, this is what Sam also asked for. They do, see my comments in
physics/h2o_def.meta, physics/h2ophys.meta, physics/gfdl_fv_sat_adj.meta.
It's in both the long name and the standard name.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77DX7LHYCELG7NSQA6LUNPWXFANCNFSM5HX53CUQ>
.
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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
But didn't we agree on the rule that units are only mentioned explicitly if they deviate from the standard for the quantity in question? I have no problem changing two or three long names post-testing, because we know for sure that those are completely ignored by the parser or code generator, but I want to make sure that we follow the rules. |
|
Pressure is a bit more variable in units than most, but if MKS is assumed
it is fine.
…On Tue, Nov 23, 2021 at 1:06 PM Dom Heinzeller ***@***.***> wrote:
I mean Pascals should be explicitly in the long name as the magnitude
depends on it.
… <#m_7242324771864304631_>
But didn't we agree on the rule that units are only mentioned explicitly
if they deviate from the standard for the quantity in question? I have no
problem changing two or three long names post-testing, because we know for
sure that those are completely ignored by the parser or code generator, but
I want to make sure that we follow the rules.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIZ77FG4X26I5RPNI3FRC3UNPX5JANCNFSM5HX53CUQ>
.
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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Good point. Let's add "in Pa" to the three long names before committing.
… On Nov 23, 2021, at 1:29 PM, dudhia ***@***.***> wrote:
Pressure is a bit more variable in units than most, but if MKS is assumed
it is fine.
On Tue, Nov 23, 2021 at 1:06 PM Dom Heinzeller ***@***.***>
wrote:
> I mean Pascals should be explicitly in the long name as the magnitude
> depends on it.
> … <#m_7242324771864304631_>
>
> But didn't we agree on the rule that units are only mentioned explicitly
> if they deviate from the standard for the quantity in question? I have no
> problem changing two or three long names post-testing, because we know for
> sure that those are completely ignored by the parser or code generator, but
> I want to make sure that we follow the rules.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#775 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEIZ77FG4X26I5RPNI3FRC3UNPX5JANCNFSM5HX53CUQ>
> .
> 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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#775 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RISWCMBZQWESCZ6THDUNP2T3ANCNFSM5HX53CUQ>.
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&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Regression testing complete, will merge now. |
Description
With the metadata parser update in NCAR/ccpp-framework#415, a stricter checking of units will be enforced. This includes the replacement of "convenience units" such as
log(Pa)with1. All changes:1%to1module_mp_thomson.F90from PR add semi-Lagrangian sedimentation of graupel into Thompson MP #770Contains all changes from PR #777.
Fixes #776.
Last-minute change: add @mkavulich to CODEOWNERS
Associated PRs
NCAR/ccpp-framework#415
#775
NOAA-EMC/fv3atm#422
ufs-community/ufs-weather-model#907
Testing
See ufs-community/ufs-weather-model#907