Skip to content

Periodic index spaces#765

Open
streeve wants to merge 3 commits into
ECP-copa:masterfrom
streeve:periodic_index_space
Open

Periodic index spaces#765
streeve wants to merge 3 commits into
ECP-copa:masterfrom
streeve:periodic_index_space

Conversation

@streeve

@streeve streeve commented Aug 9, 2024

Copy link
Copy Markdown
Member

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

streeve commented Aug 9, 2024

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Contributor

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

streeve commented Aug 20, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Contributor

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

streeve commented Oct 9, 2024

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Contributor

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.

@HahsFilip

Copy link
Copy Markdown

Has there been any progress on this PR?

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

3 participants