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

Functionality for Indexer masking #901

Merged
merged 112 commits into from
Jul 18, 2023

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Jun 29, 2023

PR Summary

This PR add the functionality for determining ownership based masks for boundary communication and implements those masks in the boundary communication routines. The basic problem this PR tries to solve is how to synchronize elements that are shared between blocks (e.g. the edge of a zone that is on the edge of a block may be shared by up to four blocks and those blocks may be on different refinement levels). Rather than let these evolve separately (as is done in Athena++, I think), one particular block is chosen to own each element and it communicates the value it has to the other blocks that share the element (potentially after restriction on a fine block). Ownership is determined first by the most refined block and then if multiple sharing blocks are at the same refinement level, the block with the maximum Morton number is chosen to own the element.

The index space for owned elements of a particular type on a given block may not be rectangular. As an example, consider a nodal field on a block that has one neighbor block on a corner that is more refined. When nodal zones are passed to a face neighbor (that is also a neighbor of the corner neighbor) and the main block has higher ownership status than the face neighbor, the set of nodes that need to be communicated have one node removed at the corner. To deal with this, all zones possibly required by a neighbor block are packed into a communication buffer but when the boundaries are set in the receiving block it is done with a mask that takes into account ownership. The logic for building these ownership masks first requires building a 3^3 array for each block describing which of its faces, edges, and nodes it owns (blocks always own their interior). This is implemented in DetermineOwnership. This can then be transformed into a 3^3 array describing the ownership of a block for face, edge, and nodal fields on each of the possible elements of the block itself. Finally, this field type ownership array can be transformed into the mask by accounting for where in the interior of the block the buffer is being filled from. These two operations are performed in GetIndexRangeMaskFromOwnership. These methods are the meat of this PR.

Additionally, some unit tests are included and everything seems to work with SMR in parthenon-mhd. I also moved the MakePackDescriptor stuff to its own header file.

A list of some stuff I checked/did:

  • Check logic for finding masks based on topological element, boundary of sending block, and sending block ownership is correct as implemented in the test code.
  • Move mask finding logic out of tests
  • Include masking information in bound info objects (assuming no ownership for now).
  • Add more complete test suite.
  • Test against parthenon-mhd for uniform mesh
  • Test against parthenon-mhd for SMR mesh

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Nice to see this. Excited to try face-centered fields with AMR out :D

src/interface/make_pack_descriptor.hpp Show resolved Hide resolved
src/mesh/logical_location.hpp Show resolved Hide resolved
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

Kudos for all that index magic.
I definitely cannot say that I am able to mentally process all the fine grained details.
I trust that if there's sth slightly off we'll track it down in various downstream codes.

src/bvals/comms/bnd_info.cpp Show resolved Hide resolved
src/bvals/comms/bnd_info.cpp Show resolved Hide resolved
src/bvals/comms/bnd_info.cpp Show resolved Hide resolved
@lroberts36
Copy link
Collaborator Author

I trust that if there's sth slightly off we'll track it down in various downstream codes.

I think that is right. I definitely spent a lot of time thinking about it and am pretty convinced things are correct, but at the end of the day I am sure I didn't think through every single edge case and I am relying on things generalizing correctly. The real proof is in it working, although it would be great if someone could think up a unit test which went through every possible case.

I also condensed the indexing stuff to a single function in the following PR adding AMR. It might be a little easier to track through all of the index logic there.

@lroberts36 lroberts36 changed the base branch from lroberts36/add-morton-numbers to develop July 18, 2023 18:09
@lroberts36 lroberts36 enabled auto-merge (squash) July 18, 2023 18:15
@lroberts36 lroberts36 merged commit 94caccd into develop Jul 18, 2023
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.

4 participants