Skip to content

Stop DG injection reading past a coarse cell's children - #5337

Open
pbrubeck wants to merge 7 commits into
mainfrom
pbrubeck/fix-dg-injection-child-count
Open

Stop DG injection reading past a coarse cell's children#5337
pbrubeck wants to merge 7 commits into
mainfrom
pbrubeck/fix-dg-injection-child-count

Conversation

@pbrubeck

@pbrubeck pbrubeck commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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_map right-pads its short rows with -1 out to the busiest coarse cell's
count. 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, detJ came out exactly zero, and its contribution vanished. Nothing that
arithmetic 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: MacroKernelBuilder now 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_insn gained an
    offsets argument so a callee can be handed one block of a caller array that holds several.
  • firedrake/mg/utils.py: new coarse_cell_child_count, a cached per-coarse-cell count. The count
    belongs to a base cell and every layer of that cell shares it, so on an extruded mesh its Dat
    hangs off the base set — op2.DataSet refuses an ExtrudedSet.
  • firedrake/mg/interface.py: passes that count as the fifth argument of the DG injection par_loop.
  • firedrake/mg/utils.py: drops 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 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, on HierarchyBase.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, 3
and 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_DG0 injected is constant on a unit domain, so it only ever checked that
the children's volumes add up. It covers the rewritten kernel, not the out-of-bounds read.

The coarse_mesh fixture moves from UnitSquareMesh(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 2
cells.

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>
Comment thread tests/firedrake/multigrid/test_adaptive_multigrid.py Outdated
Comment thread tests/firedrake/multigrid/test_adaptive_multigrid.py
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>
Comment thread firedrake/mg/utils.py Outdated
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
pbrubeck and others added 3 commits August 18, 2026 17:03
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LLM used An LLM was used in the production of this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant