Skip to content
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

Periodic index spaces #765

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

streeve
Copy link
Member

@streeve streeve commented Aug 9, 2024

Add local grid index space generation for periodic boundaries that matches the current boundary index space concept. This completes all current boundary types:

  • System boundary (non-periodic): boundaryIndexSpace
  • Periodic "system boundary": periodicIndexSpace
  • MPI boundary (not a system boundary): sharedIndexSpace

TODO: add tests to avoid issues similar to #615 due to assumed "equivalent" boundary spaces

Closes #762

@streeve streeve self-assigned this Aug 9, 2024
@streeve
Copy link
Member Author

streeve commented Aug 9, 2024

@JStewart28 I still need to add tests, but wanted to push this and make sure this was in the right direction

@streeve streeve force-pushed the periodic_index_space branch 2 times, most recently from 161c00e to 2775e18 Compare August 9, 2024 19:40
@JStewart28
Copy link

sharedIndexSpace and boundaryIndexSpace look good. Question about periodicIndexSpace: If the offset is in a shared index space along a periodic boundary, boundaryIndexSpace returns, for example, for i min/max and j min/max: (0, 2), (9, 12), and periodicIndexSpace returns (0, 2), (9, 11) for the same offset. Why is the upper bound for periodicIndexSpace one lower?

@streeve streeve force-pushed the periodic_index_space branch from 2775e18 to 0c7aea3 Compare August 20, 2024 15:10
@streeve
Copy link
Member Author

streeve commented Aug 20, 2024

sharedIndexSpace and boundaryIndexSpace look good. Question about periodicIndexSpace: If the offset is in a shared index space along a periodic boundary, boundaryIndexSpace returns, for example, for i min/max and j min/max: (0, 2), (9, 12), and periodicIndexSpace returns (0, 2), (9, 11) for the same offset. Why is the upper bound for periodicIndexSpace one lower?

I found a mistake here - initially I wrote returned the boundaryIndexSpace, but by definition the periodicIndexSpace is just a sharedIndexSpace which happens to be on a boundary (it's shared with a neighbor rank). But I'm confused on your comparison (which may or may not be because of my previous mistake): isn't any given boundary either a periodic or non-periodic, where one of the two index spaces is empty?

@streeve streeve requested a review from kwitaechong August 29, 2024 18:31
@JStewart28
Copy link

sharedIndexSpace and boundaryIndexSpace look good. Question about periodicIndexSpace: If the offset is in a shared index space along a periodic boundary, boundaryIndexSpace returns, for example, for i min/max and j min/max: (0, 2), (9, 12), and periodicIndexSpace returns (0, 2), (9, 11) for the same offset. Why is the upper bound for periodicIndexSpace one lower?

I found a mistake here - initially I wrote returned the boundaryIndexSpace, but by definition the periodicIndexSpace is just a sharedIndexSpace which happens to be on a boundary (it's shared with a neighbor rank). But I'm confused on your comparison (which may or may not be because of my previous mistake): isn't any given boundary either a periodic or non-periodic, where one of the two index spaces is empty?

What I mean is for the same halo distance - in this case, 2 cells - and on a periodic boundary, in a periodicIndexSpace the j indices are 9 and 10, but in a sharedIndexSpace the j indices are 9, 10, and 11. sharedIndexSpace has the behavior we intend. Shouldn't the ghost index space be the same size for shared, periodic spaces whether or not they are on only an MPI boundary or an MPI+periodic boundary? Please correct me if I am missing something.

@streeve streeve force-pushed the periodic_index_space branch from 0c7aea3 to fab6489 Compare October 9, 2024 19:32
@streeve
Copy link
Member Author

streeve commented Oct 9, 2024

@JStewart28 I finally got back to this - I fixed one more issue. Let me know if you still see any problems, but the case I'm comparing seems to match (in the tutorial I modified here)

@JStewart28
Copy link

Still trying to work this out. In Beatnik, we have the following code to give us the periodic index space for 2D partitioning:

template <class LocalGridType, class DecompositionType, class EntityType>
Cabana::Grid::IndexSpace<2>
periodicIndexSpace(LocalGridType local_grid, DecompositionType dt, EntityType et, std::array<int, 2> dir)
{
    auto & global_grid = local_grid->globalGrid();
    for ( int d = 0; d < 2; d++ ) {
        if ((dir[d] == -1 && global_grid.onLowBoundary(d))
            || (dir[d] == 1 && global_grid.onHighBoundary(d))) {
            return local_grid->sharedIndexSpace(dt, et, dir);
        }
    }
    std::array<long, 2> zero_size;
    for ( std::size_t d = 0; d < 2; ++d )
        zero_size[d] = 0;
    return Cabana::Grid::IndexSpace<2>( zero_size, zero_size );
}

Which I'm hoping becomes a 1-1 substitution for local_grid->periodicIndexSpace, but something is still off. I added this function in as another example in example/grid_tutorial/06_local_grid/local_grid_example.cpp, and sometimes the output matches boundaryIndexSpace and sometimes it matches periodicIndexSpace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create functions in the local grid to get the index space of cells on periodic or free boundaries
2 participants