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 a rule to clarify vertical stagger suffixes #65

Merged
merged 7 commits into from
Apr 16, 2024

Conversation

MayeulDestouches
Copy link
Collaborator

This PR solves issue #61 on clarifying the use of the _at_interface suffix. I expanded rule number 4, which describes the use of location suffixes:

By default (when not specified otherwise), variables are grid means or centers (defined by the host). If a variable is defined at a different physical location, a qualifier should be used to denote this. For example, to specify the vertical location of a variable with respect to vertical grid cells, the following rules apply. If [variable] doesn't have a vertical location suffix and is defined on n levels:

  • [variable]_at_interface is defined at the interfaces between grid cells vertically, including the bottom-most and top-most interfaces. It is defined on n+1 levels.
  • [variable]_at_top_interfaces is defined at the interfaces between grid cells vertically, including the top-most interface but excluding the bottom-most interface. It is defined on n levels.
  • [variable]_at_bottom_interfaces is defined at the interfaces between grid cells vertically, including the bottom-most interface but excluding the top-most interface. It is defined on n levels.

I could have added an _at_inner_interfaces suffix for completeness. I haven't as no one has expressed a need for this suffix yet.

@MayeulDestouches
Copy link
Collaborator Author

It looks like I can't add reviewers to this PR. @dudhia, @nusbaume, @svahl991, may I ask you to review this addition please?

@svahl991
Copy link
Collaborator

svahl991 commented Apr 3, 2024

It's maybe a little inconsistent that the new names are plural (interfaces), whereas the old name was singular (_at_interface). Ideally, I think we'd rename _at_interface to _at_all_interfaces for clarity, now that we have so many variations. But that's probably not worth it since it would be a change to so many names. So I don't know if we should change anything in this PR since this is probably the clearest way forward, but I thought I'd bring it up.

@MayeulDestouches
Copy link
Collaborator Author

Thanks @svahl991 for your comment. Yes, I agree that using _at_all_interfaces would be clearer and more consistent. The change would be easy to do in this repository, but may have implications elsewhere, as there are 7 names that already use _at_interface:

  • air_pressure_at_interface
  • air_pressure_of_dry_air_at_interface
  • ln_air_pressure_at_interface
  • ln_air_pressure_of_dry_air_at_interface
  • geopotential_height_at_interface
  • dimensionless_exner_function_at_interface
  • geopotential_at_interface

This is typically a situation where it would be good to have feedback from a list of "required reviewers", as suggested in issue #58. @nusbaume, do you know who uses these current names, or who we could ask approval to before changing them?

@nusbaume
Copy link
Collaborator

nusbaume commented Apr 4, 2024

We do use those variables, but it should be pretty straight-forward to change the at_interface to at_all_interfaces, so if you want to make that change in this PR that's OK with me.

Also, I did notice that there are several variables which explicitly contain the in_atmosphere_layer suffix, usually to distinguish from a variable with an otherwise similar name. The following was the list I found while quickly searching through the Markdown:

reference_pressure_in_atmosphere_layer
reference_pressure_in_atmosphere_layer_normalized_by_surface_reference_pressure
mass_content_of_cloud_ice_in_atmosphere_layer
mass_content_of_cloud_liquid_water_in_atmosphere_layer
nonconvective_cloud_area_fraction_in_atmosphere_layer
index_of_nonconvective_cloud_area_fraction_in_atmosphere_layer_in_tracer_concentration_array
index_of_nonconvective_cloud_area_fraction_in_atmosphere_layer_in_xyz_dimensioned_restart_array
index_of_subgrid_cloud_area_fracation_in_atmosphere_layer_in_xyz_dimensioned_restart_array
subgrid_scale_cloud_area_fraction_in_atmosphere_layer
nonconvective_cloud_area_fraction_in_atmosphere_layer_of_new_state

