Sort diagonal eig values and standardize output types
#151
Conversation
|
Indeed, I thought they were sorted, but it is Julia's LinearAlgebra that sorts them; the default LAPACK output is unsorted. Is that than not the reason why we only implemented this for |
|
I don't really know, I'm fine with either since I don't think this is ever all that useful, but it's definitely not intuitive that we do explicitly sort some but not all cases. In principle I can also try and come up with a more general interface for sorting as a post processing step which we then use everywhere, but the fact that the output type of the diagonal decompositions changes depending on whether or not we sort the values feels a bit off still. |
|
So I slightly changed the implementation - the idea is that Diagonal eigenvalue decompositions should just work and more or less follow the output types of the regular ones. |
lkdvos
changed the title
eig valueseig values and standardize output types
| function check_input(::typeof(eig_full!), A::AbstractMatrix, DV, ::DiagonalAlgorithm) | ||
| m, n = size(A) | ||
| @assert m == n && isdiag(A) | ||
| ((m == n) && isdiag(A)) || throw(DimensionMismatch("diagonal input matrix expected")) |
There was a problem hiding this comment.
Can we print the actual dimensions here with a lazy string?
| function check_input(::typeof(eig_vals!), A::AbstractMatrix, D, ::DiagonalAlgorithm) | ||
| m, n = size(A) | ||
| @assert m == n && isdiag(A) | ||
| ((m == n) && isdiag(A)) || throw(DimensionMismatch("diagonal input matrix expected")) |
|
I will merge and address the comments in a follow-up! |
This was already changed for
eigh, and I now made this consistent.However, while playing around with this I have to say that I don't think we are sorting the LAPACK factorizations either, so this seems inconsistent with that now.
I then browsed through what LinearAlgebra does, and they very explicitly mention that for Diagonal inputs
sortbymight not be supported, and the sorting of the eigenvalues is not consistent betweenDandMatrix(D) + eps(note that the eps is necessary since LinearAlgebra does a runtime check for diagonal, therefore leading to the incredibly stupid difference between a diagonal eigendecomposition and a matrix epsilon away from that)