-
Notifications
You must be signed in to change notification settings - Fork 19
halo exchange integration #744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 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. |
b3882c1
to
7c3adbb
Compare
88f9e68
to
63e6f77
Compare
3adf0d1
to
e5c31a6
Compare
e5c31a6
to
d1b8951
Compare
63e6f77
to
aa15779
Compare
980be5e
to
a989bb6
Compare
aa15779
to
319b07e
Compare
60cfca0
to
1434d31
Compare
5cde88d
to
9346539
Compare
e17547a
to
a0cd0ae
Compare
7950d7b
to
86ceb7e
Compare
a0cd0ae
to
6265961
Compare
6265961
to
d741989
Compare
There was a problem hiding this 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.
6bf8053
to
80f159b
Compare
6fd44c5
to
c683062
Compare
a82adee
to
ae334ca
Compare
There was a problem hiding this 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.
Co-authored-by: Joe Wallwork <22053413+jwallwork23@users.noreply.github.com>
There was a problem hiding this 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! 🥳
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 diskcore/src/ParaGridIO.cpp
core/src/RectGridIO.cpp
This PR does not add halo exchange logic into
nextsimDG
. This will be donein a following PR. But all of the functionality is here and it is tested in
core/test/HaloExchangeCB_test.cpp
andcore/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
containsmetadata on the
Model
which includes domain decomposition information which isrequired for halo exchange.
Domain decomposition data is produced by the domain
decomp
tool, which is thenread 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
andModelMetadataPB_test.cpp
- testgetPartitionMetadata
can correctly load metadata from a partition metadatafile into the appropriate data structure e.g.,
neighbourExtents
, etc.HaloExchangeCB_test.cpp
andHaloExchangePB_test.cpp
- check that wecan do basic halo exchange by making use of the new slicing utilities e.g.,
ModelArraySlice