Compatibility with MultiTensorKit

[borisdevos]

This PR can be divided in three parts:

1. The largest part concerns fusion tree manipulations where the type of unit (leftone or rightone) matters in a multifusion context. As for now during the draft, I've left some comments where the type of unit was not rigorously derived, but guessed to get the code running. These will be removed when benchmarking MultiTensorKit is complete.

2. A potential hack was used to promote storage types when I was working with ComplexF64 F-symbols in MultiTensorKit.

3. Minor docs and docstring typos.

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.91%. Comparing base (0c73bd2) to head (9d543ee).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
src/fusiontrees/iterator.jl	0.00%	1 Missing ⚠️
src/fusiontrees/manipulations.jl	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   82.65%   82.91%   +0.25%     
==========================================
  Files          43       44       +1     
  Lines        5593     5754     +161     
==========================================
+ Hits         4623     4771     +148     
- Misses        970      983      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[lkdvos]

Overall looks like a very minimal set of changes, that's really impressive!

I think I fully agree with your assessment of separating out the different points into separated PRs. Indeed, the typos can be merged as-is, so we can easily get that out of the way.

The fusiontree manipulations might indeed require some form of tests, so we may need to consider adding a very simple but non-trivial multifusion category to TensorKitSectors, just for testing purposes. For example, simply using the multifuse(Z2, Z2) sectors maybe seems feasible?
The scalartype is an independent issue that actually would have already appeared if we are dealing with complex sectortypes and real tensors, but I think this is just not something that people are ever really using that often, which explains why we haven't run into that (yet).

[lkdvos]

For these changes, I think it would be nice to have a dedicated discussion (indeed in a separate PR) and some comments explaining what is going on.

The bottom line being that we want the sectorscalartype to mix into the actual scalartype in a very subtle kind of way, only really being used to promote from <:Real to <:Complex while leaving the actual precision the same. (I think?)

The implementation below is somewhat of a hack since normally TensorOperations is responsible for figuring out the destination scalartype TC, so in principle we would expect scalartype(tensoradd_type(TC, ...)) === TC, which is not the case here.

There are a couple options for fixing this:

1. Since TensorOperations uses scalartype, we could consider redefining scalartype(::AbstractTensorMap) to take the sectorscalartype into account. This would boil down to really having a different meaning between eltype and scalartype for TensorMaps, where the former is the type of the stored data, and the latter also contains information about the field.
2. We always require complex entries if the sectorscalartype is complex. This doesnt have too many performance implications since most operations on tensors would result in a complex tensor anyways, but we would have to think carefully about DiagonalTensorMap, since that one could have real entries (e.g. for singular value decompositions)
3. We rework the implementation of promote_contract in TensorOperations and the macros to actually work on values or types thereof instead of directly on scalartypes, such that we can overload promote_contract(::AbstractTensorMap, ::AbstractTensorMap, ::Number) or promote_contract(::Type{<:AbstractTensorMap}, ...). This is quite a big internal change for TensorOperations though, since currently all macros expand to promote_contract(scalartype(A), scalartype(B), ...).

[Jutho]

Is this something that came up specifically in this PR? This seems like it should already have been an issue with existing complex sectors.

[Jutho]

Is this something that came up specifically in this PR? This seems like it should already have been an issue with existing complex sectors.

[lkdvos]

Yes, but I don't think we ever used complex sectors with non-complex tensors all that much, and MPSKit actually uses complex entries for almost everything by default.

[Jutho]

Thanks for catching this. Did you encounter this by reading the code or by an actual case where the old implementation errored? Clearly there should have been a test in place to cover those lines of code. So if you have some example code that actually triggers these lines, that might be a good starting point to extract a test case from (which might be easier then engineering one from scratch).

[borisdevos]

I found this from an error. However, I cannot find what I was doing to trigger this particular iteration... I will keep looking

[Jutho]

I left some comments and suggestions; the only part I want to revisit is the elementary_trace part in fusiontree manipulations (mostly to remind myself of how the implementation works). It is indeed very nice that such a small number of changes is sufficient to incorporate multifusion categories; even now about half of the changes are bug fixes that were needed (thanks!) even without the multifusion case. Very impressive work.

[Jutho]

This looks almost complete to me, aside from one more change in elementary_trace and the formatting issues. It would be great to push this over the finish line and have it merged.

[borisdevos]

If it's alright, I think it's better to get this merged now. I can then look into adding multifusion tests in another PR, but this will require a bit more work.

I sadly did not end up not finding the code I ran to get the fusion tree iterator error I fixed. I've left it unfixed in my local version, so I might encounter it in the future (fingers crossed).

[Jutho]

@lkdvos, did you want to have a final review or did you have any pending comments before I merge this?

[Jutho]

It seems like you will also need to update the test; I agree the current eltype return value is correct, so this is an oversight in my reviewing of #206 . While you are at it, do you think that the custom Base.isdone implementation is really useful? This seems to be mostly something for stateful operators. There is nothing wrong with having this, but less code to maintain / test is always nice 😄 .

[lkdvos]

Yeah, I bumped into this (@borisdevos showed me) that collect(blocks(t)) failed because of this, so there is really no getting around this as an eltype. I agree that removing this is probably better.

[lkdvos]

Ok to merge for me if tests pass.

[Jutho]

Thanks for the final fixes @borisdevos ; I'll merge.