Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set up ParaGridIO_Xios as a copy of ParaGridIO for now #758

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jwallwork23
Copy link
Contributor

@jwallwork23 jwallwork23 commented Dec 9, 2024

Set up ParaGridIO_Xios as a copy of ParaGridIO for now

Closes #552
Refinement of #751

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 creates a ParaGridIO_Xios class for us to work on, but just copies what's in ParaGridIO for now. This means that we can convert the functionality to use XIOS piece by piece, rather than doing it all at once.

I can't tick the box for "no new warnings are generated" because warnings are indeed generated to say that we're using the XIOS implementation but that it hasn't been completed yet.


Test Description

CMake for the tests needed to be reordered so that XIOS includes can be picked up. undef INFO is added in ParaGrid_test to avoid a redefinition warning.

An extra CI test workflow is added that builds and runs without XIOS, so we can check that both I/O implementations are working.


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
  • File dates have been updated to reflect modification date
  • This change conforms to the conventions described in the README

@jwallwork23 jwallwork23 added the ICCS Tasks or reviews for the ICCS team label Dec 9, 2024
@jwallwork23 jwallwork23 self-assigned this Dec 9, 2024
@jwallwork23 jwallwork23 added the enhancement New feature or request label Dec 9, 2024
@jwallwork23 jwallwork23 marked this pull request as ready for review December 9, 2024 17:08
Copy link
Collaborator

@timspainNERSC timspainNERSC left a comment

Choose a reason for hiding this comment

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

I'd suggest instead using conditional inclusion in ParaGirdIO.hpp if the non-XIOS and XIOS versions of that header need to diverge.

Comment on lines 3 to 8
if(ENABLE_XIOS)
set(ParaGridIO_Impl "ParaGridIO_Xios")
else()
set(ParaGridIO_Impl "ParaGridIO")
endif()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Posibbly add the .cpp suffix here to make it clear this is a source file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 1291ae4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the expected differences between the current and _Xios versions of the header? Keeping the same header name would mean a lot of the changes in this PR would be unnecessary. If the header does need to change, one option would be to create both ParaGridIO_NoXios.hpp and ParaGridIO_Xios.hpp with the actual header info in, and to have #ifdef #include statements include the correct file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! There will be parts of it that are needed by one implementation and not the other, but we can start out with selective #ifdefs and consider splitting if they diverge too far.

Addressed in 1291ae4.

# for free to join this conversation on GitHub. Already have an account? # 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.

Setup framework for implementing ParaGridIO using XIOS
2 participants