insertleftunit, insertrightunit and removeunit

[lkdvos]

Here I extend the insertunit function, which previously only worked for ProductSpace, to also include HomSpace and TensorMap instances. As this is an operation which is quite common in MPSKit, it would be nice to have a centralized implementation for it, which is also more efficient than the current approach which contracts with a trivial vector.

Additionally, I add the removeunit function to undo that operation.

I'm not entirely sold on the names and interface I currently have for the function, but I'm struggling to come up with anything better. If you would have more inspiration, I'm all ears.

[codecov]

Codecov Report

Attention: Patch coverage is 58.92857% with 23 lines in your changes missing coverage. Please review.

Project coverage is 82.92%. Comparing base (c041bfe) to head (cb0e1f2).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/tensors/indexmanipulations.jl	30.00%	21 Missing ⚠️
src/auxiliary/auxiliary.jl	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   83.16%   82.92%   -0.24%     
==========================================
  Files          42       42              
  Lines        5257     5312      +55     
==========================================
+ Hits         4372     4405      +33     
- Misses        885      907      +22     

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

[Jutho]

Thanks for this; looks great. Regarding names, sounds good to me. The only other thing I can come up with is "apply_unitor" 😺 .

[Jutho]

It does seem like the preferdomain constant propagation is not working on Julia 1.11. Not sure if anything can be done about that?

Also, if you want, could you add the doc references in the docs/src/lib sections of the manual? It's ok if you'd prefer me to do so.

[lkdvos]

You mean that the constant propagation is not working on 1.10 right? I could be convinced to ignore that, but this ties in with the following comment:

The main problem (and what I like the least about the interface right now) is that if you want to insert a unit between the codomain and domain, it's unclear whether that should end up in the domain or codomain. This is also what's bothering the constant propagation I think, since that involves an additional flag to control that...
The only alternative I can come up with is to change this to insertcodomainunit and insertdomainunit, but these names are rather long... This would however solve the type instability issues.
Another option could be to internally write it in terms of these functions, and then have a separate insertunit that just switches between these two.
Another option I came up with is to just consider numind(t) + 2 possible ways of inserting a unit and using 0:(numind(t)+1) to denote them. In other words, 0 ends up before the first space of the codomain, ..., numout(t)-1 before the last space of the codomain, numout after the last space of the codomain,
and thennumout(t)+1 before the first space of the domain, ..., numind(t) before the lats space of the domain, and numind(t)+1 after.
I'm not sure how confusing that gets though...

It's just very unfortunate that I can't come up with a good way to disambiguate between these with just an integer.

[Jutho]

Well, if you want to stick to the category language, there is a left and right unitor. So you could say, insertleftunit/insertrightunit and then you specify the index i (i ∈ 1:numind) to the left/right of which the unit index should appear. But I agree that this is also not very nice, and that making insertunit work is more convenient.

Also, I haven't checked in detail whether the constant propagation failing is only for the case where preferdomain is relevant (i.e. i==numout), or just in general on the value of i as well (although, given that that is a regular argument, that should probably work).

[lkdvos]

So, I actually prefer the insertleftunit and insertrightunit syntax a lot more, since at the very least that gives a consistent answer, and it sounds more convenient to reason about in my head.
Additionally, since for me the main use case is MPSKit, your comment made me realize that this is something that @borisdevos had to patch everywhere because in the multi-fusion case there is actually a distinction. So, for me it sounds reasonable to just "do it right" in the first place and have the correct left or right unit throughout MPSKit, and I think the left_ and right_ names are more intuitive than preferdomain.

[Jutho]

So it does seem there is still an issue with constant propagation on julia-latest. I am not entirely sure but I think a construct like @constinferred removeunit(insertleftunit(W, 3), 3) only tests the type stability of removeunit. It is very strange that this line is failing, whereas similar lines above are not.

[Jutho]

So I played around with this on Julia master to test what is going on. The reason that removeunit is interfering with constant propagation is because removeunit(::ProductSpace, ::Int) contains to error checks and corresponding throw comments. You can make everything work by something like this:

function removeunit(P::ProductSpace, i::Int)
    1 ≤ i ≤ length(P) || _boundserror(P, i)
    isisomorphic(P[i], oneunit(P[i])) || _nontrivialspaceerror(P, i)
    return ProductSpace{spacetype(P)}(TupleTools.deleteat(P.spaces, i))
end

@noinline function _boundserror(P, i)
    throw(BoundsError(P, i))
end
@noinline function _nontrivialspaceerror(P, i)
    throw(ArgumentError(lazy"Attempting to remove a non-trivial space $(P[i])"))
end

On a different note, as I then tried to insert the same bounds check for insertleftunit and insertrightunit, I got confused by the doc string for the meaning of i and the default arguments. For insertleftunit, I see how inserting something to the left of the current index i makes the new index to appear at position i, whereas for insertrightunit, inserting to the right of index i is what you do. However, is it meaningful to insert something to the left of the non-existing index length(P)+1 (the current default), or to the right of the nonexisting index at position 0. I guess a reasonable default for insertleftunit is just i=1, no? Then insertleftunit(P) inserts an extra space in the beginning, and insertrightunit(P) inserts it at the end. Should we then be strict and require 1 <= i <= length(P) in both methods, or is that too pedantic?

[lkdvos]

I agree that it's a bit counterintuitive, but it's actually required (but could maybe do with some better explanation):
For a 0,N homspace, if you want to have the ability to turn that in to 1,N as well as 0,1+N (both inserted in the beginning), we need to allow insertrightunit on 0 for the former, and insertleftunit on 1 for the latter

[Jutho]

Ok I hadn't thought about that. But then what if you want to turn a N,0 into an N,1 tensor? is that possible even?

[lkdvos]

It should be: insertleftunit N+1 would do that

[lkdvos]

I tried making the changes and still ran into some type-instabilities, so I just caved and added the @constprop annotations. Locally it seems to run, so if everything turns green it should be good to go.