Fix planar index reordering and remove _contractedspace - #532
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Member
|
Always happy to be superseded, this looks great! |
lkdvos
force-pushed
the
lb/planar-general-pab
branch
from
September 15, 2026 17:27
095fcbe to
8ba2eaa
Compare
Jutho
reviewed
Sep 17, 2026
Jutho
reviewed
Sep 17, 2026
Jutho
reviewed
Sep 17, 2026
lkdvos
force-pushed
the
lb/planar-general-pab
branch
from
September 17, 2026 17:59
8ba2eaa to
acf2daf
Compare
Jutho
reviewed
Sep 17, 2026
Jutho
reviewed
Sep 17, 2026
Jutho
approved these changes
Sep 17, 2026
Jutho
left a comment
Member
There was a problem hiding this comment.
Up to small suggestions, very much approved.
…pace` `@planar` allocates through `TO.tensoralloc_contract` with the raw index tuples of the planar decomposition, in which `pA` and `pB` need not be planar partitions by themselves; only `planarcontract!` rotated them into shape, and only when `pAB` could be absorbed into those rotations. Replace `reorder_indices` by `planar_contract_indices`, which canonicalizes `pA` and `pB` and remaps `pAB` instead of absorbing it, let `planarcontract!` apply a residual cyclic `pAB` with `transpose!` through an intermediate, and allocate planar destinations through the new `planaralloc_contract`. Allocate the destination of `⊗` directly, since its intermediates are non-planar by construction. With every caller passing planar partitions, `tensorcontract_structure` can again compose the intermediate spaces, which also restores contractions of operands with different `spacetype` parameters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`planar_contract_indices` only needs the index partitions of both operands, which `@planar` knows: `_extract_tensormap_objects` checks the partitions written in the expression against the actual tensors. Record them while preprocessing and canonicalize the index tuples of the emitted planar contractions, so that the runtime canonicalization is left with nothing to do for macro-generated code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Now that `@planar` emits canonical index tuples, its allocations no longer need to be canonicalized at runtime and can go through `tensoralloc_contract` again. Canonicalizing before the planar operations are inserted also means that the contraction and its allocation still share a single argument layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename `_index_partitions!` to `_record_index_partitions!` so the preprocessor name starts with a verb, and take `ex` as its first argument for consistency with the other preprocessors. Add the missing `!` to `_decompose_planar_contractions!`, which likewise mutates its accumulator. Also test the planar contraction with explicit parentheses, so that both association orders of the three-factor contraction are covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`mul!(tC, tA, tB, α, β)` forwarded `VectorInterface.One`/`Zero` verbatim to LinearAlgebra's block `mul!`. Ordinary GEMM tolerates this, but a contraction of a tensor with its own adjoint dispatches to the HERK path, where `herk_wrapper!` calls `isreal` on the scalars and throws a `MethodError`. Map them to `true`/`false` first, which LinearAlgebra still recognizes as exact one and zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lkdvos
force-pushed
the
lb/planar-general-pab
branch
from
September 20, 2026 14:39
acf2daf to
bb637c8
Compare
This was referenced Sep 20, 2026
Merged
lkdvos
added a commit
that referenced
this pull request
Sep 21, 2026
* Draft changelog for v0.17.2 Consolidates the Unreleased section (which already included the real entries added by #526/#532 on merge) with entries for the remaining PRs merged since v0.17.1 (#487-#535), and retitles it as 0.17.2. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Bump version to v0.17.2 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix confirmed small bugs from the pre-release audit - isunitspace: require dim(V) == 1 for GenericUnit sectors (#537) - GradedSpace ⊕/supremum: check unit homogeneity of the result (#538) - isconj(::ComplexSpace): return isdual(V) instead of always true (#539) - multi_associator: return a vector, not a scalar, on early-exit for GenericFusion (#540) - split(f, 0): use leftunit(f.coupled) instead of indexing an empty uncoupled tuple (#541) - repartition: return a Pair in the identity branch, matching every other branch (#542) - Mooncake scalar_pullback: accumulate into the tangent instead of overwriting it (#543) - rand/randn/randexp/randisometry(rng, T, space): fix one(domain) typo (#544) - pinv(::DiagonalTensorMap): fix inverted atol/rtol defaulting and empty-tensor throw (#545) - t1 / t2: promote to a float scalartype, matching t1 \ t2 (#546) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add changelog entry for the audit bugfixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address fable review findings on the audit bugfixes - split(f, 0): also guard innerlines_extended construction, which still indexed the empty uncoupled tuple for a 0-leg tree - pinv(::DiagonalTensorMap): use eps (not sqrt(eps)) for the default rtol, matching _default_rtol's convention and dense LinearAlgebra.pinv Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address tuicr review comments on the audit bugfixes - pinv(::DiagonalTensorMap): reuse _default_rtol instead of duplicating its formula - Add regression tests for split(f, 0) on a genuine 0-leg tree and for multi_associator's early-exit branch on a GenericFusion sector Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix planar issues after MPSKit test rerun * harden Mooncake scalar pullback --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Fixes the root cause that #531 works around, and supersedes it.
Problem
@planarallocates throughTensorOperations.tensoralloc_contractwith the raw index tuples of the planar decomposition, in whichpAandpBneed not be planar partitions by themselves — onlyplanarcontract!rotated them into shape, and only whenpABcould be absorbed into those rotations. Since #515 added the coloring checks to theProductSpace/HomSpaceconstructors, computing the output space aspermute(compose(permute(VA, pA), permute(VB, pB)), pAB)throws forGenericUnitsectors, which is why #515 replaced it by the leg-wise_contractedspace. That workaround pins both operands to a single spacetype, which breaks contracting operands whosespacetypeparameters differ (aBlockTensorMapwith aTensorMap, as reported from MPSKit in #531); the fallback added there fixes theMethodErrorbut reintroduces exactly the intermediate spaces that #515 removed.Changes
planarcontract!takes an arbitrary cyclicpAB, like the non-planarcontract!: a residual permutation is applied withtranspose!through an intermediate, mirroring whatblas_contract!does withpermute!. The remaining difference between planar and non-planar contraction istransposevspermute, which is also what the AD rules of Planar additions [WIP] #124 need.reorder_indicesis replaced byplanar_contract_indices(A, pA, B, pB, pAB) -> pA′, pB′, pAB′: it rotates each operand's index cycle so the contracted indices form a contiguous arc and remapspABinstead of absorbing it. It is shared with the twoBraidingTensormethods and works onHomSpaces as well as on tensors.@planarcanonicalizes the index tuples it emits at macro-expansion time:planar_contract_indicesonly needs the index partitions of both operands, and those are written in the expression and already checked against the actual tensors by_extract_tensormap_objects. Both the contraction and its allocation therefore see planar partitions, and the allocation needs no planar-specific entry point. Hand-written planar kernels should canonicalize withplanar_contract_indicesbefore allocating withtensoralloc_contract;planarcontract!keeps canonicalizing at runtime, so the kernel itself is safe either way.⊗allocates its destination directly; itspA = ((all indices), ())intermediate is non-planar by construction.tensorcontract_structurecomposes the intermediate spaces again and_contractedspaceis gone, which restores mixedspacetypesupport. ForGenericUnitsectors,pAandpBpassed to it must now be planar partitions.Tests
New multifusion coverage (there was no
@planarbinary contraction on multifusion spaces): a planar contraction whose partitions are non-planar by themselves,⊗with more than one index in the domain, a non-trivial and a non-cyclicpAB,planar_contract_indicesunit tests including the fully contracted and non-planar cases, and a check on the index tuples of the macro expansion.Verified on the full space list for
test/tensors/{contractions,braidingtensor,planar,linalg,indexmanipulations}.jlandtest/symmetries/spaces.jl. The mixed-spacetype case has no in-repo coverage (it needs a downstream space type), so it is worth a run against BlockTensorKit + MPSKit before merging.🤖 Generated with Claude Code