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.

joewallwork · 2026-02-16T13:06:44Z

core/src/include/Halo.hpp

@@ -52,11 +54,36 @@ class Halo {

    /*!
     * @brief Constructs a halo object from DGVector


Suggested change

* @brief Constructs a halo object from DGVector

* @brief Constructs a halo object from DGVectorHolder

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?

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.

joewallwork · 2026-02-16T13:33:48Z

core/src/include/Halo.hpp

+        for (size_t comp = 0; comp < m_numComps; ++comp) {
+            Eigen::Map<Eigen::ArrayXXd, 0, Eigen::InnerStride<Eigen::Dynamic>> source_map(
+                source.col(comp).data(), m_Nx, m_Ny, Eigen::InnerStride<>(m_numComps));
+            Eigen::Map<Eigen::ArrayXXd> buffer_map(send[comp].data(), buffer_len, nCells);
+            for (auto edge : edges) {


This code could benefit from some comments as I'm struggling to understand what it does. numComps is the N value in the template constructor, right?

joewallwork · 2026-02-16T13:34:25Z

core/src/include/Halo.hpp

+    /**
+     * @brief Calculate which edge the send position sendPos would fall under in the send buffer
+     *
+     */
+    Edge edgeFromSendPos(int sendPos, int fromRank)


Missing @params etc in docstring

joewallwork · 2026-02-16T13:42:57Z

core/src/include/Halo.hpp

+            count = 1;
+            disp = disp + sendEdge;
+            recvOffset = recvOffset + 4;
+            // this is messy


Is this a TODO to do it differently?

joewallwork · 2026-02-16T14:02:17Z

core/src/include/Halo.hpp

+            if (metadata.cornerRanks[corner].size() > 0) {
+                // non-periodic case
+                hasCorner = true;
+                isPeriodic = false;
+            } else if (metadata.cornerRanksPeriodic[corner].size() > 0) {
+                // periodic case
+                hasCorner = true;
+                isPeriodic = true;
+            }


Presumably it's impossible for both conditions to be true at the same time? Or would it be useful to include a failure mode in case it happens due to a bug?

joewallwork · 2026-02-16T14:03:49Z

core/src/include/Halo.hpp

+        }
+    }
+
+    template <typename T> void populateTarget(T& target)


Please add a docstring.

joewallwork · 2026-02-16T14:06:14Z

core/src/ModelMetadata.cpp


    readNeighbourData(ncFile);

+    // cornerHaloRecv doesn't need to be read because it can be easily calculated.


I guess there's an assumption (probably right) that a small amount of computation is cheaper than a small amount of I/O?

joewallwork · 2026-02-16T14:08:57Z

core/test/HaloExchangeCB_test.cpp

Nice to reduce the amount of numbers hard-coded in code here, well done!

Thanduriel

Here is my feedback.

Thanduriel · 2026-02-18T14:29:13Z

core/src/include/ModelMetadata.hpp

    neighbourArray neighbourHaloSendPeriodic;
    neighbourArray neighbourHaloRecvPeriodic;
+
+    typedef std::array<std::vector<int>, N_CORNER> cornerArray;


Types should be in CamelCase.

using CornerArray = std::array<std::vector<int>, N_CORNER>;

Thanduriel · 2026-02-18T14:31:31Z

core/src/include/ModelMetadata.hpp

+    std::array<std::string, N_CORNER> cornerNames
+        = { "top_left", "top_right", "bottom_right", "bottom_left" };
+
    typedef std::array<std::vector<int>, N_EDGE> neighbourArray;


Types should be in CamelCase. Prefer using over typedef (see CppCoreGuidlines).

using NeighbourArray = std::array<std::vector<int>, N_EDGE>;

Thanduriel · 2026-02-18T14:33:14Z

core/src/include/ModelMetadata.hpp

+     * @brief Gets the extents of the grid in the X direction for all ranks.
+     * @return A vector containing the number of grid points in X for each rank.
+     */
+    std::vector<int> getRankExtentsX() const;


Return by const& to prevent unnecessary copies.

Thanduriel · 2026-02-18T14:33:39Z

