Remove stub TimeSplit and ProcessSplit objects#736
Conversation
gold2718
left a comment
There was a problem hiding this comment.
I'm really not sure what this is helping. These features were originally requested by the NCAR group to make it easier to implement different types of physics packages and regions within them.
Is this code hindering development somehow?
| if (transition == RUN_PHASE_NAME) and ((not group_xml) or | ||
| (group_xml[0].tag not in | ||
| Group.__process_types)): | ||
| # Default is TimeSplit | ||
| tsxml = ET.fromstring(Group.__process_xml[_API_TIMESPLIT_TAG]) | ||
| time_split = new_suite_object(tsxml, context, self, run_env) | ||
| add_to = time_split | ||
| self.add_part(time_split) |
There was a problem hiding this comment.
Has the CAM-SIMA team signed off on this? CAM physics is time split but WRF physics is process split. Does the group planning on implementing WRF physics have no plans to leverage the process split class?
There was a problem hiding this comment.
We have, and at least @peverwhee and I are generally ok with this. Although we will certainly need to support process and time-split physics groups, it is becoming unclear if having the framework manage it is the best option, or if alternative methods (like just implementing specialized tendency_updater schemes which can be runtime controlled) is better.
We of course can always add it back in later if is decided to be handled by the framework, but for now this seems like a reasonable decision to us.
|
I am all in favor of removing this. In UFS/NEPTUNE CCPP physics, the equivalent functionality was implemented in the physics themselves by requiring schemes to always return tendencies and never update the state in place. Interstitials handle the rest when needed. Also, these are just stubs that are indeed in the way of debugging and getting capgen into the UFS and NEPTUNE. We can get actual code back in when we need it. |
Can you please elaborate on exactly how this has hindered your work? |
|
|
||
| <!-- definition of suite elements --> | ||
|
|
||
| <xs:element name="time_split"> |
There was a problem hiding this comment.
I believe this has never been implemented, therefore we can remove it from schema version 2 instead of creating a new schema version 3.
|
I support removing this. It's not currently implemented and further complicates the codebase. |
peverwhee
left a comment
There was a problem hiding this comment.
as @nusbaume mentioned, this is fine by me. If we decide we want this functionality in the future, we can add it back in. But at this point I'm in favor of trimming down framework features that can be achievable with interstitials.
|
@gold2718 A majority of us would like to move ahead with this - I am going to merge it. After this PR is merged, I would like to ask the CCPP framework representatives of at least the host models using ccpp-prebuild to update the ccpp-framework code to the head of develop and, if successful, tag it as to match the existing tags (e.g. After we have the tags, I will start removing ccpp-prebuild. An era comes to an end! |
Logic supporting time-split and process-split grouping natively in Capgen exists as only a stub capability. With no immediate plans to implement this feature, we should remove the unused code to simplify Capgen. If needed in the future this code can be recovered from history.
User interface changes?: No
Fixes: None
Testing:
test removed:
unit tests:
system tests:
manual testing: