Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 constituent tendency capability #584
base: develop
Are you sure you want to change the base?
Add constituent tendency capability #584
Changes from all commits
1ee7a16
922a4dd
44179da
a6f8dd3
0dbc881
1387cf2
dc3f3f3
fb08863
a7eb174
1f7387d
d49fcfa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Similar to the
startswith
comments above, if tendency standard names need to start withtendency_of_
but it's in the middle of a string, this could grabtendency_of_
as the standard name accidentally. We could use.pop()
or[-1]
to grab the last element but since python3.8 is about to be end of life'd in a month, we should consider using the 3.9+removeprefix
option which won't throw an error even if the prefix isn't at the start of the string:const_std_name = std_name.removeprefix('tendency_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.
thanks for pointing out this case! for more context:
If a user inputs something like
nonsense_tendency_of_courtney_constituent
, this code will add the metadata variablevars_layer_tend(:,:,index_of_courtney_constituent)
with standard namenonsense_tendency_of_courtney_constituent
. Which is not right, of course. But then, shortly thereafter, we get an exception in check_tendency_variables() saying that the tendency standard name is invalid.TLDR: this is wrong but maybe it's ok?
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.
Most of this code/logic in check_tendency_variables already exists in the the VarCompatObj (and here).
Could the ability to check for "compatibility between any variable, not just a const variable, and its tendency" be folded into there? (This would make it more useable if someone wanted to do something similar with "state_variables" in the _pre/_post scheme calls.)
I'd be happy to help.
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.
This assumes that
s-1
is always at the end. But it is entirely valid (from a math standpoint) to writekg s-1 m-2
instead ofkg m-2 s-1
. And it's of course also valid to writem s-2
for a tendency of the constituent hadm s-1
...Probably not an issue, but we should document this in the updated (!) CCPP technical documentation
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.
This adjustment for tendency/constituent vars is just for matching horizontal dimensions, correct?
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.
yep! not a permanent change, just a way to make sure that the dimensions match between the tendency variable and the corresponding constituent "state" variable