core/src/include/ModelMetadata.hpp

+     * @brief Gets the extents of the grid in the Y direction for all ranks.
+     * @return A vector containing the number of grid points in Y for each rank.
+     */
+    std::vector<int> getRankExtentsY() const;


Return by const& to prevent unnecessary copies.

Thanduriel · 2026-02-18T14:38:48Z

core/src/include/Halo.hpp

+        int fromRank;
+        size_t disp;


These variables could be defined closer to where they are used, i.e. in the if (hasCorner) branch.

Thanduriel · 2026-02-18T14:53:34Z

core/src/include/Halo.hpp

+                    auto buffer_len = recvBufferSize / CGdegree;
+                    for (size_t comp = 0; comp < m_numComps; ++comp) {
+                        Eigen::Map<Eigen::ArrayXXd> rmap(recv[comp].data(), buffer_len, nCells);
+                        auto offset = buffer_len - (4 - corner) * nCells;


Where does this 4 come from? Is it just Corner::N_CORNER?

Thanduriel · 2026-02-18T15:01:13Z

core/src/ModelMetadata.cpp

+std::vector<int> ModelMetadata::getRankExtentsX() const { return rankExtentsX; }
+std::vector<int> ModelMetadata::getRankExtentsY() const { return rankExtentsY; }


Suggested change

std::vector<int> ModelMetadata::getRankExtentsX() const { return rankExtentsX; }

std::vector<int> ModelMetadata::getRankExtentsY() const { return rankExtentsY; }

const std::vector<int>& ModelMetadata::getRankExtentsX() const { return rankExtentsX; }

const std::vector<int>& ModelMetadata::getRankExtentsY() const { return rankExtentsY; }

Thanduriel · 2026-02-18T15:12:09Z

core/test/ModelMetadataPB_test.cpp

+constexpr ModelMetadata::Corner TOP_LEFT = ModelMetadata::Corner::TOP_LEFT;
+constexpr ModelMetadata::Corner TOP_RIGHT = ModelMetadata::Corner::TOP_RIGHT;
+constexpr ModelMetadata::Corner BOTTOM_RIGHT = ModelMetadata::Corner::BOTTOM_RIGHT;
+constexpr ModelMetadata::Corner BOTTOM_LEFT = ModelMetadata::Corner::BOTTOM_LEFT;


If the intend here is just to introduce shorter names, using would be more clear:

using enum ModelMetadata::Corner;

Thanduriel · 2026-02-18T15:23:49Z

core/src/include/Halo.hpp

+        auto extentX = metadata.getRankExtentsX()[fromRank];
+        auto extentY = metadata.getRankExtentsY()[fromRank];


I think it would be better to not use auto if the expression has a simple type. Here the type is just int while in many other places the standard size type size_t is used instead. The distinction between signed and unsigned ints is important because the following conditions would not work with an unsigned type.

Thanduriel · 2026-02-18T15:29:21Z

core/src/include/Halo.hpp

+            // TODO this is too big. Should only be 2 x CGdegree * haloWidth (extentX + extentY)
+            auto fromRankSendBufferSize = 2 * haloWidth * CGdegree * (extentX + extentY);


Is this todo outdated? The line below looks just like suggested.

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

		@@ -52,11 +54,36 @@ class Halo {

		/*!
		* @brief Constructs a halo object from DGVector


		readNeighbourData(ncFile);

		// cornerHaloRecv doesn't need to be read because it can be easily calculated.

		std::vector<int> ModelMetadata::getRankExtentsX() const { return rankExtentsX; }
		std::vector<int> ModelMetadata::getRankExtentsY() const { return rankExtentsY; }

		auto extentX = metadata.getRankExtentsX()[fromRank];
		auto extentY = metadata.getRankExtentsY()[fromRank];

		// TODO this is too big. Should only be 2 x CGdegree * haloWidth (extentX + extentY)
		auto fromRankSendBufferSize = 2 * haloWidth * CGdegree * (extentX + extentY);

Comments

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!

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!

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!

Thanduriel left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thanduriel Feb 18, 2026 • 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!

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`

Thanduriel Feb 18, 2026 •

edited

Loading