-
Notifications
You must be signed in to change notification settings - Fork 53
IsingBimodule
for testing multifusion category within TensorKit
#263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I managed to fix the last bug I mentioned in the beginning. It turns out I should've known it was that, because I encountered that issue twice already previously 😭 I ended up putting all |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 83.39% 83.49% +0.10%
==========================================
Files 44 45 +1
Lines 5757 5823 +66
==========================================
+ Hits 4801 4862 +61
- Misses 956 961 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IsingBimodule
for testing multifusion category within TensorKitIsingBimodule
for testing multifusion category within TensorKit
This reverts commit 772452f.
…oris-isingbimod
sector = leftone(first(sectors(S))) | ||
return spacetype(S)(sector => 1) | ||
end | ||
leftoneunit(S::GradedSpace{I}) where {I<:Sector} = oneunit(typeof(S)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was wrong with the previous implementations? This looks like it will always fail for multifusion things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted left/rightoneunit
to also work on empty spaces, because oneunit
does so. This will indeed fail for multifusion things, hence the specialisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but what do you think about this then:
function leftoneunit(V::GradedSpace{I}) where {I<:Sector}
s = sectors(V)
u = leftoneunit(isempty(s) ? I : first(s))
return spacetype(V)(u => 1)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not great if we have to specialize everything specifically for Vect[IsingBimodule]
, since this would mean we have to redo all that work for any multifusion category. In that sense, we would either need a fusion type that specifies this, such that we can specialize properly, or write these methods in a generic way that works for everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by redoing all that work. IsingBimodule
should be the only multifusion category tested within TensorKit itself. This infrastructure already exists in MultiTensorKit (albeit in a PR for now), which will support all the other multifusion categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that if all we are testing is the specializations that are written solely for IsingBimodule, which is used mainly for testing, this doesn't actually help all that much, since it would have to be both reimplemented and retested for other multifusion categories as well
@@ -268,28 +272,27 @@ for V in spacelist | |||
end | |||
end | |||
@timedtestset "Full trace: test self-consistency" begin | |||
t = rand(ComplexF64, V1 ⊗ V2' ⊗ V2 ⊗ V1') | |||
t2 = permute(t, ((1, 2), (4, 3))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for removing this t2
call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all the permutations in tests which weren't explicitely testing for permutations, which I had to do anyway for the multifusion tests. This would allow for testing anyonic sectors if we decide to expand the space list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I think I would like to keep both though, to actually have coverage of as much as possible. Probably it is better to just separate these things out a bit further.
@planar s2 = t[a b; a b] | ||
@planar t3[a; b] := t[a c; b c] | ||
@planar s3 = t3[a; a] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely good to test both @planar
and @tensor
, but we probably want to test both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also with expanding to non-symmetric braidings in mind. I could put a SymmetricBraiding
check here to then evaluate @tensor
if you want?
This PR makes use of the newly introduced
IsingBimodule
multifusion category in QuantumKitHub/TensorKitSectors.jl#14 to test the changes in #247 within TensorKit itself. This was needed because MultiTensorKit depends on TensorKit itself.What's new:
leftoneunit
andrightoneunit
are introduced to naturally extendoneunit
, but not forType{GradedSpace{...}}
because it needs to know the objects within theGradedSpace
.artin_braid
had anisone
check replaced with a multifusion-friendly check (although this might be excessive, since as of now we don't consider braiding within multifusion categories)one(IsingBimodule)
IsingBimodule
fusion trees, spaces and tensorsI left a bunch of comments everywhere to remind me of certain choices I made in the implementation. It would be nice if these were addressed. I can clean most of them afterwards, but should probably keep some of them, or at least the ones in the tests to help whoever might need to maintain those in the future.
As I started on this before #247 got merged, its commit history is here as well, and I'm not sure what kind of rebase I need to do to get rid of it. I tried something and files seemed to want to vanish, so I aborted that attempt 😅 Those luckily don't appear in the files changed though.
IsingBimodule
still needs to get renamed to that, so currently everything isIsingBimod
. I'm assuming we're waiting on QuantumKitHub/TensorKitSectors.jl#19 to merge to release v0.2 though.Edit: I forgot to mention, but currently there's one thing broken which I can't find the error in. It appears in the full trace test. Ignoring the part where I don't know what to do with tracing module legs, it seems that the behavior is different for the two fusion categories inIsingBimodule
. In particular, the full trace test passes for the "C" fusion category, but not for "D", the latter tracing to zero via@planar
. I checked this quickly in MultiTensorKit with the multifusion category I got there, and it is consistent there.Edit 2: I also forgot that @Jutho and I briefly discussed trying to increase code coverage, most prominently for
GenericFusion
but also here and there for other functions. I can do this in a separate PR.