Skip to content

Conversation

TomMelt
Copy link
Contributor

@TomMelt TomMelt commented Nov 25, 2024

Fixes #132

Change Description

This PR adds necessary changes to support halo cells into nextsimDG.

  • ModelArray's are padded with halo cells at the moment they are read from disk
  • This happens in the appropriate IO routines, e.g.,
    • core/src/ParaGridIO.cpp
    • core/src/RectGridIO.cpp
  • Similarly when data is written to disk the halo cells are discarded

This PR does not add halo exchange logic into nextsimDG. This will be done
in a following PR. But all of the functionality is here and it is tested in
core/test/HaloExchangeCB_test.cpp and core/test/HaloExchangePB_test.cpp
(closed and periodic boundaries).

As part of this work, 2 singleton classes have been added:

  • ModelMetadata (already existed but has been refactored as a singleton)
  • ModelMPI (new class)

ModelMPI stores the MPI communication, rank and size. ModelMetadata contains
metadata on the Model which includes domain decomposition information which is
required for halo exchange.

Domain decomposition data is produced by the domain decomp tool, which is then
read by ModelMetadata when it is first initialized.

I have tried to privatize as much of the class data/methods as possible to
simplify their APIs and avoid mis-use. Therefore member data is privatized as
much as possible and public getters have been made available.

Test Description

This PR adds 4 new tests to check the new functionality, for closed and periodic
boundaries (CB/PB respectively)

  • ModelMetadataCB_test.cpp and ModelMetadataPB_test.cpp - test
    getPartitionMetadata can correctly load metadata from a partition metadata
    file into the appropriate data structure e.g., neighbourExtents, etc.
  • HaloExchangeCB_test.cpp and HaloExchangePB_test.cpp - check that we
    can do basic halo exchange by making use of the new slicing utilities e.g.,
    ModelArraySlice

@TomMelt TomMelt marked this pull request as draft November 25, 2024 12:22
@TomMelt TomMelt mentioned this pull request Nov 25, 2024
7 tasks
@TomMelt TomMelt changed the title wip Add halo exchange logic Nov 25, 2024
@jwallwork23
Copy link
Contributor

Note that I changed the logic for determining whether the Docker workflow is run. I an attempt to fix the test failure https://github.com/nextsimhub/nextsimdg/actions/runs/12009927720/job/33475689562 might it be feasible to rebase on top of or merge in develop?

Compare https://github.com/nextsimhub/nextsimdg/blob/halo-exchange/.github/workflows/docker.yml vs https://github.com/nextsimhub/nextsimdg/blob/develop/.github/workflows/docker.yml.

@nextsimhub nextsimhub deleted a comment from coderabbitai bot Nov 25, 2024
@TomMelt TomMelt force-pushed the halo-exchange branch 2 times, most recently from 980be5e to a989bb6 Compare December 20, 2024 17:49
@TomMelt TomMelt changed the title Add halo exchange logic mwe of halo exchange Dec 20, 2024
@TomMelt TomMelt self-assigned this Dec 20, 2024
@TomMelt TomMelt added the ICCS Tasks or reviews for the ICCS team label Dec 20, 2024
@TomMelt TomMelt force-pushed the halo-exchange branch 2 times, most recently from 60cfca0 to 1434d31 Compare January 20, 2025 15:03
@TomMelt TomMelt force-pushed the halo-exchange branch 3 times, most recently from 5cde88d to 9346539 Compare January 28, 2025 14:37
@TomMelt TomMelt force-pushed the melt-mpi-metadata branch from e17547a to a0cd0ae Compare March 18, 2025 16:41
@TomMelt TomMelt force-pushed the halo-exchange branch 2 times, most recently from 7950d7b to 86ceb7e Compare March 19, 2025 19:32
@TomMelt TomMelt force-pushed the melt-mpi-metadata branch from a0cd0ae to 6265961 Compare March 19, 2025 19:32
@TomMelt TomMelt force-pushed the melt-mpi-metadata branch from 6265961 to d741989 Compare April 20, 2025 09:13
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Nice that function signatures are more consistent between the MPI and non-MPI cases with this change.

I've not gone through all of the new test code but I have a few comments and requests on the source code and changes to the XIOS tests.

I think some of the changes you made to ParaGridIO.cpp need to be reflected in ParaGridIO_Xios.cpp, but that's my problem, not yours. I'm working on the relevant bits of code in #916 and #917 anyway.

@TomMelt TomMelt marked this pull request as ready for review September 18, 2025 13:06
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this, @TomMelt! It mostly looks great but I have a few minor requests - mostly related to docs and rewording. Then it should be good to go.

I also have a few suggestions for code changes to ease understanding and reduce duplication but I think they can wait until after this is merged.

TomMelt and others added 2 commits October 2, 2025 09:31
Co-authored-by: Joe Wallwork <22053413+jwallwork23@users.noreply.github.com>
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing those @TomMelt. Let's merge it! 🥳

@TomMelt TomMelt merged commit e2debc9 into develop Oct 2, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Review required to Done in neXtSIM_DG overview Oct 2, 2025
@TomMelt TomMelt deleted the halo-exchange branch October 2, 2025 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ICCS Tasks or reviews for the ICCS team
Projects
Development

Successfully merging this pull request may close these issues.

Support padding of data arrays to enable halo exchanges in distributed-memory version
5 participants