Use TestSuite for eig

[kshyatt]

Made some changes to the GenericSchur wrappers so that input DV to eig_full! actually does get modified, for example.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
see 3 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[lkdvos]

Except for the comment about the copy and the === tests I think this is also good for me

[kshyatt]

Previously all our tests were checking that that D, V you get out of the ! methods is the same D, V that went in, in terms of memory location. That might be worth holding on to in case people are eg passing in a SubArray for D and V?

…

On Tue, Dec 23, 2025 at 5:56 PM Lukas Devos ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ext/MatrixAlgebraKitGenericSchurExt.jl <#130 (comment)> : > + if !isnothing(D) + copyto!(D, Diagonal(D_)) + else + D = D_ + end + if !isnothing(V) + copyto!(V, V_) + else + V = V_ + end I'm still not sure how I feel about this. I would still think our interface specifies that we "may" use the input arrays, and since the D_, V_ is already allocated anyways we might just return that and not use the extra copy? If this is to make the tests work, then the previous tests were relying on behavior that is not required by the interface and so that should definitely not be tested. — Reply to this email directly, view it on GitHub <#130 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGKJYYL5FCX4QM22J2QGJD4DFX5HAVCNFSM6AAAAACP3B7D7KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMBYG4ZDENBQGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Jutho]

While I agree that we do not commit to actually using the given output arguments in our current interface specification, I am starting to wonder if that was a mistake. It becomes very hard to draw a line. There are various ...ViaTransposed... methods, where the final step also involves something like adjoint!(given_output_argument, newly_allocated_output_of_transposed_problem). This is of the same form as a copy!, so why are we doing this and not simply return adjoint(newly_allocated_output_of_transposed_problem), thereby discarding given_output_argument.

I do think type stability is a good reason. In that case, it would be fairly obvious that that the two approaches yield different output types. But indeed, as pointed out by @kshyatt , also in the more subtle cases that do not involve an adjoint! but only a copy!, there is a difference if the given output matrix is not a simple Matrix but some SubArray or ... . So now we have output types that depend on the chosen algorithm? Is that desirable?

I don't think including non-mutable types in that discussion is really valid; if something like StaticArrays are ever supported, they should not go via the bang methods altogether.

[Jutho]

I guess the problem partially stems from the trunc methods, where the provided "output" matrices serve more as intermediate workspace, since you cannot provide the actual output matrices if you don't know their size yet. Maybe this has been a bit of an abuse of our own interface, where these arguments indeed serve more as temporary workspace.

[lkdvos]

I think it would indeed be a good idea to rethink our interface and re-evaluate what we want to do with that.
I do think I agree that we might have to tweak the current ideas slightly, based on having a bit more different implementations etc.

This being said, the current state is pretty clear, all our docstrings explicitly state that we do not guarantee that the provided memory will be used, which is reflected by the GenericLinearAlgebra implementations not having this.
Basically, we are using a !!-style interface right now (and we could decide to be explicit about that if we want to).
Therefore, at least for the current state, these === tests are technically wrong, and if you provide a SubArray you have to manually check whether or not the output actually egals the provided output and act accordingly.
This is also what we do in TensorKit: see e.g. here

To repeat, I'm not saying that this is necessarily the best solution, and we really should have a proper discussion about the interface after the holidays.
However, I do think for now we can continue like this, as this is not something that has to be tackled in this (set of) PRs?

---

One detail I want to mention too is that the current design isn't only because of f_trunc, but also because of ChainRules.jl, and reverse AD in general.
In that context, the fact that we are allowed to treat the provided output memory as workspace and aren't guaranteeing anything is crucial to enforce correct derivatives with Zygote, since I think a pattern like the code snippet below is "undefined behavior" according to our current interface, and would indeed yield wrong results with Zygote.

Q, R = qr_compact(A)
# some intermediate steps
# ...

# Wrong: reusing Q and R here, assuming result will be stored in `Q` and `R`
qr_compact!(A, (Q, R), ...)
return Q, R

# Correct: reusing Q and R here but not assuming where the result is stored:
Q, R = qr_compact!(A, (Q, R), ...)
return Q, R

In that sense, I somewhat like that this isn't just a bug that would only show up when performing AD, and rather already is wrong behavior with some of the algorithms.

Similar arguments hold for Mooncake too in principle, as we can greatly simplify some of these implementations just by allowing them to be not in-place, avoiding the need to store the intermediates etc.
(Im not too sure if we have enough control over Enzyme to use this though, I don't know enough about that)

[kshyatt]

I also think if we want to modify these guarantees, it's a bit of a separate issue from this PR (e.g. we can always modify things later to enforce the guarantee and update the tests if/when we do so)