Skip to content

Ownership for self-bordering blocks - #1445

Draft
c-prather wants to merge 1 commit into
developfrom
c-prather/fix-oneblock
Draft

Ownership for self-bordering blocks#1445
c-prather wants to merge 1 commit into
developfrom
c-prather/fix-oneblock

Conversation

@c-prather

@c-prather c-prather commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR Summary

This fixes an issue in very small periodic forests with very particular synchronization needs, in particular where a block borders itself. We track when a block borders itself on the left, and flag that the block does not own those left edges, so they are always synchronized from the right.

Generally this bug actually shouldn't be an issue, at least for codes which maintain binary similarity for themselves between operations performed on the right and left faces of a single periodic block. However, KHARMA doesn't, and further relies on having a single consistent EMF at those faces in order to preserve magnetic field divergence. The result is a divergence which climbs unless this patch is applied.

I'm not sure this is the best way to do it, but it's gotten the job done downstream. In particular, I have no idea how it interacts with multiple forests, which I think is theoretically possible (that is, having a forest/base block border itself in a periodic direction, in a simulation with multiple forests not all of which share that configuration) but I'm not sure how to construct such a geometry, let alone whether it will happen for any practical setup.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • Any contribution that was created or modified with the assistance of generative AI is disclosed here and in code following the guidelines
  • (@lanl.gov employees) Update copyright on changed files

@Yurlungur
Yurlungur requested a review from lroberts36 August 28, 2026 18:24
block_ownership_t
DetermineOwnership(const LogicalLocation &main_block,
const std::vector<forest::NeighborLocation> &allowed_neighbors,
const std::array<bool, 3> self_border_block,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to actually pass this in or can we just check if the main_block logical location is in allowed_neighbors?

Comment on lines +89 to +92
} else if (main_block.level() == 0 && main_block == n.global_loc &&
self_border_left(ox1, ox2, ox3)) {
main_owns(ox1, ox2, ox3) = false;
break;

@lroberts36 lroberts36 Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, I see you are making the comparison here. So why do we need the self_border_block information at all? Shouldn't we be able to just have a tie breaker for ownership based on the offsets for the two sides in their own frames?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The main thing I am thinking here is that in a general forest, there can be all sorts of connectivity between a block and itself beyond what a periodic mesh would give.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah I get what you mean.
I was treating ownership_less_than as always comparing blocks, but we can just make it direction-aware. Testing a much smaller patch that just does that now

@Yurlungur Yurlungur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am preemptively approving now, since I think this is a small change, but I completely defer to @lroberts36

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.

3 participants