[Yue-Zhengyuan]

Fix missing type parameter bounds that caused TensorMap <: AbstractTensorMap to return false.

Add T <: Number to all four concrete tensor map types, plus the missing S <: IndexSpace bounds on AdjointTensorMap and BraidingTensor. Add regression coverage for unparameterized types and types with real or complex scalar parameters.

Background

I bumped into this in QuantumKitHub/PEPSKit.jl#425. There we define

const MPOTerm{T} = AbstractVector{T} where {T <: AbstractTensorMap}

However, if a vector of TensorMaps contains mixed scalar types (e.g. a Float64 tensor and a ComplexF64 tensor) as well as different in/out number of legs, then the vector fails to be recognized as an MPOTerm, because the inferred T is Any:

V = ComplexSpace(2)
v = [randn(Float64, V ⊗ V ← V), randn(ComplexF64, V ← V ⊗ V)]
typeof(v)         # Vector{TensorMap{T, ComplexSpace} where T}
v2 isa MPOTerm    # false before the patch

[lkdvos]

Hmmm, this honestly looks like something that Julia itself might be fixing in the near future. I recall doing this more or less on purpose, such that the bounds are defined in a single place instead of for every type, which can be useful if we ever have to change them, and the weird thing is that I think you can never instantiate a TensorMap with a non-Number element type...
I think I'd have to study a bit more in detail what the upcoming type/subtyping changes will be to see if this makes sense, maybe?

Considering your usecase, that looks like a bit of a strange definition of the type alias to begin with, and I think for inference purposes it is probably better to immediately instantiate these as either Vector{Any} or Vector{AbstractTensorMap}, since the compiler can not really meaningfully optimize with the partial type information. Typically this results in long compilation times because of things like invalidations, so if you really need heterogeneous containers, telling the compiler about that might be your best bet? Happy to have a bit closer look to the exact case if you like too

[Yue-Zhengyuan]

In the PEPSKit case, sometimes an MPO with mixed scalar type can be constructed. A simple example is [real(S_x), S_y] with two sites connected by a trivial leg (suppose we keep this leg for test purposes). This thing fails to be recognized as MPOTerm defined above. Do you mean we should adopt a better definition of MPOTerm? (You can comment in that PEPSKit PR by @leburgel)

[lkdvos]

I think the idea is to make that explicitly AbstractTensorMap[t1, t2], also to avoid having too many different types to compile for. This also makes the compiler give up on inference quicker, which reduces compilation times. To be honest, I'm also expecting that quite a lot of the downstream code is going to not be happy about having a single scalar type/storage type, and the question is whether that is worth it instead of just promoting everything to the same scalar type?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/tensors/adjoint.jl	87.50% <ø> (ø)
src/tensors/braidingtensor.jl	88.60% <ø> (ø)
src/tensors/diagonal.jl	90.41% <ø> (ø)
src/tensors/tensor.jl	81.68% <ø> (+0.99%)	⬆️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[leburgel]

Promoting everything to the same scalar type makes sense, and this is now done on the PEPSKit side. This allows us to keep our type alias as is for now, so that's kind of decoupled from this now. Still I think the suggestions here might make sense in their own right. I find TensorMap <: AbstractTensorMap returning false a bit unfortunate given that there's no way to actually instantiate a TensorMap that's not an AbstractTensorMap.

For context, I can give some motivation for the choice of the type alias and how this came up:

• For the MPOTerm type alias, I did actually consider making this Vector{AbstractTensorMap}, but what was bothering me about this is that the default scalartype implementation doesn't work on this, even when its now a vector with a uniform scalartype. With the current choice the element type is TensorMap{ComplexF64, ComplexSpace, N₁, N₂, Vector{ComplexF64}} where {N₁,N₂}, which is always concrete in scalartype. Since this gives one UnionAll per scalar type, I thought this was a reasonable choice. Do you think this will be straining on the compiler?
• For the constructor (via add_term!), I think we really want to support inputs like [t1, im*t2], without users doing anything special for this. With the current setup, we had to widen the type restriction on this constructor to AbstractVector, since a vector of tensor maps with mixed scalar types doesn't resolve as an MPOTerm under our choice of type alias. With this PR, we could narrow this constructor back down to just MPOTerm, which is just a bit tidier. Changing the type alias to Vector{AbstractTensorMap} wouldn't resolve this with or without this PR, so in the end I didn't find a convincing case for this.