Generalize blockedperm ellipsis inputs, change constructor names

[ogauthe]

This PR changes the behavior of blockedperm when an Ellipsis appears in the arguments. The inferred elements are now merged into one single block. The objective is to have blockedperm(.., (4, 3)) always returning a BlockedPermutation{2}, and more generally to easily predict blocklength(blockdedperm(input)).

It also makes blockedperm fully type stable when length is provided as a Val.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.60%. Comparing base (f67770f) to head (673dd2b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   93.95%   94.60%   +0.65%     
==========================================
  Files          15       15              
  Lines         364      371       +7     
==========================================
+ Hits          342      351       +9     
+ Misses         22       20       -2     

Flag	Coverage Δ
docs	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[ogauthe]

This PR changes the behavior of fusedims(a, (3, 1), ..), which now always returns an AbstractMatrix. I think this new behavior is indeed better and paves the way for #22.

[mtfishman]

I can see why the new definition is helpful for FusionTensors, but I think the reason for the old definition was because there are cases where you want to fuse some subset of indices and leave the rest alone (say for example doing a "tensor network renormalization", say by contracting pairs of neighboring MPS tensors together and fusing the site indices into one "renormalized" site index).

A proposal I can think of to allow users to choose between the two behaviors could be:

blockedperm(.., (4, 3)) # ((1,), (2,), (4, 3))
blockedperm((..,), (4, 3)) # ((1, 2), (4, 3))

Then, fusedims can have two modes:

fusedims(a, (3, 1), ..) # Only fuse dimensions 3 and 1 together, leave the rest unfused
fusedims(a, (3, 1), (..,)) # Fuse dimensions 3 and 1 together and fuse the rest with each other

[ogauthe]

I now allow both Ellipsis and Tuple{Ellipsis} as input, with different behaviors.

[ogauthe]

blockedperm(::AbstractBlockedTuple) now supports a length kwarg to support generic code (I needed it while working on fusedims). I added a check for consistent lengths.

@mtfishman this is ready to merge

[mtfishman]

Besides the comments I've left, this looks good to me.

The only other thing I can think of is that we probably will want to support blockedperm((5, 4), (.., 1)) == blockedperm((5, 4), (2, 3, 1)), but that could be left for future work. (Though see my comment above about how the notation should probably be permmortar(((5, 4), (.., 1))) to be consistent with BlockArrays.jl.)

I forgot about this issues with the testing which we should sort out before merging this.

[ogauthe]

I forgot about this issues with the testing which we should sort out before merging this.

I am not sure to understand what has to be done. This PR defines breaking changes for blockedperm and is therefore labelled as breaking release. There are therefore dependency compatibility issues that are solved by ITensor/NamedDimsArrays.jl#43 and ITensor/BlockSparseArrays.jl#56. Tests locally pass with these PR together. However I do not know how to run GitHub actions while combining PR on different projects.

[mtfishman]

I think the heart of the issue is that some of the tests in this package make use of BlockSparseArrays.jl (https://github.com/ITensor/TensorAlgebra.jl/blob/f67770f305b8fe9605c8e3091976ad1a1dc632b5/test/test_gradedunitrangesext_contract.jl, https://github.com/ITensor/TensorAlgebra.jl/blob/f67770f305b8fe9605c8e3091976ad1a1dc632b5/test/test_blockarrays_contract.jl), which can't be installed because of the version bump in this PR. So I think we should move those tests to the BlockSparseArrays.jl repository. That is a better place for them anyway since that is where the package extension connecting TensorAlgebra.jl and BlockSparseArrays.jl (BlockSparseArraysTensorAlgebraExt) is defined.

[mtfishman]

Basically, we just want to make sure that the tests of a package A.jl (in this case, TensorAlgebra.jl) don't make use of packages that depend on package A.jl, since otherwise if we make breaking releases to A.jl we won't be able to run the tests. Downstream tests are a different story, if we make a breaking release to A.jl, downstream tests won't run but we are ok with that, since we know we'll have to update the downstream tests in subsequent PRs.

[ogauthe]

I removed BlockSparseArrays from the test dependencies and move the test to BlockSparseArrays.jl in ITensor/BlockSparseArrays.jl#56. Since BlockSparseArrays is no more loaded, BlockArrays tests now pass, but the issue lingers.

[mtfishman]

Thanks, glad that fixed it. What issue still lingers?

[ogauthe]

Thanks, glad that fixed it. What issue still lingers?

BlockSpareseArrays changes the behavior of TensorAlgebra.contract(::BlockArrays.BlockArray. The current PR makes the broken tests introduced in #34 now pass, however this is not because it solves the issue that caused them. Since it does not import BlockSparseArrays any more, the trigger vanishes, but the core issue (discussed in ITensor/BlockSparseArrays.jl#57) lingers.

It think the 3 linked PR
#27 (this one)
ITensor/BlockSparseArrays.jl#56
ITensor/NamedDimsArrays.jl#43
are ready to merge. Tests for ITensor/NamedDimsArrays.jl#43 should pass once the 2 other PR are merged.

[mtfishman]

Got it, thanks for the reminder and summary.