Implementation of CCPP timestep_init and timestep_final phases#344
Conversation
- use CCPP_stages dictionary to abbreviate timestep_init and timestep_final as tsinit and tsfinal in cap subroutine names - drop requirement that CCPP schemes have to have (stub) subroutines for each phase
climbfuji
force-pushed
the
timestep_init_final
branch
from
0bd4405 to
0fa458c
Compare
…amework into timestep_init_final
climbfuji
requested review from
gold2718,
grantfirl and
llpcarson
as code owners
|
I am opening this PR for review. Although unlikely, I may have to pull in updates from NCAR master in case anything gets committed until it is the turn for this PR. But the code changes itself should be final. |
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
I do not see anything concerning but I have a couple of questions.
| # Add special kind variables | ||
| if tmpvar.type in STANDARD_VARIABLE_TYPES and tmpvar.kind and not tmpvar.type == STANDARD_CHARACTER_TYPE: | ||
| kind_var_standard_name = tmpvar.kind | ||
| if not kind_var_standard_name in local_kind_and_type_vars: | ||
| if not kind_var_standard_name in metadata_define.keys(): | ||
| raise Exception("Kind {kind} not defined by host model".format(kind=kind_var_standard_name)) | ||
| kind_var = metadata_define[kind_var_standard_name][0] | ||
| module_use.append(kind_var.print_module_use()) | ||
| local_kind_and_type_vars.append(kind_var_standard_name) | ||
|
|
There was a problem hiding this comment.
Is this directly related to the new phases? I do not immediately see the connection.
There was a problem hiding this comment.
Indirectly. By coincidence, all existing phases (_run, _init, _finalize) always had at least one scheme for each group that used variables with a kind_phys or kind_dyn type directly (not just as part of a DDT_. Because of the lines immediately preceding the new block in lined 138-147, kind_phys was getting passed correctly to the auto-generated caps.
For the time_vary group and phase timestep_init, only the DDTs needed to be passed from the host model to the auto-generated cap, and kind_phys was thus not known to the cap. When blocked data conversions happen, the temporary variables for the DDT constituents are of kind_phys. Thus the additional logic.
| ccpp_var = None | ||
| parent_standard_names = [] | ||
| for ccpp_stage in CCPP_STAGES: | ||
| for ccpp_stage in CCPP_STAGES.keys(): |
There was a problem hiding this comment.
Isn't the .keys() here redundant? Same question on line 303 below.
There was a problem hiding this comment.
Not sure, haven't tried. I generally prefer to add .keys(), because it makes it explicit what we are looping over.
| self._parents = { ccpp_stage : {} for ccpp_stage in CCPP_STAGES.keys() } | ||
| self._arguments = { ccpp_stage : [] for ccpp_stage in CCPP_STAGES.keys() } |
There was a problem hiding this comment.
I do not think you need .keys() here (or below).
gold2718
left a comment
gold2718
left a comment
There was a problem hiding this comment.
I'm seeing the same code I reviewed before. Does that sound right?
Yes, because I replied to your comment that I prefer to explicitly spell out |
I don't either. I was just a bit worried because I saw a new commit ab48311 but no changes for me to review and I wanted to make sure I was looking at it right. |
Thanks for checking, got it. |
3 participants
This PR contains the following changes:
timestep_initandtimestep_finalphasestimestep_initandtimestep_finalastsinitandtsfinalin cap subroutine namesAssociated PRs:
ufs-community/ufs-weather-model#337
NOAA-EMC/ufsatm#217
NCAR/ccpp-physics#534
#344
NOAA-EMC/GFDL_atmos_cubed_sphere#47
For regression testing information, see ufs-community/ufs-weather-model#337.