Fix splitdims(::BlockSparseArrays)

[ogauthe]

Fixes #50

Non blocksorted axes were used to index an array whose axes are blocksorted. Therefore the intermediary arrays carries invalid blocks. This is a quickfix that returns the correct result, but the code can be improved.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.11%. Comparing base (711dc43) to head (1ddbfb4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   75.09%   75.11%   +0.02%     
==========================================
  Files          30       30              
  Lines        1108     1109       +1     
==========================================
+ Hits          832      833       +1     
  Misses        276      276              

Flag	Coverage Δ
docs	24.81% <0.00%> (-0.03%)	⬇️

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]

A few comments:

• Here, we end up doing the same operations as in GradedUnitRanges.blockmergesort with similar code. I also do not like having to rely on GradedUnitRanges implementation details here. I think we could avoid both code duplication and dealing with internals by defining GradedUnitRanges.blockpermute(a::AbstractBlockedUnitRange, indices::AbstractVector{<:Block{1}}) to return a range with permuted blocks.
• Currently splitdims uses a flat tuple of axes and check lengths to block it. We may replace this logic with a BlockedTuple of ranges.
• as mentioned in the comments, we should only use views of the blocks instead of copying. A naive a_unblocked = @view a[sorted_axes...] still errors.
• we should have explicit test for fusedims and splitdims to check these tricky cases before they appear buried in contract. I think they should be here in BlockSparseArrays.

[mtfishman]

Thanks for investigating this and finding a fix! I guess basically it was using the inverse of the correct permutation? That would explain why it didn't come up yet, since for simpler permutations the permutation and its inverse are the same.

• Here, we end up doing the same operations as in GradedUnitRanges.blockmergesort with similar code. I also do not like having to rely on GradedUnitRanges implementation details here. I think we could avoid both code duplication and dealing with internals by defining GradedUnitRanges.blockpermute(a::AbstractBlockedUnitRange, indices::AbstractVector{<:Block{1}}) to return a range with permuted blocks.

I think it could be reasonable to make functionality like:

• GradedUnitRanges.blockmergesortperm
• GradedUnitRanges.blockmergesort
• GradedUnitRanges.blocksortperm
• GradedUnitRanges.blocksort (not defined yet but probably should be).

part of the public interface of GradedUnitRanges.jl, since sorting by sector and merging common sectors is a pretty common operation. However, maybe they should be renamed to:

• GradedUnitRanges.sectormergesortperm
• GradedUnitRanges.sectormergesort
• GradedUnitRanges.sectorsortperm
• GradedUnitRanges.sectorsort

to make it clearer that the sorting/merging is done by sector. I don't think there is a need for GradedUnitRanges.blockpermute(a::AbstractBlockedUnitRange, indices::AbstractVector{<:Block{1}}) since if I understand what you mean by that, that is already defined through indexing syntax like a[indices], or to get it as a new axis you can do only(axes(a[indices])) as I suggest in the comment above.

• Currently splitdims uses a flat tuple of axes and check lengths to block it. We may replace this logic with a BlockedTuple of ranges.

I think that makes sense, that should make the logic clearer.

• as mentioned in the comments, we should only use views of the blocks instead of copying. A naive a_unblocked = @view a[sorted_axes...] still errors.

Agreed.

• we should have explicit test for fusedims and splitdims to check these tricky cases before they appear buried in contract. I think they should be here in BlockSparseArrays.

Agreed, I see a test of fusedims/splitdims here:

BlockSparseArrays.jl/test/test_gradedunitrangesext.jl

Lines 89 to 110 in 711dc43

	@testset "fusedims" begin
	d1 = gradedrange([U1(0) => 1, U1(1) => 1])
	d2 = gradedrange([U1(0) => 1, U1(1) => 1])
	a = BlockSparseArray{elt}(d1, d2, d1, d2)
	blockdiagonal!(randn!, a)
	m = fusedims(a, (1, 2), (3, 4))
	for ax in axes(m)
	@test ax isa GradedOneTo
	@test blocklabels(ax) == [U1(0), U1(1), U1(2)]
	end
	for I in CartesianIndices(m)
	if I ∈ CartesianIndex.([(1, 1), (4, 4)])
	@test !iszero(m[I])
	else
	@test iszero(m[I])
	end
	end
	@test a[1, 1, 1, 1] == m[1, 1]
	@test a[2, 2, 2, 2] == m[4, 4]
	@test blocksize(m) == (3, 3)
	@test a == splitdims(m, (d1, d2), (d1, d2))
	end

but I guess it was too simple to catch this issue.

[ogauthe]

I guess basically it was using the inverse of the correct permutation? That would explain why it didn't come up yet, since for simpler permutations the permutation and its inverse are the same.

It was using permuted axes to read the input array, instead of reading with no permutation, then permute to write. It does not show put for trivial permutation indeed.

[mtfishman]

Looks good to me, thanks! Can you bump the version so we can register it after merging?