Read input file to determine dimensions #974

joewallwork · 2025-11-04T11:42:06Z

Read input file to determine dimensions

Fixes #972
Merges into #968

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

Currently, there are differences in how the serial, MPI-parallel, and XIOS configurations read dimensions. For greater consistency, this PR implements a setDimensionsFromFile member function for ModelMetadata. It gets called in Model.configureRestarts. It also gets called in ParaGridIO.getModelState (for consistency with the existing implementation).

Test Description

XIOS tests are updated to account for the fact that dimensions are set in Model.configureRestarts.

Documentation Impact

None

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

joewallwork · 2025-11-04T15:00:49Z

[Rebased on top of updated base branch]

joewallwork · 2025-11-10T16:57:26Z

[Rebased on top of develop again.]

joewallwork · 2025-11-18T10:57:06Z

core/src/ParaGridIO.cpp

+    // TODO: Do this once rather than with every read
+    ModelMetadata& metadata = ModelMetadata::getInstance();
+    metadata.setDimensionsFromFile(filePath);


Question for reviewer: Do we really want to set dimensions from file in every call to getModelState, as in the existing implementation? Would it not be better to do the following?

Suggested change

// TODO: Do this once rather than with every read

ModelMetadata& metadata = ModelMetadata::getInstance();

metadata.setDimensionsFromFile(filePath);

ModelMetadata& metadata = ModelMetadata::getInstance();

if (filePath /= metadata.initialFileName) {

metadata.setDimensionsFromFile(filePath);

}

How many calls to getModelState() are expected? In each model run, there should only be one call to SomethingGrid::getModelState() and within that one call to SomethingGridIO::getModelState() (where SomethingGrid is the name of the IStructureimplementing class). There are a few more examples around the code base for testing, but I believe that is all.

Fair point, thanks. I removed the TODO in 17ea449 and will merge once tests pass.

timspainNERSC

If we were calling getModelState() in the main model loop, the extra change would be worth adding. As it should only be called once per run, I'm not sure even the small amount of added complexity is worthwhile.

timspainNERSC · 2025-11-18T12:16:52Z

core/src/ParaGridIO.cpp

+    // TODO: Do this once rather than with every read
+    ModelMetadata& metadata = ModelMetadata::getInstance();
+    metadata.setDimensionsFromFile(filePath);


How many calls to getModelState() are expected? In each model run, there should only be one call to SomethingGrid::getModelState() and within that one call to SomethingGridIO::getModelState() (where SomethingGrid is the name of the IStructureimplementing class). There are a few more examples around the code base for testing, but I believe that is all.

joewallwork · 2025-11-18T12:57:50Z

Actually, before merging I should check with @Thanduriel that this PR does what was requested and will be of use in the GPU port?

Thanduriel

I solved my buffer initialization problem by doing it lazily on GPU, so as long as the CPU initialization still works it should be fine for me.

I noticed that you sometimes pass large objects (like std::string) by value. This is considered bad practice in general for performance reasons (see the CppCoreGuidelines). It does not really matter in the places I commented on, so you decide if you want to change it, but I think it is best to simply always follow the linked rule.

Thanduriel · 2025-12-01T09:51:38Z

