fix: LQ pullback modifies input cotangents

[lkdvos]

Bumped into this to resolve QuantumKitHub/TensorKit.jl#419.

TLDR: the QR pullback correctly did not modify the input cotangents, using a getindex, while the LQ pullback uses a view.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/pullbacks/lq.jl	96.62% <100.00%> (ø)
src/pullbacks/qr.jl	95.55% <ø> (-0.05%)	⬇️

🚀 New features to boost your workflow:

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

[kshyatt]

Could we add an explicit test to make sure the cotangents are not touched?

[Jutho]

Thanks this looks good.

This makes me however wonder to what extent we can rely on getindex returning a copy. If we ever switch over the TensorKit blocks to return a StridedView instead of a reshape(view(...)), we might run into problem, because getindex producing a view on StridedView objects has been a (possibly misinformed) design principle from early on. Maybe we should be more explicit with copy(view(...)) ?

[lkdvos]

@Jutho I have switched everything to copy(view(...)), but I think this is more a case of Strided performing piracy on a Base method by altering its semantics than MatrixAlgebraKit not being able to rely on this. It is a good comment though, this would be an annoying thing to debug in the future...

@kshyatt Good point, should be done now :).
Interestingly, my first idea was that this should be caught by the AD testing frameworks, but that just isn't the case because of different requirements: Mooncake actually requires us to zero out these output tangents, so that would never catch this (or be affected by this), and adding this to ChainRulesTestUtils seems like it will be hard to find someone to merge this...
This also brings up the point that if we end up dropping chainrules in the end, it would actually be more efficient to assume we can modify these inputs, but I guess we can keep that for the future 😉