Skip to content

Remove stub TimeSplit and ProcessSplit objects#736

Merged
climbfuji merged 2 commits into
NCAR:developfrom
mkavulich:feature/remove_process_time_split_logic
Apr 28, 2026
Merged

Remove stub TimeSplit and ProcessSplit objects#736
climbfuji merged 2 commits into
NCAR:developfrom
mkavulich:feature/remove_process_time_split_logic

Conversation

@mkavulich
Copy link
Copy Markdown
Collaborator

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:

@mkavulich mkavulich requested review from a team and gold2718 as code owners April 17, 2026 18:52
@mkavulich mkavulich added the hackathon_20260417 For work done on the April 17 2026 Capgen Hackathon label Apr 17, 2026
Copy link
Copy Markdown
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread scripts/suite_objects.py
Comment on lines -1611 to -1618
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)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@climbfuji
Copy link
Copy Markdown
Collaborator

climbfuji commented Apr 17, 2026

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.

@gold2718
Copy link
Copy Markdown
Collaborator

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?
I am interested because I still would like to tackle the project which would remove the need for all those interstitial routines (which is a lot more code to test, debug, and maintain).

Comment thread schema/suite_v2_0.xsd

<!-- definition of suite elements -->

<xs:element name="time_split">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this has never been implemented, therefore we can remove it from schema version 2 instead of creating a new schema version 3.

@dustinswales
Copy link
Copy Markdown
Member

I support removing this. It's not currently implemented and further complicates the codebase.
The TimeSplit class is partially complete, and not implemented, while the Process Split class is empty. So it doesn't make much sense to keep these stubs in place IMO.
If/when we get to implementing this, we could just add it back in.

@climbfuji climbfuji requested a review from nusbaume April 28, 2026 19:29
Copy link
Copy Markdown
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mkavulich!

@climbfuji
Copy link
Copy Markdown
Collaborator

@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

<lower-case-host-model-identifier>_YYYY-MM-DD_ccpp-prebuild_final

to match the existing tags (e.g. sima_2026-04-13). SIMA folks, you are of course more than welcome to do the same.

After we have the tags, I will start removing ccpp-prebuild. An era comes to an end!

@climbfuji climbfuji merged commit 207d4ee into NCAR:develop Apr 28, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hackathon_20260417 For work done on the April 17 2026 Capgen Hackathon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants