[Breaking] UnitStyle, allunits and a bunch of renaming

[borisdevos]

allones will return all units of some sector, which is useful for multifusion cats where there are multiple.

MultiFusionStyle will aid us where needed in specialising to multifusion cases within TensorKit. I added also its interplay with FusionStyle, but this is completely untested. It's untested even within the multifusion case, as in I've yet to take any product sector involving a multifusion sector.

A question is if we should have aliases combining e.g. UniqueFusion and UniqueMultiFusion, since there are some specialties for e.g. abelian groups which otherwise go through GenericFusion code. Coming up with a name is then hard as usual 😄. This works, so it might be overkill.

[borisdevos]

Tests fail because IsingBimodule no longer has the FusionStyle property. Should this refer to MultiFusionStyle, or the other way around everywhere (as in all FusionStyle checks become MultiFusionStyle checks, and MultiFusionStyle(...) = FusionStyle(...) by default)?

[lkdvos]

I'm not so sure it makes sense to have the same distinction as for the FusionStyle, since really the only distinction we care for currently is whether or not the unit is simple. I think I would suggest

abstract type MultiFusionStyle end

struct SimpleMultiFusion <: MultiFusionStyle end
struct GenericMultiFusion <: MultiFusionStyle end

where the former denotes a simple unit, and the latter everything else.

Similarly, I don't think this should be defined as a replacement of FusionStyle, or have any interplay with that at all, since that really only says something about the output of otimes(a, b), and how many times these appear, so I'd definitely keep that for IsingBimodule.

[codecov]

Codecov Report

❌ Patch coverage is 86.36364% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/sectors.jl	84.84%	5 Missing ⚠️
src/anyons.jl	86.66%	2 Missing ⚠️
src/precompile.jl	0.00%	2 Missing ⚠️
src/product.jl	84.61%	2 Missing ⚠️
src/trivial.jl	50.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/TensorKitSectors.jl	16.66% <ø> (ø)
src/fermions.jl	73.17% <100.00%> (ø)
src/groupelements.jl	78.12% <100.00%> (ø)
src/irreps/cu1irrep.jl	96.47% <100.00%> (ø)
src/irreps/dnirrep.jl	97.36% <100.00%> (ø)
src/irreps/su2irrep.jl	91.66% <100.00%> (ø)
src/irreps/u1irrep.jl	94.11% <100.00%> (ø)
src/irreps/znirrep.jl	100.00% <100.00%> (ø)
src/multifusion.jl	89.58% <100.00%> (ø)
src/trivial.jl	16.66% <50.00%> (+5.55%)	⬆️
... and 4 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jutho]

There is a little bit of interplay in the sense that the multifusion case can never be UniqueFusion I think, provided we interpret UniqueFusion as every tensor product of simple objects has exactly one simple object as outcome. Give me a heads up if this is at the point of being merged; I would like to contemplate about the trait naming a bit more, but not for too long.

[borisdevos]

I think strictly that fusion categories aren't multifusion ones, so in principle SimpleMultiFusion is a property just for fusion categories. In this sense, UniqueFusion is impossible for multifusion categories. However, if you choose to view every fusion category as a 1x1 multifusion category, then UniqueFusion is possible. This is the approach I took.

[Jutho]

I want to propose something a bit radical, which is that we adopt the name unit instead of one more generally. This implies that leftone and rightone are renamed to leftunit and rightunit, and that we perhaps have a unit function for the cases where they are equal. We furthermore have allunits for the new function. Specific sectors can implement allunits, and unit can have the general definition unit(I::Type{<:Sector}) = only(allunits(I)). Also, I would like dual to be the function that is defined instead of conj. Hence, the basic interface for a sector would essentially not use any methods from Base, and would adhere to

Every new `I<:Sector` should implement the following methods:
*   `allunits(::Type{I})`: list of unit elements of `I`
*   `dual(a::I)`: ``a̅``, dual label of ``a``
*   `⊗(a::I, b::I)`: iterable with unique fusion outputs of ``a ⊗ b``
    (i.e. don't repeat in case of multiplicities)
*   `Nsymbol(a::I, b::I, c::I)`: number of times `c` appears in `a ⊗ b`, i.e. the
    multiplicity
*   `FusionStyle(::Type{I})`: `UniqueFusion()`, `SimpleFusion()` or
    `GenericFusion()`
*   `BraidingStyle(::Type{I})`: `Bosonic()`, `Fermionic()`, `Anyonic()`, ...
*   `Fsymbol(a::I, b::I, c::I, d::I, e::I, f::I)`: F-symbol: scalar (in case of
    `UniqueFusion`/`SimpleFusion`) or matrix (in case of `GenericFusion`)
*   `Rsymbol(a::I, b::I, c::I)`: R-symbol: scalar (in case of
    `UniqueFusion`/`SimpleFusion`) or matrix (in case of `GenericFusion`)
and optionally
*   `dim(a::I)`: quantum dimension of sector `a`
*   `frobeniusschur(a::I)`: Frobenius-Schur indicator of `a`
*   `Bsymbol(a::I, b::I, c::I)`: B-symbol: scalar (in case of
    `UniqueFusion`/`SimpleFusion`) or matrix (in case of `GenericFusion`)
*   `twist(a::I)` -> twist of sector `a`

Base.:*, Base.one and Base.conj can remain as convience wrappers for ⊗, unit and dual, respectively. I think the current approach is very heterogeneous, with using Base.one and Base.conj as primary methods (and defining dual as a fallback), but at the same time using ⊗ as primary method with Base.:* the fallback.

The new trait could simply be called

abstract type UnitStyle end
struct SimpleUnit <: UnitStyle end # ordinary fusion category
struct GenericUnit <: UnitStyle end # multifusion category

While I recognize that this is a large breaking change for TensorKitSectors.jl, I am not sure that the downstream effects would be that large. TensorKit.jl would need to make a few small leftone->leftunit replacements, but itself doesn't export any of this. It does however export insertleftunit and insertrightunit at this very point, making this all a bit more consistents.

Finally, TensorKit should have an intrinsically named method to replace oneunit, e.g. unitspace, which calls allunits and add those with dimension one to a GradedSpace. We can keep the fallback so that Base.oneunit still works.

[lkdvos]

I can definitely see the arguments for this, and I agree that it is nice to be a bit more consistent internally about when we do and do not incorporate the Base overloads. In that regards I'm more than happy to switch the default implementation of all these methods over to TensorKitSectors' methods, and dispatch the Base ones through to that where applicable.

I don't think this is really what you are suggesting, but I would be opposed to dropping these Base overloads altogether. While it is definitely nice to be correct and consistent, the number one complaint I hear about TensorKit is that it requires too much knowledge that is irrelevant to typical usecases. Having Base.one be the trivial sector which you can multiply intuitively makes a lot of sense, and most people don't really feel strongly about the small inconsistencies this brings with respect to the mathematical rigor.

I do want to say that all of these changes are very much breaking, as SUNRepresentations and CategoryData and GroupData and SymmetricGroupRepresentations all have to be updated to reflect this change since they have to implement the intrinsic functions, not the overloads. TensorKit also has to be updated for this, and I do think we are making use of this in both MPSKit and PEPSKit as well. I'm not against breaking changes, but I do want to mention that this is quite a big change just to be more correct about naming things, where most of the users probably don't even have the background to see why this is more correct because this doesn't relate to things they are using.

[Jutho]

I don't think this is really what you are suggesting, but I would be opposed to dropping these Base overloads altogether.

Well, actually, I had clearly myself forgotten, since at the level of sectors, Base.:* does not even work, only ⊗ works. So in that sense one is even more inconsistent than I originally thought (and could actually be removed, as I don't know how often users need to operate at the level of sectors).

For spaces, on the other hand, that is a very different situation. There I am definitely not suggesting to remove the Base.:* alias for ⊗ (also not in the long term), and also one should remain in its current form (I wouldn't be able to come up with an alternative name for the empty ProductSpace), as well as Base.adjoint as alias for dual.

I do want to say that all of these changes are very much breaking, as SUNRepresentations and CategoryData and GroupData and SymmetricGroupRepresentations all have to be updated to reflect this change since they have to implement the intrinsic functions, not the overloads. TensorKit also has to be updated for this, and I do think we are making use of this in both MPSKit and PEPSKit as well. I'm not against breaking changes, but I do want to mention that this is quite a big change just to be more correct about naming things, where most of the users probably don't even have the background to see why this is more correct because this doesn't relate to things they are using.

Oh yes, when I said, not that breaking, I should have clarified that I meant this from the perspective of the user (of which there are hopefully ever more), exactly for the reason of above: I don't know if they need to manipulate sectors often (but I don't mind being corrected). On the developer side, this whole suggestion is of course very much breaking throughout the several levels of packages in our ecosystem 😄 .

[borisdevos]

I'm not sure if what I've changed up till now is breaking or not. If so, depending on whether we rename or not, I might need to revert a bunch of changes.
Tests fail currently at one(::Type{ProductSector}). In general I'm not sure if the changes I made concerning product sectors is the cleanest, but it does do what I expect. For example, allones for a product sector consisting of regular fusion categories returns a tuple with just one object, while units are pairwise multiplied when multifusion categories are involved.

[Jutho]

I think this is good to go. But it would be good to get a second review by @lkdvos before merging.

[lkdvos]

I left some small comments, but otherwise looks like a great PR. Can you also bump the version, since this is definitely breaking?

[lkdvos]

Do we also want SectorSet here?

[borisdevos]

While implementing isunit, I noticed FibonacciAnyons field name to be isone. While we're making breaking changes anyways, should we make this also isunit? This is assuming we accept isunit.

[lkdvos]

Definitely fine by me, I think that also doesn't even qualify as breaking, since fields are typically assumed internal

[Jutho]

+1 for isunit

[Jutho]

I added one more comment/request. Otherwise this is good to go for me.

[Jutho]

Ok, this looks great. Thanks @borisdevos . I know we suggested to start with this "small" PR that would be easy and quickly merged a few weeks ago, and it turned out a bit bigger than that. But the result is really nice.