FInAT: Dual evaluation for compositions of TensorProductElement + EnrichedElement - #268
Merged
Merged
Conversation
This was referenced Jul 29, 2026
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
July 29, 2026 12:08
77a6f51 to
bdb3b8b
Compare
This was referenced Jul 29, 2026
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
July 29, 2026 18:27
bdb3b8b to
e3ada2f
Compare
pbrubeck
changed the base branch from
pbrubeck/sum-factorise
to
pbrubeck/hdivcurl-delta
July 29, 2026 18:30
pbrubeck
added a commit
to firedrakeproject/firedrake
that referenced
this pull request
Jul 29, 2026
EnrichedElement has no dual basis without it, so interpolating into the facet space of a hexahedron raises NotImplementedError, and gem cannot unconcatenate a dual basis whose blocks have already been contracted with a Cofunction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
commented
Jul 30, 2026
rckirby
reviewed
Aug 5, 2026
rckirby
left a comment
There was a problem hiding this comment.
It looks like there are a lot of tests. We're returning a lot of more complicated GEM (that gives fewer flops in the end), so I guess this wants testing in Firedrake as well?
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 18, 2026 10:03
cf129c7 to
7eacfd1
Compare
pbrubeck
added a commit
to firedrakeproject/firedrake
that referenced
this pull request
Aug 18, 2026
EnrichedElement has no dual basis without it, so interpolating into the facet space of a hexahedron raises NotImplementedError, and gem cannot unconcatenate a dual basis whose blocks have already been contracted with a Cofunction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
added a commit
to firedrakeproject/firedrake
that referenced
this pull request
Aug 18, 2026
EnrichedElement has no dual basis without it, so interpolating into the facet space of a hexahedron raises NotImplementedError, and gem cannot unconcatenate a dual basis whose blocks have already been contracted with a Cofunction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 19, 2026 14:11
7eacfd1 to
20379ad
Compare
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 24, 2026 21:15
20379ad to
a62a980
Compare
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 26, 2026 11:11
a62a980 to
d630222
Compare
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 26, 2026 11:57
d630222 to
4d52a34
Compare
find_group() collected indexed Concatenate nodes by intersecting against the free indices of the expressions, and asserted that the concatenation index was among them. But a Concatenate can only be split if the index is carried by an assignment *variable*: that is what split_variable() needs to slice the variable into blocks. Intersect against the variables' free indices instead. An index that has already been contracted away in the expression, or that belongs to another pair's variable, is now left alone rather than tripping the assertion or looping forever on a pair that cannot be split. The groups found are unchanged whenever the variables carry every concatenation index, which is the case for form compilation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Interpolating into the facet space of a hexahedron raised
"NotImplementedError: Dual basis not defined for element EnrichedElement",
and matrix-free adjoint interpolation out of a restricted space failed in
gem.unconcatenate. Both come from an element whose functionals do not all
evaluate on the same points having no way to be dual evaluated except
through dual_basis, which is a single weight tensor on a single point set.
Such an element is a direct sum. Give every element the sub_elements it is
the direct sum of -- itself, unless it is one -- and dual evaluate each on
its own points, stacking the results along the basis index the direct sum
occupies:
* an EnrichedElement is the sum of the elements it enriches;
* a TensorProductElement with a summed factor is the sum of the products
of that factor's sub-elements with the factors either side of it, since
the tensor product distributes over the direct sum;
* H(div)/H(curl), flattened and discontinuous wrappers carry the
decomposition of what they wrap.
The decomposition is one level deep and dual_evaluation recurses, so each
element concatenates along the index its own sum occupies. That matters for
a tensor product: its sub-elements own a slice of one factor's basis index
and the whole of every other, which is a sub-box of the basis multiindex
rather than a contiguous range of a flat one. Stacking on the factor's own
index is correct wherever the summed factor sits; flattening to leaves would
renumber the DoFs unless it were outermost.
Concatenating over the basis index is what unconcatenate is for: the index
stays free in the assignment, so it can be split downstream. A concatenation
over the points could not be -- points are always contracted by the sum in
dual_evaluation -- which is why the blocks are never brought onto common
points.
dual_point_set names the points of every sub-element, as a UnionPointSet, for
the callers that build a function space on them rather than contract against
them. It carries no structure across the sub-elements: contracting is the
business of each of them, and each keeps its own indices.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FiniteElementBase.dual_evaluation brings the sum outermost and delegates to a _dual_evaluation hook, so as_enriched is called from one place rather than from each element that can hold a sum. Drop the overrides in TensorProductElement and WrapperElementBase, and reduce the ones in TensorFiniteElement and EnrichedElement to the hook. Fix QuadratureElement's union branch, which unpacked a pair from the triple dual_evaluation returns, and returned a pair in turn. Firedrake builds a quadrature space on the points of the target's dual basis, so this is reached whenever that target is a direct sum; test it. Collapse restrict_hcurl and restrict_hdiv onto WrapperElementBase, and trim the docstrings to :arg: style. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 26, 2026 15:19
215aa47 to
050579b
Compare
pbrubeck
commented
Aug 26, 2026
pbrubeck
commented
Aug 26, 2026
pbrubeck
commented
Aug 26, 2026
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
pbrubeck
commented
Aug 26, 2026
pbrubeck
commented
Aug 26, 2026
A `QuadratureElement` whose rule sits on a `UnionPointSet` split the union
itself, in its own `_dual_evaluation`. That only covered the element as it
stands: a `TensorFiniteElement` wrapping it takes the weights from
`dual_basis` and calls the callable once, on the whole union. The callable
then tabulates into a `Concatenate` over the points, which is contracted
away before `unconcatenate` can split along it, and the `Concatenate`
reaches loopy:
AssertionError: cannot generate expression from <class 'gem.gem.Concatenate'>
Firedrake interpolates into a vector-valued quadrature space on the points
of the target's dual basis, so every H(div)/H(curl) grid transfer took that
path.
Register the union with `as_enriched` instead. `dual_evaluation` brings a
direct sum outermost before it evaluates, so the summands then each get
their own point set wherever the element appears, the tensor wrapper
included, and the hand-rolled split goes away.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rckirby
approved these changes
Aug 27, 2026
rckirby
left a comment
There was a problem hiding this comment.
This seems quite technical, but the tests seem to cover the particular things being added.
A tensor element couples a basis function to a value component through an identity, so it needs no rewrite of its own to reach a direct sum: dual evaluating the base element one component at a time gives the components whatever structure the base element has, a direct sum among it, and keeps the basis indices in the element's own index shape rather than the flat one a sum of tensor elements gives. The identity is contracted rather than carried into the basis indices, as a free index into a ListTensor defeats argument factorisation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fix the getattr/setattr lazy init in QuadratureElement._weights, dedupe the as_enriched summand-distribution shape into distribute_over_sum, rename find_group's free_indices to splittable_indices, replace the repeated isinstance(ps, UnionPointSet) checks with a point_sets property, restore DiscontinuousElement's dual evaluation as the _dual_evaluation hook instead of a public override, and dedupe repeated test setup. Guard EnrichedElement.dual_basis to raise NotImplementedError for a summand with its own dual_transformation, since the property has no coordinate_mapping to apply it with and would otherwise silently combine untransformed weights. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BjZ6kYK1tmLfm6wUxXwVtG
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
August 28, 2026 10:02
46e2872 to
46eef3f
Compare
A sum whose summands are not mutually nodal has no dual basis to give. Its functionals are still the block diagonal weights that dual_basis builds, but every caller contracts those into coefficients, and for a non-nodal sum that contraction is the inverse of the nodal matrix, not the identity. MINI is the case in hand: the linear basis functions are not zero at the bubble's point. dual_evaluation already refused this. dual_basis did not, so the sum advertised weights that no caller could use, and has_pointwise_dual_basis answered False only because the block diagonal carries Zero blocks rather than because the sum is non-nodal. Refuse it in dual_basis as well. dual_evaluation keeps its own guard: it contracts each summand through that summand's dual_basis and never asks self for one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NoTXWyj2fVdJTHnMDFB4k
Phase 3 of _collect_monomials summed the indices that no atomic carries without cancelling the Deltas over them first, and left the remaining factors as the product trees a nested expansion had already built, where a Delta is invisible. Such a contraction was carried out rather than cancelled, keeping an index that the Delta should have renamed away. Flatten the non-atomic factors and eliminate Deltas over every index the monomial contracts, as gem.optimise.contraction already does for coefficient evaluation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Vnd4VHtANbfmiMxSPSXSG
Phase 3 of _collect_monomials flattens the non-atomic factors before cancelling Deltas, and did so in two unsound ways. traverse_product started a fresh renamer, so a contraction it hoisted out of the rest kept an index the monomial was already summing over, and the duplicate tripped MonomialSum.add's assertion. Hand it the renamer that has already seen the monomial's own indices. The product being flattened also folds to Zero as soon as one of its factors is Zero, taking the free indices of the others with it, and sum_factorise was then left to contract an index that no factor carried. Such a monomial is zero, so recognise it and skip the contraction rather than planning one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1K5upgeZTKWj9m4tHt9Vn
pbrubeck
force-pushed
the
pbrubeck/fix/dual-enriched
branch
from
September 7, 2026 17:06
f38a79c to
8e77580
Compare
Phase 3 of _collect_monomials flattens the non-atomic factors before cancelling Deltas, and did so in two unsound ways. traverse_product descends into contractions, so flattening pulled apart the coefficient evaluations that gem.optimise.contraction had already planned and re-planned them as a single problem. An interior facet integral of two DG3 coefficients on a hexahedron then handed sum_factorise one connected contraction over eight indices -- the two facet quadrature indices and each cell's three basis indices -- which it refuses. Stop the flattening at the contractions the product already contains; the Deltas that have to be cancelled are factors of the product tree, not of those. The product being flattened also folds to Zero as soon as one of its factors is Zero, taking the free indices of the others with it, and sum_factorise was then left to contract an index that no factor carried. Such a monomial is zero, so recognise it and skip the contraction rather than planning one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F1K5upgeZTKWj9m4tHt9Vn
collect_monomials cancels Deltas over the indices contracted by a monomial, which needs the non-atomic factors flattened first so that the Deltas expansion buried inside a product are visible. Rebuilding rest from that flattened list unconditionally discards the grouping the recursion had already chosen, and sum_factorise re-associates it worse: hexahedral Q3 Helmholtz cost 2.3% more flops, and every laplacian, Helmholtz and H(div) kernel measured paid between 0.2% and 2.3%. Cancel in _cancel_deltas, which hands back rebound factors only when a Delta does cancel, and otherwise the factors it was given. Form assembly flop counts then match what they were before the cancellation was introduced, and interpolation against a Cofunction dual argument keeps its win: 1128 -> 360 flops on a degree 4 quadrilateral, 140039 -> 25000 on a degree 8 facet-restricted hexahedron, with no Delta left for the replace_delta fallback to lower into an identity matrix. Take the sum indices that reach the rest from the free indices of its factors, rather than as those the atomics do not carry. A Zero absorbs the free indices of the factors it multiplies, so the two differ exactly when the monomial is zero, and sum_factorise then contracts nothing and folds the Zero on its own. Drop the index_replacer argument to traverse_product, which stop_at makes unreachable, and pin the cancellation with a test: main carries the Delta into the contraction, and flattening alone leaves the nested one behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158RTiFnSyhgKbsbz11JMak
Both sides rewrote how phase 3 of _collect_monomials flattens the non-atomic factors before cancelling Deltas. Keep this side's flattening, which stops at the contractions the product already contains. The other side descends into them and threads a renamer through traverse_product so that the indices it hoists stay distinct, but descending re-plans the contractions gem.optimise.contraction had already planned, and sum_factorise re-associates them worse: hexahedral Q3 Helmholtz costs 2.3% more flops, and every laplacian, Helmholtz and H(div) kernel measured pays between 0.2% and 2.3%. Nothing is hoisted when the flattening stops, so the indices cannot collide and the renamer has nothing to keep apart. Take the pruning of the cancelled factors, which delta_elimination leaves wrapped in a ComponentTensor that nothing downstream is obliged to remove. Two survived per dual evaluation kernel here; none now, at no cost in flops. The Zero case both sides special-cased is already covered: this side takes the sum indices that reach the rest from the free indices of its factors, so a Zero that has absorbed them contracts nothing and folds on its own. Keep the traverse_product signature from the other side, whose renamer argument subsumes the rename_map it replaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158RTiFnSyhgKbsbz11JMak
pbrubeck
commented
Sep 9, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4zn3Z5RoNPnaVpDzqP2dg
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.
Interpolating into the facet space of a hexahedron raised
NotImplementedError: Dual basis not defined for element EnrichedElement,and matrix-free adjoint interpolation out of a restricted space failed in
gem.unconcatenate. Both come from the same gap:dual_basisis a single weighttensor on a single point set, and an element whose functionals do not all evaluate
on the same points has no such thing.
An element is a direct sum
Such an element is a direct sum, so give every element the
sub_elementsit is thedirect sum of — itself, unless it is one — and dual evaluate each on its own points:
EnrichedElementis the sum of the elements it enriches;TensorProductElementwith a summed factor is the sum of the products of thatfactor's sub-elements with the factors either side of it, since the tensor product
distributes over the direct sum;
whatever they wrap.
dual_basiskeeps its exact meaning; it is simply only defined on an element that isnot a sum. A sub-element is an element rather than a bare
(Q, x)pair becausecontracting it needs its own
dual_transformation— a physically mappedsub-element's basis transformation applies to its own index range, not the enriched
one — and its own
get_indices().dual_evaluationon a sum is then literally"concatenate my sub-elements' own dual evaluations", and each of those is the
ordinary single-element path.
Why the points are never brought together
The blocks could instead be aligned onto one point set and stacked along a single
point index. They must not be:
unconcatenatesplits aConcatenateonly along anindex that is still free in an assignment, and the point index never is — points are
always contracted by the sum in
dual_evaluation. A concatenation over points is adead end by construction. It survives into the generated code as a
Concatenatetemporary, and the per-block structure that motivated aligning them is destroyed
anyway.
Concatenating over the basis index is exactly what
unconcatenatewas designedfor; its own docstring works the example through for
RTCF7, splitting into blocksof 8x7 and 7x8 that each keep their own tensor structure. That index stays free in
the assignment, so it splits downstream, and each block is contracted on its own
points with whatever structure it has.
One level at a time
The decomposition goes one level deep and
dual_evaluationrecurses, so each elementconcatenates along the index its own sum occupies. That matters for a tensor
product: its sub-elements own a slice of one factor's basis index and the whole of
every other, which is a sub-box of the basis multiindex rather than a contiguous
range of a flat one. Stacking on that factor's own index is correct wherever the
summed factor sits, whereas flattening straight to the elements that are not sums
would renumber the DoFs unless the summed factor happened to be outermost.
The restricted
NCE/NCFtests exercise this several levels deep: the summed factorsits inside a
FlattenedDimensionsinside an H(curl) or H(div) wrapper.Restricted H(div)/H(curl) elements are nodal
EnrichedElementinfers nodality from pairwise orthogonality, which only recognisesblocks that map to different components. Restricting an element on a tensor product
cell splits each direction into several blocks that map to the same component, so
the inferred answer was
Falseand restrictions ofNCE/NCFwere reported asnon-nodal. Restriction selects disjoint subsets of the DoFs, so the restricted
subelements are nodal whenever the original ones are; carry the flag through instead
of re-deriving it, as the tensor product restriction already does.
Testing
check_nodal— applying the dual basis to the primal basis and asserting theidentity — is the mathematical check, and it runs over the enriched, tensor-product
and restricted H(div)/H(curl) cases. The FInAT and gem suites are green (352 passed,
8 skipped), as is
tests/firedrake/multigrid/test_hiptmair.py::test_gmg_hiptmair_hcurl[hexahedron-aij],which was the failing case that motivated this.
The broader Firedrake suites (multigrid, interpolation) have not been run against
this yet.