core/src/Xios.cpp

    auto& metadata = ModelMetadata::getInstance();

    // Initial read of the NetCDF file to deduce the dimensions
    for (std::string filename : { inputFilename, forcingFilename }) {


Unnecessary copy.

for(const std::string& filename : ...)

Thanks @Thanduriel, fixed in 7c27843.

Thanduriel · 2025-12-01T09:53:09Z

core/src/ModelMetadata.cpp


 #endif

+void ModelMetadata::setDimensionsFromFile(const std::string filename)


Unnecessary copy, could be a reference.

void ModelMetadata::setDimensionsFromFile(const std::string& filename)

Fixed in 7c27843.

Thanduriel · 2025-12-01T09:53:36Z

core/src/include/ModelMetadata.hpp

+     *          set. Otherwise, a consistency check is made against the dimensions read from file
+     *          and already set.
+     */
+    void setDimensionsFromFile(const std::string filename);


# XIOS handler restructure Fixes #850 Fixes #1000 ~~Merges into #974~~ This will hopefully be the last major restructuring of the XIOS handler class. The PR includes some pruning: * Remove unused `enableXios` function. * Drop errors from the `ParaGridIO_Xios` member functions already thrown in `Xios.read` and `Xios.write`. * Cut down the `configGetX` member functions. * Remove the `contextId` and `calendarType` arguments from the constructor because they're now set in `iodef.xml` as of #993. It also includes better modularisation: * Extract a new `setupFiles` member function out of the constructor. * Rename `parseInputFiles` as `setupFields`. * Split the configuration into `setupClient`, `setupContext`, and `setupCalendar` and have all of them called from `configure` alongside `setupFiles` and `setupFields`. Other misc changes: * Additional checking of the calendar setup. * Reordering of member functions to better group XIOS concepts together. * Better use of references to avoid unnecessary copies. * Use of `const` qualifier in more places.

joewallwork self-assigned this Nov 4, 2025

joewallwork added enhancement New feature or request ICCS Tasks or reviews for the ICCS team labels Nov 4, 2025

joewallwork force-pushed the issue972_initial-read branch from dcdf6b4 to 6aaebd5 Compare November 4, 2025 15:00

joewallwork linked an issue Nov 4, 2025 that may be closed by this pull request

Initial read of input file to get dimensions #972

Closed

Base automatically changed from issue850_xios-pruning to develop November 10, 2025 14:52

joewallwork force-pushed the issue972_initial-read branch from c60b3eb to f5428d5 Compare November 10, 2025 16:57

joewallwork added 10 commits November 18, 2025 10:35

Implement ModelMetadata::setDimensionsFromFile and use for XIOS and MPI

79d8c43

Skip some dimensions as in ParaGridIO

2b1cca2

Move function definition outside of preproc block

ca072ed

Check dims from file consistent with partition

e831e52

Fix includes for serial case

8bc5dbb

Increase time limit for serial jobs

56632ca

Use setDimensionsFromFile in XIOS write test

b07568d

Post-rebase fix

725e0cb

No need for extents in serial

14e7d52

Call setDimensionsFromFile in Model.configureRestarts; update tests

e45bfe1

joewallwork force-pushed the issue972_initial-read branch from f5428d5 to e45bfe1 Compare November 18, 2025 10:52

joewallwork commented Nov 18, 2025

View reviewed changes

joewallwork marked this pull request as ready for review November 18, 2025 10:57

joewallwork requested review from TomMelt, einola and timspainNERSC November 18, 2025 11:03

timspainNERSC approved these changes Nov 18, 2025

View reviewed changes

Drop unplanned TODO; improve comment consistency

17ea449

joewallwork requested a review from Thanduriel November 18, 2025 12:57

Merge branch 'develop' into issue972_initial-read

df6bacd

joewallwork mentioned this pull request Nov 18, 2025

XIOS handler restructure #996

Merged

Thanduriel approved these changes Dec 1, 2025

View reviewed changes

Avoid unnecessary copies

7c27843

joewallwork merged commit 15538b6 into develop Dec 1, 2025
9 checks passed

joewallwork deleted the issue972_initial-read branch December 1, 2025 10:23

joewallwork mentioned this pull request Dec 1, 2025

Avoid unnecessary copies in XIOS handler class #1000

Closed


		#endif

		void ModelMetadata::setDimensionsFromFile(const std::string filename)

Read input file to determine dimensions #974

Read input file to determine dimensions #974

Uh oh!

Conversation

joewallwork commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Read input file to determine dimensions

Task List

Change Description

Test Description

Documentation Impact

Pre-Request Checklist

Uh oh!

joewallwork commented Nov 4, 2025

Uh oh!

joewallwork commented Nov 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timspainNERSC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joewallwork commented Nov 18, 2025

Uh oh!

Thanduriel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joewallwork commented Nov 4, 2025 •

edited

Loading

Thanduriel left a comment •

edited

Loading