Halo add corners by TomMelt · Pull Request #1005 · nextsimhub/nextsimdg

TomMelt · 2025-12-16T09:36:41Z

Add corner neighbours into halo exchange logic

Task List

Linked an issue above that captures the requirements of this PR
Defined the tests that specify a complete and functioning change
Implemented the source code change that satisfies the tests
Commented all code so that it can be understood without additional context
No new warnings are generated or they are mentioned below
The documentation has been updated (or an issue has been created to do so)
Relevant labels (e.g., enhancement, bug) have been applied to this PR
This change conforms to the conventions described in the README

Change Description

This PR has the following key changes:

adds support for corner neighbours to the existing halo exchange logic (which previously just exchanged edges)
add support for CGVector and DGVectorHolder exchange (this is required for corners and edges)
updates metadata tests and closed-boundary (CB) halo test to work with corner neighbours
removes the Periodic-Boundary (PB) halo exchange test for now, this will be addressed in a subsequent PR Halo add pb test for halo exchange #1044

Test Description

`HaloExchangePB_test.cpp`

HaloExchangeCB_test.cpp creates test data for all the array types:

HField
VertexField
DGField
DGVector
CGVector

I modified the previous version, so that the model data are created in such a way that the value of each cell can be calculated based on its indices. Therefore we can check after exchange that all of the cells have been exchanged successfully. This also now allows us to change the number of ranks arbitrarily (assuming you also provide the appropriate partition_metadata file.

`ModelMetadataCB_test.cpp` and `ModelMetadataPB_test.cpp`

Small modifications have been made to the periodic and closed boundary ModelMetadata tests.

The changes now add the additional corner neighbour metadata and check that it is ready correctly.

`partition_metadata_3_cb.cdl` and `partition_metadata_3_pb.cdl`

These files are now generated at compile time by running the decomp tool on a new file halo_test_mask.cdl. This is in line with similar changes introduced by @joewallwork on the XIOS tests.

Other Details

domain_decop git commit has now been bumped to the latest version on main. This version of the decomp tool contains the corner neighbour metadata.

Further work

Integrating halo exchange to nextsim is handled by PR Add Halo Exchange to Dynamics #924

joewallwork

Thanks for this @TomMelt. Here's some initial feedback. I haven't checked all of the logic but let me know if there's anything in particular you want checking.

core/src/include/Halo.hpp

joewallwork · 2026-02-16T13:10:29Z

core/src/include/Halo.hpp

+    template <int N> Halo(DGVectorHolder<N>& dgvh)
+    {
+        m_numComps = N;
+        setSpatialDims();
+        intializeHaloMetadata();
+    }
+
+    /*!
+     * @brief Constructs a halo object from DGVector
+     * @param dgv DGVector object to create halo from
     */
    template <int N> Halo(DGVector<N>& dgv)
    {
        m_numComps = N;
-        isVertex = false;
+        setSpatialDims();
+        intializeHaloMetadata();
+    }
+
+    /*!
+     * @brief Constructs a halo object from CGVector
+     * @param cgv CGVector object to create halo from
+     */
+    template <int N> Halo(CGVector<N>& cgv)
+    {
+        m_numComps = 1;
+        isCG = true;
+        CGdegree = N;
+        nCells = CGdegree;
        setSpatialDims();
        intializeHaloMetadata();


In none of these cases do the constructors depend on their argument, right? The only information that's being used is the value of N and the type. So do we actually need to provide dgv, dgvh, or cgv as arguments?

I think we do, because we need to templated N from dgv and dgvh.

You are right that Halo(DGVectorHolder<N>& dgvh) and Halo(DGVector<N>& dgv) are essentially the same. But Halo(ModelArray& ma) and Halo(CGVector<N>& cgv) set different member variables from dgv.

What I could do however, is change Halo(DGVectorHolder<N>& dgvh) to call Halo(DGVector<N>& dgv) with static_cast<DGVector<DGadvection>&>() like we do in dynamics:

nextsimdg/dynamics/src/include/BrittleCGDynamicsKernel.hpp

Lines 131 to 134 in 87dadeb

Halo halo(hice);

halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(hice));

halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(cice));

halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(damage));

I have simplified this in f1829af

joewallwork · 2026-02-16T13:31:05Z

core/src/include/Halo.hpp

There seems to be quite a lot of implementation in this header file. Might it be better to separate it out into core/src/Halo.cpp?

Agreed. Function definitions should go in a source file if possible. Exceptions are (public) template functions which have to be defined in the header and maybe one-liners.

Thanks @joewallwork and @Thanduriel . I have split the implementation as suggested and I also tidied up a bit.

Split implementation out from Halo.hpp into core/src/Halo.cpp

Moved implementation of non-templated functions to core/src/Halo.cpp

Made the following functions private:

setSpatialDims()

intializeHaloMetadata()

sendBufferPositions()

sourceToSendBuffer()

recvBufferPositions()

recvBufferToTarget()

recvBufferToTarget()

transposeCorners()

populateTarget()

See commit c213634

core/src/include/Halo.hpp

core/src/ModelMetadata.cpp

core/test/HaloExchangeCB_test.cpp

Thanduriel

Here is my feedback.

core/src/include/ModelMetadata.hpp

core/src/include/Halo.hpp

core/src/ModelMetadata.cpp

core/test/ModelMetadataPB_test.cpp

core/src/include/Halo.hpp

…eriodic

