Skip to content

Halo add corners#1005

Merged
TomMelt merged 20 commits into
developfrom
halo-add-corners
Mar 16, 2026
Merged

Halo add corners#1005
TomMelt merged 20 commits into
developfrom
halo-add-corners

Conversation

@TomMelt
Copy link
Copy Markdown
Contributor

@TomMelt TomMelt commented Dec 16, 2025 •
edited
Loading

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

@TomMelt TomMelt self-assigned this Dec 16, 2025
@TomMelt TomMelt added the ICCS Tasks or reviews for the ICCS team label Dec 16, 2025
@TomMelt TomMelt changed the base branch from halo-dynamics to develop February 16, 2026 08:50
@TomMelt TomMelt added the enhancement New feature or request label Feb 16, 2026
joewallwork
joewallwork reviewed Feb 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@joewallwork joewallwork 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 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.

Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp
Comment thread core/src/include/Halo.hpp
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/ModelMetadata.cpp
Comment thread core/test/HaloExchangeCB_test.cpp
@TomMelt TomMelt mentioned this pull request Feb 16, 2026
Merged
Thanduriel
Thanduriel requested changes Feb 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Thanduriel Thanduriel left a comment

Choose a reason for hiding this comment

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

Here is my feedback.

Comment thread core/src/include/ModelMetadata.hpp Outdated
Comment thread core/src/include/ModelMetadata.hpp Outdated
Comment thread core/src/include/ModelMetadata.hpp Outdated
Comment thread core/src/include/ModelMetadata.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/ModelMetadata.cpp Outdated
Comment thread core/test/ModelMetadataPB_test.cpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
Comment thread core/src/include/Halo.hpp Outdated
@TomMelt TomMelt force-pushed the halo-add-corners branch from 6af5840 to eb26a81 Compare March 4, 2026 10:32
@TomMelt 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
@TomMelt
[skip ci] chore: replace auto with int for extents
23e3ca9
@TomMelt
[skip ci] chore: remove old comment
eeeecf0
@TomMelt
chore: split Halo class into header and implementation files
c213634 
- 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)
@TomMelt TomMelt force-pushed the halo-add-corners branch from e4d3e65 to c213634 Compare March 6, 2026 11:31
Thanduriel
Thanduriel approved these changes Mar 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Thanduriel Thanduriel left a comment

Choose a reason for hiding this comment

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

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
joewallwork approved these changes Mar 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@joewallwork joewallwork left a comment

Choose a reason for hiding this comment

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

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

Comment thread core/src/include/Halo.hpp
Comment thread core/src/include/Halo.hpp
Comment thread core/src/Halo.cpp
Comment thread core/src/Halo.cpp Outdated
@TomMelt TomMelt merged commit 2165b00 into develop Mar 16, 2026
9 checks passed
@TomMelt TomMelt deleted the halo-add-corners branch March 16, 2026 10:28
@TomMelt TomMelt restored the halo-add-corners branch March 16, 2026 10:32
@TomMelt TomMelt deleted the halo-add-corners branch March 16, 2026 10:35
This was referenced Mar 16, 2026
Closed
Merged
TomMelt added a commit that referenced this pull request Mar 23, 2026
@TomMelt
Fix Halo constructor for DGVectorHolder (#1065)
f53d8c6 
# Fix `Halo` constructor for `DGVectorHolder`

Fixes #1064 

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

---
# Change Description

After simplifying #1005 I accidentally introduced a bug by making the
`DGVectorHolder` constructor directly call the `DGVector` one i.e.,

https://github.com/nextsimhub/nextsimdg/blob/2165b00b858f7c15139b8c5a3ed581c942a568d8/core/src/include/Halo.hpp#L59

This is incorrect and leads to a segmentation fault.

I have now modified the constructor to use the Member Initializer List,
which will ensure the object is stored correctly

https://github.com/nextsimhub/nextsimdg/blob/6f337fb12867865fb1a1842c64f8949098b6395b/core/src/include/Halo.hpp#L60-L64


I added a test to check it works correctly, and I have fixed a small
typo in the naming of `initializeHaloMetadata` (which was
`intializeHaloMetadata` before)

---
# Test Description

I added `MPI_TEST_CASE("DGVectorHolder", 3)` to
`HaloExchangeCB_test.cpp` to check that halo exchange works correctly
for `DGVectorHolder`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Thanduriel Thanduriel Thanduriel approved these changes

@joewallwork joewallwork joewallwork approved these changes

@timspainNERSC timspainNERSC Awaiting requested review from timspainNERSC

@niravshah241 niravshah241 Awaiting requested review from niravshah241

@einola einola Awaiting requested review from einola

Assignees

@TomMelt TomMelt

Labels

enhancement New feature or request ICCS Tasks or reviews for the ICCS team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TomMelt @Thanduriel @joewallwork