Sum factorise independent contractions separately - #269
Merged
Conversation
Author
|
@connorjward this is a bugfix, should I target release? |
Author
|
I think that we only hit this in interpolate and not in assemble because assemble does some extra COFFEE processing. But arguably the right thing is to have the decomposition into smaller subproblems in one place. |
This was referenced Jul 29, 2026
I don't especially mind. Given that we are releasing 2026.10.0 soonish, and I have no intention of making another patch release unless required, the issue is somewhat moot. |
rckirby
reviewed
Aug 5, 2026
rckirby
left a comment
There was a problem hiding this comment.
I think this is fine, if kind of complicated. Is there a test that this is correct, or at least returns gem that evaluates to something equivalent to the previous version?
pbrubeck
commented
Aug 18, 2026
sum_factorise searches every ordering of the contraction indices, so it
gave up past six of them. But a product of tensor product coefficient
evaluations has one set of indices per coefficient, and no factor carries
the indices of another, so the orderings that interleave them are never
worth searching.
Split the contraction into connected components of the graph joining the
indices that share a factor, and search each separately. Interpolating
f*f*f into a hexahedron CG1 space raised
NotImplementedError: Too many indices for sum factorisation!
as three coefficients contribute three indices each; it now factorises as
three independent contractions. This is also cheaper wherever it already
worked, as the search is over the orderings of each component rather than
of all the indices at once.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pbrubeck
force-pushed
the
pbrubeck/sum-factorise
branch
from
August 18, 2026 10:03
06f25f6 to
949de24
Compare
This was referenced Aug 18, 2026
rckirby
approved these changes
Aug 18, 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.
Interpolating a product of three coefficients into a hexahedron CG1 space:
raised
sum_factorisesearches every ordering of the contraction indices, which isfactorial in their number, so it gave up past six of them. On a tensor product cell
each coefficient evaluation contributes one index per direction, so three
coefficients on a hexahedron already need nine.
But those indices do not interact: no factor carries the indices of more than one
coefficient, so the orderings that interleave them are never worth searching. This
splits the contraction into the connected components of the graph joining indices
that share a factor, and searches each component separately.
That fixes the failure above, and is also cheaper wherever it already worked, since
the search is over the orderings of each component rather than of all the indices at
once. A single connected contraction is still bounded at six.
Testing
test/gem/test_sum_factorise.pychecks the factorised expression against the valueobtained by contracting the tables directly, for products of up to five independent
contractions of up to five indices each — well beyond the old limit — and checks that
one connected contraction is still rejected. Both new cases were confirmed to fail on
the unpatched code with the original error.
Verified end to end in Firedrake that
f**ninterpolates correctly fornup to 5 onquadrilateral, triangle, hexahedron and prism meshes, to machine precision.
Full FIAT suite: 2431 passed, 26 skipped, 31 xfailed.
🤖 Generated with Claude Code