Skip to content

refactor: Refactor/cleanup partitions #3609

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

rrsettgast
Copy link
Member

This PR seeks to remove a redundant definition of DomainPartition::m_neighbors and use the one that exists in PartitionBase

@rrsettgast rrsettgast self-assigned this Mar 25, 2025
@rrsettgast rrsettgast changed the title Refactor/cleanup partitions refactor: Refactor/cleanup partitions Mar 25, 2025
@rrsettgast rrsettgast added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Mar 25, 2025
@rrsettgast rrsettgast requested a review from bd713 March 25, 2025 19:30
{
secondNeighborRanks.erase( neighbor.neighborRank() );
}

for( integer const neighborRank : secondNeighborRanks )
{
m_neighbors.emplace_back( neighborRank );
neighbors.emplace_back( neighborRank );
Copy link

Choose a reason for hiding this comment

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

We used to have two versions of m_neighbors: one in DomainPartition, one in SpatialPartition.
Despite sharing the same name, I have the impression that:

  • the SpatialPartition version used to be a "strict" list of neighbors.
  • the DomainPartition version also appended the neighbors of neighbors.

After this refactoring, I understand only one version will remain. It will be stored in SpatialPartition, and will now include the neighbors of neighbors.

If that's correct, this might conflict notably with the Particles machinery (e.g. SpatialPartition::repartitionMasterParticles) which was relying so far on the "strict" definition of neighbors.

Best case scenario: the MPM will simply iterate over more neighbors for nothing.

NB: the CI tests for MPM may still run fine as they seem to use a 1x1x1 or 2x2x1 partition... meaning they won't generate neighbors of neighbors anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants