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 96.22642% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pullbacks/eigh.jl	95.71%	3 Missing ⚠️
src/pullbacks/eig.jl	97.40%	2 Missing ⚠️
src/pullbacks/svd.jl	91.66%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/pullbacks/svd.jl	92.70% <91.66%> (+0.07%)	⬆️
src/pullbacks/eig.jl	92.72% <97.40%> (+0.01%)	⬆️
src/pullbacks/eigh.jl	86.13% <95.71%> (+2.99%)	⬆️

🚀 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]

Suggested change

	if indV == 1:p
	ΔV₁ = copy(ΔV)
	else
	ΔV₁ = zero(V)
	for (j, i) in enumerate(indV)
	ΔV₁[:, i] .= view(ΔV, :, j)
	end
	end
	ΔV₁ = similar(V)
	ΔV₁[:, indV] = ΔV
	zero!(view(ΔV₁, :, (length(indV) + 1):p))

I think this ends up doing the same, I just had a hard time parsing what was going on before

[lkdvos]

Suggested change

	if indV == 1:p
	ΔV₁ = copy(ΔV)
	else
	ΔV₁ = zero(V)
	for (j, i) in enumerate(indV)
	ΔV₁[:, i] .= view(ΔV, :, j)
	end
	end
	ΔV₁ = similar(V)
	ΔV₁[:, indV] = ΔV
	zero!(view(ΔV₁, :, (length(indV) + 1):p))

same comment here

[lkdvos]

Not really relevant for this PR, but I think this has come up somewhere before as well. Do you think we could benefit from a project_hermitian!(C, A, alpha, beta) approach, or is that really overkill?