Stop DG injection reading past a coarse cell's children - #5337
Open
pbrubeck wants to merge 3 commits into
Open
Conversation
Adaptive refinement gives coarse cells different numbers of fine children, so coarse_cell_to_fine_node_map right-pads its short rows with -1. The DG injection kernel integrated over every slot of a row, padding included, and op2.Map applies no mask to a negative index. The kernel therefore read two doubles from before the start of the fine coordinate array. Every node slot of a padded block holds -1, so all of a block's vertices read the same address. The block described a degenerate cell, its Jacobian determinant came out as exactly zero, and the answer was right. The read was still out of bounds. Count each coarse cell's real children and stop there. The kernel now integrates one micro-cell per call, and the outer kernel calls it once per child, so the padding is never read. Also drop the rank-local `if not valid.all():` in coarse_node_to_fine_node_map. Which rows hold padding depends on the partition, so that branch let the ranks disagree. The fill it guards does nothing when no row is padded, so every rank can just run it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
commented
Aug 8, 2026
pbrubeck
commented
Aug 8, 2026
This was referenced Aug 8, 2026
coarse_to_fine_cells spans the owned coarse cells, so most halo rows of the node map hold no candidate. Measured on an adaptive hierarchy at four ranks: 1428 of 1779 halo rows for CG3 on the cube. Those rows keep the zero filler, which points them at fine node 0. Nothing reads them: poisoning every halo row of the map leaves the injected result bit-identical at two and four ranks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
commented
Aug 8, 2026
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AI-assisted (Claude Code)
Adaptive refinement gives coarse cells different numbers of fine children, so
coarse_cell_to_fine_node_mapright-pads its short rows with -1 out to the busiest coarse cell'scount. The DG injection kernel integrated over every slot of a row, padding included, and PyOP2
applies no mask to a negative map entry —
_maskreturnsNoneand nothing overrides it. The kerneltherefore read two doubles from before the start of the fine coordinate array.
The answer was nonetheless right, which is why no test caught this.
valuesis filled only atvalues[valid, :], so every node slot of a padded block keeps -1, and all of that block'svertices read the same address. The block described a degenerate cell,
abs(detJ)came out asexactly 0.0, and the contribution vanished. This is an out-of-bounds read that produces the correct
result on every platform we can test — the kind of thing ASan or valgrind flags and arithmetic never
will.
This PR counts each coarse cell's real children and stops there.
Main changes
firedrake/mg/kernels.py:MacroKernelBuildernow builds a kernel for a single micro-cell.The outer loopy kernel loops over child slots and predicates each call on
entity < nchild[0],slicing the macro arguments at
entity * per_entity_size._generate_call_insngained anoffsetsargument so a callee can be handed one block of a caller array that holds several.firedrake/mg/utils.py: newcoarse_cell_child_count, a cached per-coarse-cell count. The countbelongs to a base cell and every layer of that cell shares it, so on an extruded mesh its
Dathangs off the base set —
op2.DataSetrefuses anExtrudedSet.firedrake/mg/interface.py: passes that count as the fifth argument of the DG injection par_loop.firedrake/mg/utils.py: drops the rank-localif not valid.all():incoarse_node_to_fine_node_map. Which rows hold padding depends on the partition, so that branchlet the ranks disagree. The fill it guarded is a no-op when no row is padded, so every rank can
just run it. This is @connorjward's review point from Multigrid: skip re-evaluation of unrefined nodes #5288, and matches what he already did on
connorjward/pyop3.firedrake/mg/utils.py: docstrings for the three node-map builders, each stating what uniformrefinement does, what adaptive refinement does, and where the counterpart map lives. The three
had no docstrings at all.
Tests
Both are in
tests/firedrake/multigrid/test_adaptive_multigrid.py, at 1, 2 and 4 processes.test_dg_injection_conserves_mass— injection into a DG space is a cellwise L2 projection, andevery DG space holds the constants, so the integral of the injected function over a coarse cell
must equal the integral of the fine function over that cell's children. A random fine function
is what makes this bite: the step function
test_DG0injects is constant on a unit domain, so itonly ever checked that the children's volumes add up.
test_dg_injection_ignores_padded_children— points the padding at a real child, so that readingit would integrate that child twice. This is the test that actually distinguishes fixed from
unfixed. Forcing the loop back to the full width gives 3 failed / 3 passed: the poisoned test
catches it, the mass test cannot.
The
coarse_meshfixture moves fromUnitSquareMesh(1, 1)/UnitCubeMesh(1, 1, 1)to(4, 4)/(2, 2, 2), so that a coarse cell's child count varies widely across the mesh rather than over 2cells.
test_grid_transferis unchanged at 170 passed / 0 failed, which is the baseline onmain.The halo rows: checked, and inert
coarse_node_to_fine_node_map's empty-candidate check covers owned nodes only:A halo row with no candidate therefore keeps the zero filler, which points every one of its
candidates at fine node 0. Such rows are not rare:
coarse_to_fine_cellsspans the owned coarsecells, so most halo rows have no candidate at all. Measured on an adaptive hierarchy at four ranks,
CG3 on the cube, 1428 of 1779 halo rows are empty.
Nothing reads them. Poisoning every halo row of the map and re-injecting leaves the result
bit-identical at two and four ranks, for CG1/2/3 — injection visits owned nodes only. So this is a
trap for the next reader rather than a defect, and this PR adds a comment saying so instead of
changing the behaviour. Widening the guard to the halo rows would be wrong: it would raise on
essentially every parallel run.
That also rules it out as the cause of the CG divergence reported against #5288, which I had
suspected. That remains unexplained.