Skip to content

fix(petsc,mpi): align the global-index exchange instead of trusting both sides to agree - #495

Open
gouarin wants to merge 1 commit into
hpc-maths:mainfrom
gouarin:petsc-mpi-numbering-exchange
Open

fix(petsc,mpi): align the global-index exchange instead of trusting both sides to agree#495
gouarin wants to merge 1 commit into
hpc-maths:mainfrom
gouarin:petsc-mpi-numbering-exchange

Conversation

@gouarin

@gouarin gouarin commented Jul 27, 2026

Copy link
Copy Markdown
Contributor
  • I have installed pre-commit locally and use it to validate my commits.
  • The PR title follows the conventional commits convention.
  • This new PR is documented.
  • This new PR is tested.

Independent of the boundary-machinery rewrite and based on main, but found by it: #493's wider
ghost band turns this into a hard petsc error. #493 needs it.

Description

The exchange that gives a ghost its petsc global index is positional: the n-th value received
is the n-th value the neighbour sent. Both sides walked their own version of the sequence:

  • the sender pushed a value only for the cells it owned; the receiver read one only for the
    cells it believed the neighbour owned. Those are two ranks' opinions of ownership, and where
    they differ - which is what compute_cell_ownership's correction passes exist to repair, and
    cannot always - the sequences drift and every later ghost is handed another cell's global index;
  • the level range came from the sender's own mesh (min_level..max_level) on one side and from
    0..max_level on the other, and the intersection was built with the operands in the opposite
    order on each side.

The corruption is silent: a shifted index is a valid-looking index in another rank's range, so
nothing complains until petsc refuses a column it was not preallocated for, one rank's worth of
rows away from the row - New nonzero at (455,13367), (456,13368), … which is the trail I
followed here.

Now: one value per shared unknown whether owned or not (UNSET for what the sender does not own),
a level range derived from both meshes, and an operand order derived from the rank numbers. The
sequences cannot drift, and a real ownership disagreement is reported instead of being turned into
a wrong index.

has_duplicates, the check that catches exactly this class of defect, was O(n^2) - which is why
it could only ever live inside an assert, and asserts are compiled out of every build CI runs.
Sorting a copy makes it O(n log n), so keeping it out of release builds becomes a choice rather
than a necessity.

Related issue

None. Found while closing the MPI gap of #493.

How has this been tested?

finite-volume-heat --init-sol crenel -pc_type lu --Tf 0.02 --min-level 3 --max-level 8, built
with MPI + PETSc and with asserts on (-DCMAKE_CXX_FLAGS_RELEASE=-O1, since the guard is an
assert):

1 rank 2 3 4
main ok ok duplicate indices ok
main + this PR ok ok duplicate indices ok
#493's wider ghost band, without this PR ok ok fails fails
that band, with this PR ok ok fails ok

So this repairs the case the wider band exposed, and it changes nothing on main - no regression,
and no claim beyond what is measured.

It leaves a pre-existing defect standing, and that is worth stating plainly: at 3 ranks the
mapping already has duplicate global indices on main today. It is invisible in CI because the
guard is an assert, and it is not the exchange - the exchange is now provably aligned. It deserves
its own investigation, most likely into why the ownership correction passes do not converge for
that partition (ci.yml already documents the neighbouring symptom next to a disabled 3-rank
test: "multiple ranks own the same ghost cell, which is not supported").

Code of Conduct

  • I agree to follow this project's Code of Conduct

…oth sides to agree

The exchange that gives a ghost its petsc global index is positional: the n-th value received
is the n-th value the neighbour sent. Both sides walked their own version of the sequence:

- the sender pushed a value only for the cells **it** owned, the receiver read one only for the
  cells it believed **the neighbour** owned. Those are two ranks' opinions of ownership, and
  where they differ - which is exactly what `compute_cell_ownership`'s correction passes exist
  to repair, and cannot always - the sequences drift and every later ghost is handed another
  cell's global index;
- the level range came from the sender's own mesh (`min_level..max_level`) on one side and from
  `0..max_level` on the other, and the intersection was built with the operands in the opposite
  order on each side.

The corruption is silent. A shifted index is a valid-looking one in another rank's range, so
nothing complains until petsc refuses a column it was not preallocated for - one rank's worth of
rows away from the row, which is the signature I chased.

Now: one value per shared unknown whether owned or not (UNSET for what the sender does not own),
a level range derived from both meshes, and an operand order derived from the rank numbers. The
sequences cannot drift, and an actual ownership disagreement is reported instead of being turned
into a wrong index.

`has_duplicates`, the check that catches this class of defect, was O(n^2), which is why it could
only live inside an `assert` - and asserts are compiled out of every build CI runs. Sorting a
copy makes it O(n log n), so it is now affordable enough that keeping it out of release builds is
a choice rather than a necessity.

Measured, `finite-volume-heat --init-sol crenel -pc_type lu --min-level 3 --max-level 8`, with
asserts **on** (`-DCMAKE_CXX_FLAGS_RELEASE=-O1`):

| | 1 rank | 2 | 3 | 4 |
|---|---|---|---|---|
| main | ok | ok | **duplicate indices** | ok |
| main + this commit | ok | ok | **duplicate indices** | ok |
| with the boundary rewrite's wider ghost band, before this commit | ok | ok | fails | **fails** |
| with that band, after | ok | ok | fails | **ok** |

So this repairs the case the wider band exposed, and it leaves a **pre-existing** defect standing:
at 3 ranks the mapping already has duplicate global indices on main. That one is invisible in CI
because the guard is an assert, and it deserves its own investigation - the exchange is no longer
where it comes from.
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@gouarin

gouarin commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

The pre-existing 3-rank defect this PR deliberately leaves standing is now #496.

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.

1 participant