Prevent construction of multifusion tensors with incompatible coloring - #515
Prevent construction of multifusion tensors with incompatible coloring#515borisdevos wants to merge 14 commits into
Conversation
lkdvos
left a comment
There was a problem hiding this comment.
I do think this is probably quite a big performance hit right in the hot path of the fusiontree constructors though, I don't think there is typically a fast implementation of this. Additionally, I was kind of expecting to convert more things to Nsymbol, to actually make them error if the fusion is disallowed?
|
I'm misunderstanding then what kind of behavior we want. So we want |
|
I think that was what I was expecting, especially since we decided |
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This reverts commit 5f9e5be.
…into bd/fusiontree-iterate
Nsymbol calls or add guards in fusion tree iteration
lkdvos
left a comment
There was a problem hiding this comment.
Other than the comments, looks good, thanks for looking into this!
|
|
||
| @inbounds for i in 1:(N - 1) | ||
| _rightunit(spaces[i]) == _leftunit(spaces[i + 1]) || | ||
| throw(ArgumentError(lazy"spaces $(i) and $(i + 1) have incompatible coloring")) |
There was a problem hiding this comment.
Do we want this to be SpaceMismatch, or should we just introduce a new error type for this? Do you also want to print which spaces are the offending ones?
This comment also holds for the HomSpace checks
There was a problem hiding this comment.
Changed this to spacemismatch and return the spaces in both cases
| return HomSpace{S, P1, P2}(codomain, domain) | ||
| end | ||
|
|
||
| function _check_unit_compatibility(codomain::ProductSpace{S}, domain::ProductSpace{S}) where {S <: ElementarySpace} |
There was a problem hiding this comment.
I think there are some missing edge cases that are not really handled correctly/consistently here, mostly for the case where leftunit or rightunit cannot be inferred. In principle it is allowed (?) to create 0-dimensional ElementarySpace objects, which in this implementation will always error even though no violation is detected. (In some sense this is because here you check for validity and error if it cant be proven, while you probably want to error if invalidity can be proven).
The only reason I can think of why this might be important is that if you have an SVD that accidentally kills all sectors, you end up with a 0-dim leg on the S which will now error on the space constructions, even though nothing really went wrong
There was a problem hiding this comment.
You're right, I was also not dealing with length-1 product spaces correctly. My fix for the 0d ones fixed both automatically. Importantly though, I determined that when one of the spaces is 0d, there's no need to check colorings. This is based on the sectors of the total product space always being empty, regardless of valid or invalid coloring, and things like blocksectors and fusiontrees are built on this.
| semisimple unit structure, this returns `true` if `V` is isomorphic to either the left, right or | ||
| semisimple unit space. |
There was a problem hiding this comment.
https://en.wikipedia.org/wiki/Semi-simplicity
[edit] it seems like wikipedia isn't even consistent with itself... (https://en.wikipedia.org/wiki/Semisimple_module)
There was a problem hiding this comment.
The Bible doesn't hyphenate it, so I'll follow suit :)
|
|
||
| function unitspace(S::Type{<:GradedSpace{I}}) where {I <: Sector} | ||
| UnitStyle(I) isa GenericUnit && | ||
| throw(ArgumentError("Cannot construct unit space for sector types with semisimple unit structure.")) |
There was a problem hiding this comment.
Would it be useful to actually check if length(allunits(I)) > 1 before erroring? That way simply declaring a normal fusion category as GenericUnit would not immediately trigger the error.
There was a problem hiding this comment.
I think instead of this, we should add a test in the TKS testsuite checking that GenericUnit actually agrees with having length(allunits(I)) > 1, precisely to prevent normal fusion categories from having GenericUnit. I don't see why we would want that in the first place.
Deals with #514, at least partially.
Now we have
which behaves the same way as
Edit: decided to actually prevent construction of these kinds of tensors at the level of the product space and hom space.
Since this was incompatible with the previous implementation of
unitspace, this now errors forGenericUnitsector types.