Should we also change the in_atmosphere_layer to in_all_atmosphere_layers? I am mostly just worried about the "interface" variables being plural, while the "layer" variables are singular. Thoughts?

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.

I think a sentence could be clarified and I inject yet another question into the debate (sorry).

StandardNamesRules.rst Outdated Show resolved Hide resolved
StandardNamesRules.rst Show resolved Hide resolved
@MayeulDestouches
Copy link
Collaborator Author

Should we also change the in_atmosphere_layer to in_all_atmosphere_layers? I am mostly just worried about the "interface" variables being plural, while the "layer" variables are singular. Thoughts?

Yes, good point @nusbaume. I am in favor of using _in_atmosphere_layers rather than _in_atmosphere_layer.

In my opinion, the current suffix _in_atmosphere_layer is not very explicit. It took me a while to understand that mass_content_of_cloud_ice was a 2D variable (integrated content from surface to top of the atmosphere) while mass_content_of_cloud_ice_in_atmosphere_layer was the corresponding 3D variable (integrated content over each model level).
Having a plural (layers) would make it clearer.

I don't think we need the _all_ part here though (_in_all_atmosphere_layers). at_interfaces didn't make it clear whether you include inner interfaces only or top and bottom interfaces as well. There is no such ambiguity with model layers. Also, _in_all_atmosphere_layers could suggest that the quantity is integrated over the whole atmosphere, which is not the case.

@nusbaume
Copy link
Collaborator

nusbaume commented Apr 6, 2024

@MayeulDestouches I am happy with just using _in_atmosphere_layers, I think I just added the all to match the interface suffix, but I agree that it makes it sound like a column or vertically-integrated quantity instead of one defined at each vertical mid-level.

Before making the change though it would be good to get someone more closely associated with NOAA to chime in. @mkavulich @grantfirl any thoughts on changing _in_atmosphere_layer to _in_atmosphere_layers?

@MayeulDestouches
Copy link
Collaborator Author

Tagging @danholdaway for feedback.

@danholdaway
Copy link

Tagging @danholdaway for feedback.

Thanks @MayeulDestouches. The changes look good to me, clear and concise meaning in the way the variables are appended.

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.

I think this is okay.

@MayeulDestouches
Copy link
Collaborator Author

I see there is no reaction to the modification of _in_atmosphere_layer into _in_atmosphere_layers. Unless there are positive reactions to this in the next couple of days, I propose we don't modify _in_atmosphere_layer in this PR. @nusbaume, would you agree with this?
I think it is OK to limit the plural to the _at_... suffixes that clarify vertical stagger.

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 am a bit late to the game, sorry. My preference would have been
_at_interfaces for what is currently at_interface and suggested to become at_all_interfaces, the bottom/top definitions are ok as they are.

Whether that change is being made or not, I would like to see the updates discussed for layers (in this or a future PR; an issue to remind us should suffice) so that the terminology is consistent.

That is: at_layers or at_all_layers instead of at_layer, again with my personal preference being at_layers.

But don't make any changes removing all_ yet just based on my review - I would like to hear from @grantfirl and/or @mkavulich first.

@MayeulDestouches
Copy link
Collaborator Author

@climbfuji, thanks for your feedback. I'm happy to remove the _all_ if other people think similarly.

About the layer, I'm happy to open an issue if it doesn't go in this PR.
However, I would like to clarify that there is no _at_layer in the CCPP naming standard. What have been discussed here is the closest clause, _in_atmosphere_layer, which has nothing to do with the vertical staggering. It is just used to distinguish between a 2D variable [variable] and its 3D counterpart [variable]_in_atmosphere_layer. This is typically used for area fraction or for mass per unit area.

@dustinswales
Copy link
Collaborator

dustinswales commented Apr 15, 2024

My preference would at_interfaces instead of _at_all_interfaces. Adding new conventions for the _botton_interface and _top_interface seems like a fine idea to me.

@climbfuji
Copy link
Collaborator

