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

New wind variables at surface #75

Merged
merged 7 commits into from
Oct 1, 2024

Conversation

svahl991
Copy link
Collaborator

This adds two pairs of wind variables for winds at the surface layer.

Note that the names here assume that the naming rule changes in #72 will be merged.

@svahl991
Copy link
Collaborator Author

Tagging @shlyaeva

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.

LGTM

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.

I am probably making a mountain out of a molehill, but what is the "surface" that these standard names are referring to? For example my understanding is that when meteorologists report surface winds they are usually reporting winds at ten meters above the surface, which we already have a suffix for (at_10m).

Alternatively my (extremely limited) understanding of boundary layer turbulence assumes that the wind speeds at the actual surface are exactly zero relative to the surface itself, so any local non-zero wind speed must be defined at some height above the actual surface.

Given this, if this is a quantity defined on model levels, would something like at_surface_adjacent_layer or at_bottom_interface be a better suffix?

Anyways, I certainly won't hold up this PR for this as it sounds like it is needed soon, but I am just worried that we are potentially adding some ambiguity here. Of course I'm happy to hear if people disagree. Thanks!

@ss421
Copy link
Collaborator

ss421 commented Sep 24, 2024

I am probably making a mountain out of a molehill, but what is the "surface" that these standard names are referring to? For example my understanding is that when meteorologists report surface winds they are usually reporting winds at ten meters above the surface, which we already have a suffix for (at_10m).

Alternatively my (extremely limited) understanding of boundary layer turbulence assumes that the wind speeds at the actual surface are exactly zero relative to the surface itself, so any local non-zero wind speed must be defined at some height above the actual surface.

Given this, if this is a quantity defined on model levels, would something like at_surface_adjacent_layer or at_bottom_interface be a better suffix?

Anyways, I certainly won't hold up this PR for this as it sounds like it is needed soon, but I am just worried that we are potentially adding some ambiguity here. Of course I'm happy to hear if people disagree. Thanks!

I agree with this comment but I do not have anything useful to add as I'm not sure what at_surface means in particular since we have the at_10m suffix. I can see in JEDI that we have an "at surface" wind variable name. Would be great if the detail related to that could be added either in the name as suggested or in the description of the name. I appreciate that may not be possible and I do not want to hold this up so am approving in this basis.

@svahl991
Copy link
Collaborator Author

svahl991 commented Sep 24, 2024

I am probably making a mountain out of a molehill, but what is the "surface" that these standard names are referring to?

The names in this PR were previously part of #71 (but were removed from that PR for other reasons), and this same discussion came up there. See comments from @BenjaminTJohnson and @mikecooke77 on that PR.
#71 (comment)

Also tagging @shlyaeva

@nusbaume
Copy link
Collaborator

@svahl991 Thanks for pointing me to the extra discussion! I think that all makes sense to me. Just to make sure it is as clear as possible, could we possibly update the long names to make it obvious that these wind fields are whatever wind variable is closest to the surface? So using eastward_wind_at_surface as an example, could the long name be:

Wind vector component closest to surface, positive when directed eastward

@svahl991
Copy link
Collaborator Author

So using eastward_wind_at_surface as an example, could the long name be:
Wind vector component closest to surface, positive when directed eastward

I've now updated the long names in this PR with this clarification, as requested.

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 humoring me! Everything looks good on my end now.

@climbfuji
Copy link
Collaborator

Thanks @svahl991. Let's wait for @mkavulich to review and merge?

Copy link

@mikecooke77 mikecooke77 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 adding

@svahl991
Copy link
Collaborator Author

svahl991 commented Oct 1, 2024

Both this PR and #76 have several approvals and don't seem to have any ongoing discussions. Is it possible to get them merged soon?

@climbfuji
Copy link
Collaborator

We are unfortunately still waiting for approval from the NOAA side. I pinged @mkavulich and @dustinswales in the other PR #76 a few days ago.

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

@climbfuji Sorry for the delay.
No objections from me.

@climbfuji climbfuji merged commit 583a379 into ESCOMP:main Oct 1, 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.

8 participants