make ind functions consistent in gauge removal

[lkdvos]

Bumped into this while trying to port this over to TensorKit:
Other parts of the code use ind = Colon() as default, and then first take indV = axes(V, 2)[ind] before doing the check of the length (length(ind) fails if ind = Colon()).
Here I just did the same.

I checked for the SVD gauge removal and was surprised that there is no ind argument there, @Jutho is this meant to be like this?

[codecov]

Codecov Report

❌ Patch coverage is 0% with 153 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pullbacks/eig.jl	0.00%	74 Missing ⚠️
src/pullbacks/eigh.jl	0.00%	67 Missing ⚠️
src/pullbacks/svd.jl	0.00%	12 Missing ⚠️

Files with missing lines	Coverage Δ
src/pullbacks/svd.jl	0.00% <0.00%> (-92.64%)	⬇️
src/pullbacks/eigh.jl	0.00% <0.00%> (-83.15%)	⬇️
src/pullbacks/eig.jl	0.00% <0.00%> (-92.71%)	⬇️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[Jutho]

Let me continue on this PR, there are some more inconsistencies in how ind is treated also in the eig and eigh pullbacks. This should perhaps have been part of the previous PR, but it got merged quickly to fix the svd_full case. I will also switch over the eig and eigh pullbacks to this prepare_and_check_cotangents format, and at an ind argument in the remove_svd_gauge_dependence .

[Jutho]

Upon some consideration, I think it is actually easier to remove the ind argument in the remove_x_gauge_dependence! functions. I think in these functions it is fine to assume that the V and ΔV passed in have the same number of columns. The ind is mostly an internal implementation detail that needs to be intercepted for TruncatedAlgorithm decompositions.

[lkdvos]

That's definitely also okay for me

[Jutho]

Ok, this is a first set of changes. It makes the eigh pullback related functionality in line with the svd one, but also fixes some things in the svd pullbacks that went unnoticed. In particular, the gauge check rewritten in terms of a broadcast operation to avoid allocations had the arguments in the ternary operator ? : reversed, so that it was plain wrong and a lot of gauge warnings were printed, even after applying remove_gauge_dependence. This should now be fixed.

[Jutho]

Remains to be done: the eig case, which I will tackle next.

[Jutho]

Ok, I finally found the time to complete this. I hope the tests pass. Then this is ready for a review @lkdvos .

[lkdvos]

I think I only have a small comment about some aesthetics, otherwise I think this looks good, thanks!

[lkdvos]

(I can't approve this because I started the PR, so will need approval from someone else)

[Jutho]

Tests seem to be failing so don't merge; I'll have to look at it.

[lkdvos]

I might have found the fix :)

[lkdvos]

Tests failures on AMD seem unrelated, so will just merge as-is.

[Jutho]

I would still like to keep some of the old behaviour, so maybe this is a compromise

Suggested change

	ΔV₁ = zero(V)
	ΔV₁[:, indV] = ΔV
	if indV == 1:p
	ΔV₁ = copy(ΔV)
	else
	ΔV₁ = zero(V)
	ΔV₁[:, indV] .= ΔV
	end

[Jutho]

Same:

Suggested change

	ΔV₁ = zero(V)
	ΔV₁[:, indV] = ΔV
	if indV == 1:p
	ΔV₁ = copy(ΔV)
	else
	ΔV₁ = zero(V)
	ΔV₁[:, indV] .= ΔV
	end

[Jutho]

Oh, I only now noticed this is merged already.