Stop DG injection reading past a coarse cell's children - #5337
Open
pbrubeck wants to merge 7 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>
Move the -1 padding rule onto HierarchyBase.coarse_to_fine_cells, where the array itself is documented, and shorten the three places that re-derived it. Add type hints to coarse_cell_child_count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GRZG3iuNzgXQWGiuzrjKQo
This was referenced Aug 29, 2026
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.
Stack
PR 1 of 4: this PR (
fix-dg-injection-child-count) -> #5288 (adaptive-multigrid) -> #5270 (mesh-redistribution) -> #5287 (assign-submesh-restricted).Description
AI-assisted (Claude Code)
Adaptive refinement gives coarse cells different numbers of 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, so the kernel read two doubles from before the start of the
fine coordinate array.
The result was still correct. A padded block's vertices all read one address, so the block described
a degenerate cell,
detJcame out exactly zero, and its contribution vanished. Nothing thatarithmetic can see is wrong, on any platform we can test, so this PR adds no test that fails
without it: only a memory checker such as ASan or valgrind observes the read.
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/mesh.py: the -1 padding rule is stated once, onHierarchyBase.coarse_to_fine_cells,rather than re-derived by each of its three consumers.
Tests
tests/firedrake/multigrid/test_adaptive_multigrid.py::test_dg_injection_conserves_mass, at 1, 2, 3and 4 processes. Injection into a DG space is a cellwise L2 projection, and every 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 the old
test_DG0injected is constant on a unit domain, so it only ever checked thatthe children's volumes add up. It covers the rewritten kernel, not the out-of-bounds read.
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.