Multigrid: skip re-evaluation of unrefined nodes by pbrubeck · Pull Request #5288 · firedrakeproject/firedrake

pbrubeck · 2026-07-28T19:39:47Z

Description

AI-assisted (Claude Code)

An adaptive refinement leaves most of the mesh alone. Prolongation, restriction and injection
previously treated every fine node as if the whole mesh had changed: locate the coarse cell holding
it, pull it back to the coarse reference cell, and evaluate the coarse basis there. On a cell the
refinement did not touch, that reproduces the coarse dof it started from, to within roundoff — so
this PR skips the re-evaluation and copies those nodes' values directly.

Main changes

firedrake/cython/mgimpl.pyx: preserved_points finds, for a coarse/fine plex pair produced by
adaptive refinement, which fine points are exact copies of a coarse point (the cells the refinement
left alone).
firedrake/mg/utils.py: preserved_node_sf builds the PETSc.SF pairing the coarse and fine nodes
that correspond to those preserved points (via Section.distributeSection/createSectionSF,
trimmed to owned fine nodes only); transfer_node_subset gives the complementary Subset of fine
nodes the transfer kernels must still visit; prolong_preserved_nodes/restrict_preserved_nodes
do the copy/reduce for the nodes the SF accounts for.
firedrake/mg/interface.py: prolong/restrict run their op2.par_loop only over
transfer_node_subset's nodes, then call the preserved-node copy/reduce for the rest.
Test coverage in tests/firedrake/multigrid/test_adaptive_multigrid.py: asserts the transfer
kernels are actually skipped on preserved nodes (_copied_nodes), across firedrake/netgen
meshes, square/cube, and multiple process counts.

Changes since the last review

Rebased onto Stop DG injection reading past a coarse cell's children #5337. The macro-cell map fix that was in this PR has moved there, and grew into a
real fix: the DG injection kernel now stops at each coarse cell's actual child count instead of
padding the map. @connorjward's point about if not valid.all(): causing rank divergence is
handled there too, by dropping the branch entirely rather than by filling padding.
AGENTS.md edits moved to Agent tooling: sharper prose rules, a prose checker, and an fdk helper #5338, with the ASD-STE100 rule and two prose anti-patterns.
Imports moved to the top of test_adaptive_multigrid.py and firedrake/mg/utils.py
(transfer_node_subset, coarse_cell_to_fine_node_map, complex_mode, _get_mtype). Checked
that the firedrake.halo one does not make the import circular.
Docstrings rewritten to ASD-STE100, addressing the "very unclear"/"I don't understand this"
threads on preserved_node_sf, _preserved_point_sf and the mgimpl.pyx helpers.
The FIAT dependency is gone; that commit is no longer on the branch.

Testing

test_adaptive_multigrid.py passes at 1, 2 and 4 processes (91/80/58). test_grid_transfer.py
passes at 170 / 0 failed, matching main.

pbrubeck · 2026-08-05T08:22:12Z

+        # An adaptive refinement leaves most of the mesh alone, and the nodes
+        # it preserves are copied rather than evaluated.
+        node_subset = utils.transfer_node_subset(Vc, Vf)
+        op2.par_loop(kernel, node_subset, *kernel_args)
+        utils.prolong_preserved_nodes(coarse, fine)


This is the main part of the PR

pbrubeck · 2026-08-06T09:22:33Z

+        # cell's count. op2.Map cannot hold the -1 that marks a slot as empty,
+        # and the macro-cell kernel reading through this map integrates over
+        # every slot alike. Point all of an empty slot's nodes at a single one


I'm not sure how accurate this is.

It is certainly confusing.

macro-cell kernel

Huh?

empty slot's nodes

unclear

Context: DG injection integrates on a macro-cell formed by all fine children, which for an adaptive mesh are not uniform, so we need a ragged array with empty slots

I am asking about op2.Map not being able to hold a negative value

connorjward · 2026-08-06T10:29:24Z

+
+def _copied_nodes(mh, V):
+    """Count the nodes of ``V`` that the transfers copy rather than evaluate."""
+    from firedrake.mg.utils import transfer_node_subset


top level import

connorjward · 2026-08-06T10:33:59Z

-        op2.par_loop(kernel, fine.node_set, *kernel_args)
+        # An adaptive refinement leaves most of the mesh alone, and the nodes
+        # it preserves are copied rather than evaluated.
+        node_subset = utils.transfer_node_subset(Vc, Vf)


Suggested change

node_subset = utils.transfer_node_subset(Vc, Vf)

changed_node_subset = utils.transfer_node_subset(Vc, Vf)

clearer?

connorjward · 2026-08-06T10:38:03Z

+        # of the cell's own children's nodes: the slot then describes a
+        # degenerate cell sitting on the patch it belongs to, whose Jacobian
+        # determinant, and so whose contribution, vanishes.
+        if not valid.all():


I've just had to fix this in pyop3. Please remove this branch because it causes rank divergence. All ranks should just do it

What do you mean? Have you fixed this bug or a different one?

connorjward · 2026-08-06T10:42:54Z

+        # cell's count. op2.Map cannot hold the -1 that marks a slot as empty,
+        # and the macro-cell kernel reading through this map integrates over
+        # every slot alike. Point all of an empty slot's nodes at a single one


It is certainly confusing.

macro-cell kernel

Huh?

empty slot's nodes

unclear

