Skip to content

Conversation

@TomMelt
Copy link
Contributor

@TomMelt TomMelt commented Dec 2, 2025

This PR fixes corner neighbour metadata for periodic boundaries

Key Changes

  • Fixed haloCornerBufferPositions() logic to correctly handle periodic boundary conditions
  • Updated corner neighbour detection to properly identify neighbours across periodic boundaries
  • Added comprehensive tests for halo buffer positions and corner neighbour detection
  • Updated reference test data for integration tests
  • Improved documentation with better comments and docstrings (but more to come in cosmetic changes to domain_decomp #80)

Tests

  • test_haloBufferPositions.cpp
  • test_haloCornerBufferPositions.cpp
  • test_is_corner_neighbour.cpp
  • test_is_neighbour.cpp

TODO:

  • add unit tests for haloCornerBufferPositions
  • add unit tests for haloBufferPositions functions @niravshah241 is working on
  • rename haloBufferPositions to haloEdgeBufferPositions (this will be handled in a following PR -- see issue cosmetic changes to domain_decomp #80 )
  • fix haloCornerBufferPositions for periodic corner neighbours (@niravshah241 is double checking)
  • @niravshah241 is investigating why the test_halo_corner_start test is failing -> See https://github.com/
    Fix corner starts #78/commits/b8ef5f59ec36814b6029fe174bc70a37ba888581
  • @niravshah241 to resolve merge conflict
  • Add test diagrams in repository for easier reference later @niravshah241
  • check all edge metadata in haloBufferPositions (i.e., use same approach as haloCornerBufferPositions)

@TomMelt TomMelt requested a review from niravshah241 December 2, 2025 15:45
@TomMelt TomMelt self-assigned this Dec 2, 2025
@TomMelt TomMelt added the ICCS label Dec 2, 2025
@TomMelt TomMelt marked this pull request as draft December 2, 2025 15:46
@TomMelt TomMelt changed the base branch from main to add-periodic-corner-nbrs December 2, 2025 15:46
Copy link

@niravshah241 niravshah241 left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Some quick formulae checks and making private members to public in partition.hpp have been suggested.

The unit tests for is_corner_neighbour and haloCornerBufferPositions (previously halo_corner_start) have been written in the branch add-periodic-corner-nbrs. (It will need some changes depending upon final function calls and index changes.)

@TomMelt TomMelt force-pushed the fix-corner-starts branch from 0e4179e to 66cd9d5 Compare January 8, 2026 09:11
@niravshah241
Copy link

niravshah241 commented Jan 12, 2026

Tests are added to this branch. The tests in test_halo_corner_start nonperiodic cases are fixed now b8ef5f5 .

@niravshah241
Copy link

niravshah241 commented Jan 20, 2026

Tests related to is_neighbour and haloBufferPositions added. There is difference in how the neighbours are considered in two functions. Given 2 subdomains d1 and d2, the RIGHT in is_neighbour means d2 is RIGHT of d1 but in haloBufferPositions, d1 is considered RIGHT of d2. Also, for LEFT, BOTTOM and TOP .

In addition the images for tests have now been added in img folder.

@TomMelt
Copy link
Contributor Author

TomMelt commented Jan 22, 2026

Tests related to is_neighbour and haloBufferPositions added. There is difference in how the neighbours are considered in two functions. Given 2 subdomains d1 and d2, the RIGHT in is_neighbour means d2 is RIGHT of d1 but in haloBufferPositions, d1 is considered RIGHT of d2. Also, for LEFT, BOTTOM and TOP .

In addition the images for tests have now been added in img folder.

There is no difference. I had made a typo in the doc string but the functionality is correct. This has been fixed in commit 5380006

Please remove the image from the img folder. If we want to upload an image it should be svg and have more description. FWIW we can see the domain layout looking at the partition_mask files.

@TomMelt TomMelt marked this pull request as ready for review January 22, 2026 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants