Skip to content

Conversation

jwallwork23
Copy link
Contributor

Introduce static TimePartition struct for Iterator

Fixes #925

Task List

  • Defined the tests that specify a complete and functioning change (It may help to create a design specification & test specification)
  • Implemented the source code change that satisfies the tests
  • Documented the feature by providing worked example
  • Updated the README or other documentation
  • Completed the pre-Request checklist below

Change Description

This PR follows the recommendation in #925 to reduce duplication of configuration code by introducing a TimePartition struct and a static instance of it associated with the Iterator class. This gets set up during Model.configure and may be accessed from other classes via the Iterator::getTimePartition interface.


Test Description

The XIOS tests are updated to be configured by Model, so all of them now create an instance of this at the start. However, they currently use a reduced configureTime because setting up restart and partition files seems unnecessary for most of them.


Documentation Impact

The XIOS doc pages are updated to reflect these changes.


Pre-Request Checklist

  • The requirements of this pull request are fully captured in an issue or design specification and are linked and summarised in the description of this PR
  • No new warnings are generated
  • The documentation has been updated (or an issue has been created to track the corresponding change)
  • Methods and Tests are commented such that they can be understood without having to obtain additional context
  • This PR/Issue is labelled as a bug/feature/enhancement/breaking change
  • This change conforms to the conventions described in the README

@jwallwork23 jwallwork23 self-assigned this Oct 1, 2025
@jwallwork23 jwallwork23 added enhancement New feature or request ICCS Tasks or reviews for the ICCS team labels Oct 1, 2025
testXiosCalendar_MPI1
PRIVATE "${MODEL_INCLUDE_DIR}" "${XIOS_INCLUDE_LIST}" "${ModulesRoot}/StructureModule"
PRIVATE
${PHYSICS_INCLUDE_DIRS}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add these includes to each of the XIOS tests to avoid linking errors when including Model.hpp.


// Create a Model and configure it so that options are parsed
Model model(MPI_COMM_WORLD);
model.configureTime(); // TODO: Use Model.configure to parse restart files this way, too?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To address this TODO (and the similar ones in the other tests), we'll need to align the config option used for restart files with and without XIOS. It would make sense to me to do so, rather than having separate XIOS-specific options. We want to make the user experience as similar to the non-XIOS case as possible.

@timspainNERSC
Copy link
Collaborator

I need the model start and stop times for issue #926. I wrote some code yesterday to move the time logic to ModelMetadata, which seems a more sensible place for it. Random other parts of the model should not be interfacing with the class that controls the main loop time stepping. Once I work out which branch I committed that code on, I'll post it as a competing pull request.

@jwallwork23
Copy link
Contributor Author

I need the model start and stop times for issue #926. I wrote some code yesterday to move the time logic to ModelMetadata, which seems a more sensible place for it. Random other parts of the model should not be interfacing with the class that controls the main loop time stepping. Once I work out which branch I committed that code on, I'll post it as a competing pull request.

Okay great, thanks @timspainNERSC.

@timspainNERSC
Copy link
Collaborator

See #936

@jwallwork23
Copy link
Contributor Author

Closing in favour of #936.

@jwallwork23 jwallwork23 closed this Oct 2, 2025
@jwallwork23 jwallwork23 deleted the issue925_xios-timestep-start-stop branch October 2, 2025 08:15
timspainNERSC added a commit that referenced this pull request Oct 2, 2025
# Add time limits to `ModelMetadata`

Fixes #932 (partially)

---
# Change Description

Remove the time limit calculations (such as they are) from `Iterator` to
`ModelMetadata`. Also, store the time limits in `ModelMetadata` and
output them to the restart file through `CommonRestartMetadata`. As an
added advantage, these values are now actually written to the restart
file.

---
# Test Description

Ran the standard model configuration by hand to make sure the run length
was still correct. The integration tests in the CI suite will provide
automated testing.

---
# Documentation Impact

None: purely an internal change.

---
# Other Details

As it stands, this may currently break XIOS, as no configuration related
to start time, stop time or time stepping is now passed to XIOS.
@jwallwork23 jwallwork23 mentioned this pull request Oct 2, 2025
11 tasks
jwallwork23 added a commit that referenced this pull request Oct 3, 2025
# Use model config in XIOS

Fixes #925
Follows #936
Second attempt following #932
~~Merges into #913~~

### Task List
- [x] Defined the tests that specify a complete and functioning change
(*It may help to create a [design specification & test
specification](../../../wiki/Specification-Template)*)
- [x] Implemented the source code change that satisfies the tests
- [x] Documented the feature by providing worked example
- [x] Updated the README or other documentation
- [x] Completed the pre-Request checklist below

---
# Change Description

This PR reuses the time metadata set as part of the config set in the
XIOS handler.

---
# Test Description

For the purposes of testing, a separate `Model.configureTime` member
function is created to avoid having to configure *everything*. I also
needed to get the build system to create partition metadata files for 1
and 3 MPI ranks for the XIOS tests and to extend the configuration for
all of them.

---
# Documentation Impact

The XIOS doc pages are updated to reflect these changes.

---
### Pre-Request Checklist

- [x] The requirements of this pull request are fully captured in an
issue or design specification and are linked and summarised in the
description of this PR
- [x] No new warnings are generated
- [x] The documentation has been updated (or an issue has been created
to track the corresponding change)
- [x] Methods and Tests are commented such that they can be understood
without having to obtain additional context
- [x] This PR/Issue is labelled as a bug/feature/enhancement/breaking
change
- [x] This change conforms to the conventions described in the README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ICCS Tasks or reviews for the ICCS team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XIOS: Avoid duplication of timestep and start and stop times
2 participants