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

Add/modify skin temperature at surface variables. #84

Merged
merged 6 commits into from
Jan 2, 2025

Conversation

svahl991
Copy link
Collaborator

@svahl991 svahl991 commented Nov 8, 2024

This PR modifies the existing surface_skin_temperature name to match the new naming rule introduced in #72. It also adds four new skin temperature variable variants. All these variables were introduced into JEDI during the October 2024 variable naming sprint.

Note a small JEDI/CCPP naming discrepancy discussed in #83. I attempted to smooth over this difference in the long name.

@svahl991
Copy link
Collaborator Author

svahl991 commented Nov 8, 2024

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.

I believe we use over_ocean instead of over_sea (and we use over_lake; in case we don't need to distinguish, we use over_water). I checked the standard name dictionary, there are two entries with over_ocean and none with over_sea. In the NCAR ccpp-physics repo, there are six entries with over_ocean, but none with over_sea.

This can be changed if there is a good reason to use sea and not ocean - is there?

@svahl991
Copy link
Collaborator Author

svahl991 commented Nov 8, 2024

This can be changed if there is a good reason to use sea and not ocean - is there?

I will wait a bit for the scientists to chime in. If I don't hear anything I'll change the name in this PR to ocean early next week.

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
I have a question about the meaning (or generality) of the term ice but do not want it to hold up this PR.

standard_names.xml Show resolved Hide resolved
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.

Approving with the assumption that over_sea will be changed to over_ocean.

@mkavulich
Copy link
Collaborator

@svahl991 Just checking in on the status of this PR, assuming that you're okay with the change of over_sea --> over_ocean it looks like this PR can be merged after that change.

@svahl991
Copy link
Collaborator Author

Thanks for the ping @mkavulich . This fell off my radar. I have now changed "sea" to "ocean" (after consulting with @travissluka ), so hopefully this is good to go now.

@climbfuji climbfuji merged commit 71d9a92 into ESCOMP:main Jan 2, 2025
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.

6 participants