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

Update Standard Name Rules table for dimensionless units, change variables improperly using "1" to "fraction" #62

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

mkavulich
Copy link
Collaborator

Related to Issue #26 and discussion therein, this PR updates the Standard Name Rules table for dimensionless units, removing some ambiguity in wording, separating out "frac" from "percent" entries (since fractions and percentages are different things, and can theoretically be converted) and clarifying when "1" should be used. In addition, several standard names that reference fractions are updated to appropriately use "frac" rather than "1".

Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this @mkavulich! Would you be willing to add a couple extra unit changes to this PR? Specifically the following:

  1. cellular_automata_seed_probability currently has units of fraction. Might be worth changing that to frac.
  2. chemical_tracer_scavenging_fractions should probably also have units of frac.

Otherwise I think everything else looks good, at least to me.

@climbfuji
Copy link
Collaborator

Why can we not spell out fraction? We also spell out percent. The 8-character limit in F77 is history!

@mkavulich
Copy link
Collaborator Author

@climbfuji I'm just going off the existing conventions; all the schemes in ccpp-physics currently use frac. But if we're okay with committing to that change then I'm all for using fraction.

@nusbaume
Copy link
Collaborator

nusbaume commented Apr 1, 2024

I'm personally fine with either frac or fraction, just as long as we aren't using both of them together. I also agree that fraction is certainly less ambiguous (but would require more effort on this particular PR).

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay for me

@climbfuji
Copy link
Collaborator

If it's not too much work, can you change all frac to fraction please?

@mkavulich mkavulich changed the title Update Standard Name Rules table for dimensionless units, change variables improperly using "1" to "frac" Update Standard Name Rules table for dimensionless units, change variables improperly using "1" to "fraction" Apr 15, 2024
@mkavulich
Copy link
Collaborator Author

@nusbaume @climbfuji I have made the suggested changes

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for considering my comments!

@climbfuji climbfuji requested a review from nusbaume April 15, 2024 21:22
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me now, thanks!

@climbfuji climbfuji merged commit 98e81e5 into ESCOMP:main Apr 16, 2024
3 checks passed
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.

5 participants