Correct and consolidate Form.h's entity-domain mapping - #4501
Open
jorgensd wants to merge 3 commits into
Open
Conversation
jorgensd
added this pull request to stack #4502
September 13, 2026 17:39
Form.h's entity-domain mappingForm.h's entity-domain mapping
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.
Correct and consolidate
Form.h's entity-domain mappingForm's constructor maps integration entities inthis->mesh()to thecorresponding cells of an argument's or coefficient's mesh. That mapping has
five defects, all currently masked because only facet integrals reach the code.
Fixing them removes an arbitrary restriction, and mixed-dimensional ridge
integrals fall out as a consequence.
1. Entity dimension conflated with the argument mesh's codimension
In both the argument and the coefficient block, before:
codimdescribes the argument mesh; the connectivity has to be indexed by theintegral's entity dimension. They coincide only for facet integrals, so the
code reads as if it handles the general case and does not. Any other integral
type would index the facet list with a non-facet local index.
The entity dimension is now computed once and used for the lookup:
The same conflation remains in
pack.hon theExpressionpath — out ofscope here, noted below.
2. Three copies of the
IntegralType → entity dimensionswitchIt was spelled out inside
impl::entity_permutationsinForm.h, and again infem/utils.handfem/utils.cpp. They have to agree and nothing enforced it.Replaced by one
beside the
IntegralTypeenum. It lives inForm.hrather thanutils.hbecause of an include cycle —
fem/utils.h→Function.h→assembler.h→the
assemble_*_impl.hheaders — which makes a helper defined inutils.hinvisible to the assemblers that include it.
Form.his upstream of that cycle.This accounts for the deletions in
utils.{h,cpp}. It throws on an unrecognisedtype rather than returning a plausible-looking dimension.
3. An arbitrary codimension restriction
compute_facet_domainsthrew"Codimension > 1 not supported."from a branchthat is already general: it works entirely through the passed-in adjacency, and
ridgeuses the same 2-wide(cell, local_entity)row layout asexterior_facet. Thecodim == 1/else throwpair collapses to onecodim >= 1branch, and the function is renamedcompute_entity_domainstomatch what it does. Lifting the restriction is deletion, not addition.
4. A comment asserting the opposite of what happens
The old comment claimed the local-entity column is reused "since
create_submeshpreserves the local facet index". It does not — on the submeshthat entity is the cell, so the column has no meaning. It is harmless only
because no consumer reads column 1: the vector and matrix assemblers read
entities0(f, 0), and coefficient packing takessubmdspan(entities, full_extent, 0). The comment now says that.5. A precondition the mapping never checked
compute_entity_domainsmaps an integration entity to a cell of the othermesh, so it can only express the case where the integral's entity dimension
equals that mesh's dimension. Nothing checked it.
Formnow throws naming bothdimensions.
This is not reachable from Python: FFCx rejects both mismatched combinations
while compiling the kernel, because the quadrature points have the wrong
dimension for the element.
So the check is a backstop for callers assembling a
Formdirectly in C++ withtheir own kernel, and it documents the precondition — it is not fixing a bug
users can hit. It is covered by a new Catch2 test that builds such a
Formandmatches the message, so it is not carried unexercised.
The argument and coefficient blocks were otherwise identical and are now one
shared
map_entitieshelper, reducing each call site to a single line.What this enables
With the mapping correct, a form may integrate over the ridges of a parent mesh
with an argument or coefficient on a codimension-2 submesh. The other two pieces
already exist:
adds the closure-dofs gather that pulls a submesh's coordinate dofs out of the
parent cell's
coordinate_dofsbuffer, indexed byquadrature_permutation.quadrature_permutationreal. Ridge kernels were previously handed an unconditionally-zero
permutation, so that gather would have read the wrong closure ordering on any
reflected edge.
Nothing else needed changing.
EntityMapis a pure index map with no notion ofcodimension;
create_submesh,compute_integration_domains(whoseexterior_facet/vertex/ridgepaths already share onedim-parameterisedbranch), coefficient packing and all three assemblers are already
codimension-generic.
IntegralType::vertexstill throws, with a message naming the supported types.Tests
Python —
python/test/unit/fem/test_assemble_submesh.py, 8 new cases.Nothing previously put a codim-2 submesh in a form;
test_submesh_fullcreatesone but checks only topology and geometry.
test_mixed_dom_codim_2integrates over every edge of a unit cube with data onthe edge submesh, and compares against integrating over the submesh directly:
The
gradandSpatialCoordinatecases go through the FFCx gather and sothrough the ridge permutations. The test is not vacuous: 58% of the cube's
edges are reflected relative to their parent cell's local orientation, and the
forms report
needs_facet_permutations(asserted). Degree 2 is theorientation-sensitive case — an edge's dofs are
[v0, v1, midpoint], so a wrongpermutation swaps the endpoints.
test_mixed_dom_codim_2_argumentscovers arguments rather than coefficients,for both a matrix and a vector. Both run over
GhostMode.noneandGhostMode.shared_facet, and in parallel — the "first incident cell" choices infem/utils.handmesh/utils.hare made independently and are rank-local,reconciled only by the permutation.
Two negative tests pin that the mismatched combinations are rejected, without
asserting which layer does the rejecting (see §5).
Six of these eight fail without this change, with
"Integral type not supported.".C++ —
cpp/test/fem/form.cpp, 1 new case. Builds aFormdirectly with adummy kernel, a ridge integral and a facet submesh, and uses
CHECK_THROWS_WITHso it pins §5's message rather than any exception.
A release-notes entry is added under
v0.12.0 (draft).Verification
fem/andmesh/.mpirun -n 3.clang-format,ruff checkandruff format --checkclean.Before merging
.github/workflows/fenicsx-refs.envcurrently pinsffcx_reftodokken/manifold-fixesso CI can run thegradandSpatialCoordinatecases.This is a temporary commit and must be reverted to
mainonceFEniCS/ffcx#879 has merged.
Required first, in order:
compilation with geometry tables (
dokken/point-coordinate-tables). Removesthe
assert ttype != "ones"that blocked point-mesh coordinate elements.Not functionally required by this PR's tests — a tet parent with an
interval submesh has a P1 coordinate element, so the all-ones case never
arises. It is in the chain only because
FEniCS/ffcx#879 carries it.
element access of codim >=1 assembly (
dokken/manifold-fixes). The realfunctional dependency; without it the
grad(g)·grad(g)andSpatialCoordinatecases cannot compile.ridge-permutations PR. The base this is stacked on, not a sibling.
Out of scope
vertexintegrals — same pattern, and FFCx would need the0-dimensional quadrature case first.
Expressionpath inpack.h,carrying the same conflation plus a parent-to-submesh-only restriction. It is
already codim-generic and correct for codim 2.
get_cell_entity_pairsisstatic_assert(num_cells == 1); patch assembly is a separate feature.AI disclosure
This PR was generated with the help of CLAUDE (OPUS 5). I have reviewed,
modified and tested the code.