-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: sort eigenvalues in a canonical order #21598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But only after 0.6 has been branched.
base/linalg/diagonal.jl
Outdated
eigvals(D::Diagonal{<:Number}) = D.diag | ||
eigvals(D::Diagonal) = [eigvals(x) for x in D.diag] #For block matrices, etc. | ||
eigvals(D::Diagonal{<:Number}) = sort(D.diag, by=eigsortby) | ||
eigvals(D::Diagonal) = sort!(vcat((eigvals(x) for x in D.diag)...),by=eigsortby) #For block matrices, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like
sort!(mapreduce(eigvals, vcat, D.diag), by=eigsortby)
be any more efficient, as it avoids the splatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be worse, I think, because it would call vcat
pairwise over and over and generate lots of temporary arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I forgot about the pairwise thing. Sorry for the noise.
base/linalg/bidiag.jl
Outdated
eigvals(M::Bidiagonal) = M.dv | ||
function eigvecs(M::Bidiagonal{T}) where T | ||
eigvals(M::Bidiagonal) = sort(M.dv, by=eigsortby) | ||
function eigvecs_(M::Bidiagonal{T}) where T # non-sorted eigenvectors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be a fallback for eigvecs that does the right thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the fallback calls eigfact(M)[:vectors]
, which gets the sorted eigenvectors.
Is calling lapack functions directly (which won't work for all array types) the only way here that an expert user could avoid the sorting overhead if they had an application that involved many small matrices and didn't need an ordering? |
@tkelman, yes. |
That's an unfortunate loss of generality then. If wanting an ordered output is primarily for pedagogical purposes, then I think it would be worth having a more convenient way of opting out of the sorting overhead. Other than the dense lapack types, do our other implementations also return unsorted results right now? If not, the sorting should maybe only be put in the code path for those dense types, rather than in the generic paths. |
I have been wondering the same thing but I've come to the conclusion that the generality isn't in demand here. As already mentioned, the symmetric solver in LAPACK sorts, see line 536 and I'm not aware of any complaints about that. Also, my guess is that the general eigenvalue problem is mainly for teaching and exploration. People who are really interested in performance and accuracy would/should use the Schur form anyway. |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
Maybe we shouldn't sort eigenvalues for the diagonal, bidiagonal, and triangular special cases where they can be just read off the diagonal? |
That's making my concern about generic code even worse. If we sort, we should do it predictably as part of the documented contract of what the generic |
I'd like to avoid adding a keyword argument if possible. We already have a generic function for sorting and a type for the eigendecomposition so would it make sense to overload Another thing is that it might be a bit weird to error on |
Adding a keyword argument isn't a big deal, and it's easier to discover than |
Sorting by default with a keyword to opt out in the rare case that it is too expensive to sort seems like a good balance to me. |
I updated this PR for I also changed the documentation to say that |
This seems to be working now. (Travis CI failure on MacOS is #30949.) @andreasnoack, @StefanKarpinski, can we consider merging this? |
Seems good to me! |
As discussed in #15734, this PR sorts the eigenvalues (and corresponding eigenvectors) in a canonical order: in ascending order for real eigenvalues (consistent with what LAPACK does automatically in the Hermitian case, for which no sorting is required), and in ascending order lexicographically by
(real(λ),imag(λ))
for complex eigenvalues (chosen so that it is consistent with the real case when the eigenvalues are real).The basic principle here is that virtually any consistent, comprehensible order is preferable to random ordering, and the cost of the sorting is negligible for matrices of any significant size. (I measured a 15% overhead for a 10x10 complex matrix, and for bigger matrices it quickly becomes unobservable.)
Along the way, I noticed thatMoved to #30681eigvals(::Diagonal{Matrix})
seemed wrong: it returned an array of arrays of eigenvalues, rather than an array of eigenvalues (which are still well-defined scalars for block matrices, not vectors). I changed it, and sorted it to be consistent with othereigvals
routines.(I implemented an internal
permutecols!!
function to permute the columns of a matrix in-place with a given permutation vector. I tried the alternative of permuting the rows one by-one usingpermute!!
on a view, but it was many times slower.)See also #17093, #9655, #8441.
Updates:
As per the comments below, I added a
sortby
keyword to the variouseig
functions. If you passsortby=nothing
, it suppresses sorting. If you pass anotherby
function, e.g.sortby=abs
, that changes the sorting accordingly.I also changed the documentation to say that
sortby
may not be a keyword for special matrix types likeSymTridiagonal
that may implement their own sorting convention.SymTridiagonal
already didn't accept the othereigen
keywords likepermute
andscale
, and allowing special matrices to decide on their preferred eigenvalue ordering seemed like the most sensible and conservative approach to me.