Implement schur_full for Diagonal inputs, deprecate schur_vals - #277
Merged
Conversation
Schur reuses eig's algorithm selection, which routes `Diagonal` to `DiagonalAlgorithm`, but no such implementation existed, so both threw a `MethodError`. A diagonal matrix is already in Schur form: `T = A`, `Z = I` and `vals = diagview(A)`, without any reordering. Fixes #276 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Member
|
What is the rational for |
Member
Author
|
No rationale... I forgot we had this whole discussion with the eigenvalues as well, I'll change it |
The eigenvalues of a `Diagonal` are now complex, as for any other input and as `eig_vals` already does. That makes `schur_vals!`'s `check_input` and `initialize_output` identical to those of `eig_vals!` for every algorithm, so they simply forward, as `copy_input` already did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LAPACK's `gees` balances with permutation only, where `geev` also scales, so `schur_vals` returns the eigenvalues of `eig_vals(A; scale = false)`. On a matrix graded over 1e12 that is a relative error of 33 against 1e-15 for `eig_vals`, and `gees` is no faster: it computes the Schur form where `geev` asks `hseqr` for eigenvalues only. This answers the `# TODO: is this useful? Is there any difference with simply eig_vals?` that stood above `schur_vals`. `schur_full` keeps returning the eigenvalues it gets from `gees` alongside `T` and `Z`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lkdvos
changed the title
Member
Author
|
After some reconsideration I decided to just deprecate the |
Jutho
approved these changes
Aug 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #276:
Diagonalinput selectsDiagonalAlgorithm, which schur never implemented, soschur_fullthrew aMethodError.A diagonal matrix is already in Schur form, so
T = A,Z = I(keptDiagonal, asqr/lqkeepQandR) andvals = diagview(A), complexified as everywhere else. One choice worth flagging: no reordering, unlike theDiagonalpath ofeig, which sorts.Also deprecates
schur_vals/schur_vals!in favour ofeig_vals/eig_vals!, answering the# TODO: is this useful? Is there any difference with simply eig_vals?that stood above it. There is a difference, and it is not in schur's favour: LAPACK'sgeesbalances with permutation only wheregeevalso scales, soschur_valsreturns exactlyeig_vals(A; scale = false). On a matrix graded over 1e12 that is a relative error of 33 against 1e-15 foreig_vals, and it is no faster —geescomputes the Schur form wheregeevaskshseqrfor eigenvalues only.schur_fullstill returns thegeeseigenvalues alongsideTandZ. Callers ofschur_vals(A; expert = true)need to drop the keyword;eig_valshas no equivalent.Enables the
Diagonalschur tests that were commented out as "not supported yet", CPU and CUDA. Note this alone does not fixschur_full(::DiagonalTensorMap): TensorKit omits schur from itsdefault_algorithmforwarding, so selection fails there for every tensor, diagonal or not. That needs a separate PR.🤖 Generated with Claude Code