My preference would at_interface instead of _at_all_interfaces. Adding new conventions for the _botton_interface and _top_interface seems like a fine idea to me.

at_interfaces please (note the plural, which triggered parts of this PR in the first place)

@climbfuji
Copy link
Collaborator

@MayeulDestouches We did discuss this today in the maintainer's meeting and folks were in favor of dropping the _all infix. If you could make this change, please, and ignore my previous comment about the layers?

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.

Sorry for the slow response! This PR looks good to me. @MayeulDestouches if you want to just make an issue regarding my in_atmosphere_layers request then we can postpone that for a future PR (especially if this PR needs to get in ASAP). Thanks!

@MayeulDestouches
Copy link
Collaborator Author

Thanks everyone for your comments. I have reverted to at_interfaces, as asked. I have also opened issue #67 as a reminder of the possible use of _in_atmosphere_layers (with an s).
I think this is now read to be merged.

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 very much @MayeulDestouches !

@climbfuji
Copy link
Collaborator

@cacraigucar @mkavulich @grantfirl @svahl991 Last chance to review - I'll merge tonight if I don't hear otherwise. Thanks!

@gthompsnJCSDA
Copy link
Collaborator

My own opinion is that the Plural version of either interface or layer makes less sense to users. The variable in question is something 2D on a layer by layer or interface by interface of the model coordinate system. The use of plurals is illogical to me. But it seems I am in obvious minority here. I consider the point as such: We have 1 kg/m2 of cloud liquid water path in layer number 26 for example. Or the vertical velocity is 1.4 m/s at model interface level number 12. Making the word plural is yet more confusing because now it invokes to me that it is more than a single level of something.

Copy link
Collaborator

@cacraigucar cacraigucar left a comment

Choose a reason for hiding this comment

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

I tend to lean to having these names be singular instead of plural as well. The dimensionality is what makes the name plural. I'm not going to make a strong stand on this one way or the other. What I will take a stand on though is that there is consistency across names (which I believe we have already agreed upon).

@svahl991
Copy link
Collaborator

I see the point about the singular name being more intuitive in some ways. The fundamental question is whether we're naming the variable for a single layer, or for the collection of all layers.

I think using the singular makes it difficult to distinguish between the different variants of collections of interface variables we need to specify. If we change _at_top_interfaces to _at_top_interface, then I think we also need to change _at_interfaces (which is currently _at_interface) to _at_both_interfaces. Which is again plural, although only refers to a single layer.

@climbfuji
Copy link
Collaborator

Alright ... lacking an individual with executive powers here and given that we've got a large number of approvals and a very late discussion (the PR has been around for at least two weeks) of the pros and cons of the new names, especially what @svahl991 said, I am going to merge this. And I take any blame and responsibility for overriding the concerns brought up last minute.

@climbfuji climbfuji merged commit d54c090 into ESCOMP:main Apr 16, 2024
3 checks passed
@gthompsnJCSDA
Copy link
Collaborator

@climbfuji I am not keeping a close eye on this topic area at all and was alerted by someone else quite recently - so I had no idea the conversation was 2 weeks long. Meanwhile, I do not yet know if a numbered versioning system is in place yet for CCPP names. If not, then it must be added as rapidly as it can because a change for plural name without a versioning system is a pretty obnoxious change to impose to users who are probably not ready. I might be wildly wrong because this isn't my focus area - so if I'm wrong, please forgive the rant.

@climbfuji
Copy link
Collaborator

I agree, we need a versioning system. This can be in form of git tags PLUS the version being coded into the xml file. Let me create an issue for this.

@climbfuji
Copy link
Collaborator

That being said, @gthompsnJCSDA if you want to be alerted and have a more formal way to approve or reject changes, I am currently adding a CODEOWNERS file (#66). Let me know please if you want to be added there. Thanks!

@MayeulDestouches MayeulDestouches deleted the feature/vertical_stagger branch May 10, 2024 11:56
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.

10 participants