Add constituent tendency capability #584
Conversation
peverwhee
added
enhancement
capgen
peverwhee
requested review from
climbfuji,
gold2718,
dustinswales,
mwaxmonsky,
grantfirl and
mkavulich
as code owners
… constituent-tend
There was a problem hiding this comment.
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.
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.
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.
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.
This adjustment for tendency/constituent vars is just for matching horizontal dimensions, correct?
There was a problem hiding this comment.
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
| 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.
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.
@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.
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