connorjward · 2026-08-06T10:46:17Z

+    if Vc.extruded or Vf.extruded:
+        # The plex of an extruded mesh is the flat base one, whose points
+        # carry a whole column of nodes that the Sections cannot tell apart.
+        return None


Why can't we do extruded?

connorjward · 2026-08-06T10:47:45Z

+        kernel accumulated from the remaining fine nodes.
+
+    """
+    from firedrake.halo import _get_mtype


top level import

connorjward · 2026-08-06T10:48:40Z

+
+
+def restrict_preserved_nodes(fine_dual, coarse_dual):
+    """Add what the nodes an adaptive refinement preserved contribute to the coarse dual.


I don't understand why we're adding instead of copying

I think we should be able to copy because the operator is the identity on these nodes.

connorjward · 2026-08-06T10:50:28Z

+        CHKERR(DMPlexGetTransitiveClosure(fine_dm.dm, fine_point[child], PETSC_TRUE,
+                                          &fine_size, &fine_closure))
+        # A cell with one child that the transform nonetheless changed would
+        # have a closure of its own shape; leave it to the transfer kernel.


I don't understand this

I think the rule to detect preserved points is more subtle. A cell might be left unrefined but still changed by the transform?

Adaptive refinement only touches part of a mesh, so most fine cells are exact copies of a coarse one. Detect those copied cells via a PETSc SF over the points an adaptive refine_sbr transform preserves, and have prolong/restrict copy their nodes' values directly instead of running them through the transfer kernel, which is both cheaper and exact where evaluation would otherwise be approximate. Also pads the macro-cell coarse-to-fine node map's empty slots (where a coarse cell has fewer fine children than the busiest one in the hierarchy) with a degenerate cell of zero measure, so the map stays rectangular without the kernel double-counting real contributions.

- Drop the temporary FIAT install step now that fiat#267 is merged. - preserved_points computes nfine/ncoarse via num_owned_cells instead of taking nfine as an argument, asserting the passed-in array shape against it rather than trusting it as the source of truth.

Follow ASD-STE100: short sentences, one idea each, active voice, and the subject named up front rather than buried in a relative clause. The AGENTS.md rules this follows are in #5338. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pbrubeck added the base:main Run this PR using a main (dev) build label Jul 28, 2026

pbrubeck commented Jul 29, 2026

View reviewed changes

Comment thread firedrake/mg/interface.py

pbrubeck commented Jul 29, 2026

View reviewed changes

Comment thread firedrake/mg/utils.py Outdated

pbrubeck force-pushed the pbrubeck/adaptive-multigrid branch from fac303d to af6c7df Compare July 30, 2026 16:21

pbrubeck changed the base branch from pbrubeck/assign-submesh-restricted to pbrubeck/mg-redist July 30, 2026 16:21

This was referenced Jul 30, 2026

Redistributed Submesh and MeshHierarchy #5270

Open

Unify assign across Submesh, RestrictedFunctionSpace and RestrictedElement #5287

Open

pbrubeck changed the title ~~Multigrid: skip re-evaluation unrefined nodes~~ Multigrid: skip re-evaluation of unrefined nodes Jul 31, 2026

Base automatically changed from pbrubeck/mg-redist to main July 31, 2026 10:37

pbrubeck force-pushed the pbrubeck/adaptive-multigrid branch from af6c7df to 1a70084 Compare July 31, 2026 13:29

pbrubeck commented Jul 31, 2026

View reviewed changes

Comment thread .github/actions/install/action.yml Outdated

pbrubeck commented Aug 5, 2026

View reviewed changes

Comment thread tests/firedrake/multigrid/test_adaptive_multigrid.py

pbrubeck commented Aug 5, 2026

View reviewed changes

Comment thread firedrake/cython/mgimpl.pyx Outdated

pbrubeck commented Aug 5, 2026

View reviewed changes

Comment thread firedrake/cython/mgimpl.pyx Outdated

pbrubeck commented Aug 5, 2026

View reviewed changes

Comment thread firedrake/mg/utils.py

pbrubeck force-pushed the pbrubeck/adaptive-multigrid branch from 646da29 to 5625a7c Compare August 5, 2026 15:54

pbrubeck requested a review from connorjward August 6, 2026 09:20

pbrubeck commented Aug 6, 2026

View reviewed changes

connorjward added the ci:complex Run the test suite in complex mode label Aug 6, 2026

connorjward requested changes Aug 6, 2026

View reviewed changes

This was referenced Aug 8, 2026

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

Open

Agent tooling: sharper prose rules, a prose checker, and an fdk helper #5338

Closed

pbrubeck and others added 3 commits August 8, 2026 13:02

pbrubeck force-pushed the pbrubeck/adaptive-multigrid branch from 1c2b268 to 994901b Compare August 8, 2026 12:15

pbrubeck changed the base branch from main to pbrubeck/fix-dg-injection-child-count August 8, 2026 12:30

	node_subset = utils.transfer_node_subset(Vc, Vf)
	changed_node_subset = utils.transfer_node_subset(Vc, Vf)



		def restrict_preserved_nodes(fine_dual, coarse_dual):
		"""Add what the nodes an adaptive refinement preserved contribute to the coarse dual.

Uh oh!

Conversation

pbrubeck commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Main changes

Changes since the last review

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pbrubeck commented Jul 28, 2026 •

edited

Loading