Extend CTMRGEnv constructor for nontrivial unit cells

[leburgel]

The CTMRGEnv(peps::InfinitePEPS{P}; Venv=oneunit(spacetype(P))) where {P} constructor was not adapted to the new unit cell convention. Noticed this right before #49 got merged, but was too late to stop the automerge.

I added this as a separate PR since there anyway might be some more things related to unit cells we want to add. One thing that comes to mind is to extend this constructor to support non-uniform virtual environment spaces. Also, we should probably add a more extensive test that puts random duals as well as random dimensions on all virtual spaces (PEPS and environments) and checks that everything matches up everywhere.

Alternatively, I could quickly tag the fix onto #15 and then add the more general constructors and tests later, if that's better @lkdvos @pbrehmer ?

[pbrehmer]

Good that you noticed, thanks! I'd say that we could still tag it onto #15 since it is an important fix. I totally agree that we need more unit cell tests and constructors, but maybe we can add them later since that takes a bit of time. (Time that I probably don't have at the moment ;-))

[codecov]

Codecov Report

Attention: Patch coverage is 78.43137% with 11 lines in your changes missing coverage. Please review.

Files	Coverage Δ
src/algorithms/peps_opt.jl	95.83% <ø> (ø)
src/states/abstractpeps.jl	70.00% <ø> (ø)
src/algorithms/ctmrg.jl	94.81% <0.00%> (ø)
src/states/infinitepeps.jl	65.75% <0.00%> (ø)
src/environments/ctmrgenv.jl	73.23% <81.63%> (+1.39%)	⬆️

... and 1 file with indirect coverage changes

[lkdvos]

I would actually argue the opposite. In principle, the tests run, and the released version is not affected by this problem. So only the master is currently in a slightly broken state, which is not that big of a problem as no-one should use that version anyways.

It's nice to have changes separated by commits, large PRs with multiple different changes are very hard to debug afterwards, and pinpointing where something went wrong is a bit of a nightmare when all we are left with is the big squashed commit...

So maybe, let's ignore the issue for PR #15 , and just merge this afterwards. It would then be nice to have this PR include:

• a CTMRGEnv constructor with variable spaces
• a 3x3 test that uses this and covers the error discovered here

I would suggest to re-use the same structure as the InfinitePEPS constructor:

CTMRG([f, T], peps::InfinitePEPS, chi_vertical::S, chi_horizontal::S=chi_vertical; unitcell::Tuple{Int,Int}=(1,1)) where S<:Union{Int,ElementarySpace}
CTMRG([f, T], D_horizontal::S, D_vertical::S, chi_vertical::S, chi_horizontal::S=chi_vertical; unitcell::Tuple{Int,Int}=(1,1)) where S<:Union{Int,ElementarySpace}
CTMRG([f, T], peps::InfinitePEPS, chis_vertical::A, chis_horizontal::A=chis_vertical) where {A<:AbstractMatrix{<:Union{Int,ElementarySpace}}}
CTMRG([f, T], Ds_horizontal::A, Ds_vertical::A, chis_vertical::A, chis_horizontal::A=chis_vertical) where {A<:AbstractMatrix{<:Union{Int,ElementarySpace}}}

In an ideal world, it would be cool if we could also allow the peps bond dimensions to be different for the bra and the ket, which enables things like VOMPS -- approximating a peps by a different one with a different bonddimension, or some time-evolution algorithms as well.

[leburgel]

Okay, I think this ticks all the boxes now. Feels a bit clunky, but I don't immediately see how to make things more compact. So good to go from my end unless there are more comments.

[lkdvos]

I quickly glanced over this, looks good to me. Maybe @pbrehmer want's to have another look, but otherwise we can merge this.

[pbrehmer]

Looks lovely, I think it can be merged as it is!