-
Notifications
You must be signed in to change notification settings - Fork 64
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?
Conversation
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.
Looks great so far @peverwhee! Still working through the details but just had a few preliminary questions/comments on the python side of things.
if is_tend_var: | ||
const_std_name = std_name.split("tendency_of_")[1] | ||
else: | ||
const_std_name = std_name |
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 with tendency_of_
but it's in the middle of a string, this could grab tendency_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 variable vars_layer_tend(:,:,index_of_courtney_constituent)
with standard name nonsense_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.
Thanks for this @peverwhee ! One or two most likely dumb questions, other than that looks fine with @mwaxmonsky's comments applied.
else: | ||
tend_dim = dim | ||
# end if | ||
if const_dimensions[index] == 'horizontal_loop_extent': |
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
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.
Some initial comments while I try to wrap my head around things.
Side note: the word "tendency" doesn't look real anymore.
const_kind = const_var.get_prop_value('kind') | ||
if tend_stdname_split == const_stdname: | ||
# We found a match! Check units | ||
if tend_units.split('s-1')[0].strip() != const_units: |
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 write kg s-1 m-2
instead of kg m-2 s-1
. And it's of course also valid to write m s-2
for a tendency of the constituent had m 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.
@peverwhee I think this is fine.
I do have a question about extending some existing code rather than adding your own routine (See inline comments).
# end if | ||
# end for | ||
# end for | ||
# Check that all tendency variables are valid | ||
errmsg, errflg = check_tendency_variables(tend_vars, const_vars) |
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.
Summary
Adds constituent tendency array and enables the use of individual constituent tendency variables.
Description
In order to facilitate time-splitting, this PR adds/enables the following:
User interface changes?: Yes, but optional
User can now use the following in scheme metadata:
ccpp_constituent_tendencies
(standard name for the array of constituent tendencies)tendency_of_CONSTITUENT
(tendency array for a specific constituent)constituent = True
(specifies variable will be handled by constituents object)Testing:
test removed: None
unit tests: Added doctests to new check_tendency_variables function in host_cap.F90
system tests: Modified advection_test to use tendency variable and tendency array
manual testing: Updates run in CAM-SIMA