-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add a rule to clarify vertical stagger suffixes #65
Conversation
It's maybe a little inconsistent that the new names are plural (interfaces), whereas the old name was singular ( |
Thanks @svahl991 for your comment. Yes, I agree that using
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 Also, I did notice that there are several variables which explicitly contain the
Should we also change the |
There was a problem hiding this 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).
Yes, good point @nusbaume. I am in favor of using In my opinion, the current suffix I don't think we need the |
@MayeulDestouches I am happy with just using 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 |
Tagging @danholdaway for feedback. |
Thanks @MayeulDestouches. The changes look good to me, clear and concise meaning in the way the variables are appended. |
There was a problem hiding this 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.
I see there is no reaction to the modification of |
There was a problem hiding this 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.
@climbfuji, thanks for your feedback. I'm happy to remove the About the |
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. |
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 |
There was a problem hiding this 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!
Thanks everyone for your comments. I have reverted to |
There was a problem hiding this 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 !
@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. |
There was a problem hiding this 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).
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 |
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 |
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.