Closed MayeulDestouches closed 2 months ago
It looks like I can't add reviewers to this PR. @dudhia, @nusbaume, @svahl991, may I ask you to review this addition please?
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.
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
:
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?
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?
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.
@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
?
Tagging @danholdaway for feedback.
Tagging @danholdaway for feedback.
Thanks @MayeulDestouches. The changes look good to me, clear and concise meaning in the way the variables are appended.
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.
@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.
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.
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)
@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
?
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.
@cacraigucar @mkavulich @grantfirl @svahl991 Last chance to review - I'll merge tonight if I don't hear otherwise. Thanks!
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.
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.
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 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.
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.
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 (https://github.com/ESCOMP/CCPPStandardNames/pull/66). Let me know please if you want to be added there. Thanks!
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: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.