- Split implementation out from `Halo.hpp` into `core/src/Halo.cpp` - Moved implementation of non-templated functions to `core/src/Halo.cpp` - Added `Halo.cpp` to `core/src/CMakeLists.txt` - Made the following functions private: - `setSpatialDims()` - `intializeHaloMetadata()` - `sendBufferPositions()` - `sourceToSendBuffer()` - `recvBufferPositions()` - `recvBufferToTarget()` - `recvBufferToTarget()` - `transposeCorners()` - `populateTarget()` All tests pass: `ctest` (6/6 tests passed)

Thanduriel

I have one alternative suggestion for the shorter enum types but you decide if you want to make the change. From my side the code is ready for merge.

joewallwork

This is great, thanks @TomMelt. I have a few small requests but I think it's otherwise good to go!

core/src/include/Halo.hpp

joewallwork · 2026-03-09T11:58:39Z

core/src/include/Halo.hpp

-     * │ │x│x│ │ (empty) = unused data in DGVector
-     * └─┴─┴─┴─┘
-     */
    template <typename T> void exchangeHalos(T& target)


Please could you add a docstring for this function?

joewallwork · 2026-03-09T12:00:06Z

core/src/Halo.cpp

+ *
+ * Halo supports the main data structures of NextSim e.g., ModelArray, DGVector and CGVector.
+ *
+ * The halos are exchange via one-sided MPI communication using RMA.


Perhaps spell out RMA here for clarity.

joewallwork · 2026-03-09T12:01:51Z

core/src/Halo.cpp

+
+void Halo::populateRecvBuffers()
+{
+


Suggested change

TomMelt self-assigned this Dec 16, 2025

TomMelt added the ICCS Tasks or reviews for the ICCS team label Dec 16, 2025

TomMelt force-pushed the halo-dynamics branch from a15788e to 1559ff0 Compare January 6, 2026 09:16

TomMelt force-pushed the halo-add-corners branch from b12e871 to 090441d Compare January 15, 2026 12:42

TomMelt added 3 commits February 16, 2026 08:38

feat: add corner neighbours to nextsim

a7e9cb8

chore: bump commit of domain_decomp

719e530

tests: remove Halo PB test for now

bb07d04

TomMelt force-pushed the halo-add-corners branch from 67e6434 to bb07d04 Compare February 16, 2026 08:49

TomMelt changed the base branch from halo-dynamics to develop February 16, 2026 08:50

TomMelt added the enhancement New feature or request label Feb 16, 2026

TomMelt added 2 commits February 16, 2026 12:02

chore: remove unused includes

d230d64

docs: update doxygen comments in Halo.hpp

c5f36a8

TomMelt requested review from Thanduriel, einola, joewallwork, niravshah241 and timspainNERSC February 16, 2026 12:05

joewallwork reviewed Feb 16, 2026

View reviewed changes

TomMelt mentioned this pull request Feb 16, 2026

Improve error message for missing partition.nc file #1006

Open

Thanduriel requested changes Feb 18, 2026

View reviewed changes

TomMelt added 4 commits March 2, 2026 10:31

chore: simplify Halo const for DGVectorHolder

f1829af

chore: update comment in recvPositions

a8c4772

docs: explain use of Eigen::Map in populateSendBuffers

0436038

[skip ci] chore: fix docstring for edgeFromSendPos in Halo.hpp

eb26a81

TomMelt force-pushed the halo-add-corners branch from 6af5840 to eb26a81 Compare March 4, 2026 10:32

TomMelt added 5 commits March 4, 2026 11:18

[skip ci] feat: catch potential error if corner is periodic and non-p…

c030bf3

…eriodic

[skip ci] chore: add docstring for populateTarget

c76f111

[skip ci] chore: use using over typedef

a59341f

[skip ci] chore: return const& instead to prevent copies

c4210bb

[skip ci] chore: move fromRank & disp definition close to usage

bcfedc7

[skip ci] chore: replace magic number 4 with N_CORNER/N_EDGE

b693869

TomMelt force-pushed the halo-add-corners branch from b8507c9 to b693869 Compare March 5, 2026 14:58

TomMelt added 3 commits March 5, 2026 16:41

[skip ci] chore: replace auto with int for extents

23e3ca9

[skip ci] chore: remove old comment

eeeecf0

TomMelt force-pushed the halo-add-corners branch from e4d3e65 to c213634 Compare March 6, 2026 11:31

TomMelt requested review from Thanduriel and joewallwork March 6, 2026 11:40

Thanduriel approved these changes Mar 6, 2026

View reviewed changes

chore: simplify enums in ModelMetadata tests

ff0235d

joewallwork approved these changes Mar 9, 2026

View reviewed changes

	Halo halo(hice);
	halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(hice));
	halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(cice));
	halo.exchangeHalos(static_cast<DGVector<DGadvection>&>(damage));

Conversation

TomMelt commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add corner neighbours into halo exchange logic

Task List

Change Description

Test Description

HaloExchangePB_test.cpp

ModelMetadataCB_test.cpp and ModelMetadataPB_test.cpp

partition_metadata_3_cb.cdl and partition_metadata_3_pb.cdl

Other Details

Further work

Uh oh!

joewallwork left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Thanduriel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Thanduriel left a comment

Choose a reason for hiding this comment

Uh oh!

joewallwork left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TomMelt commented Dec 16, 2025 •

edited

Loading

`HaloExchangePB_test.cpp`

`ModelMetadataCB_test.cpp` and `ModelMetadataPB_test.cpp`

`partition_metadata_3_cb.cdl` and `partition_metadata_3_pb.cdl`