Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement sorting for Eigen #24536

Closed
wants to merge 8 commits into from
Closed

Implement sorting for Eigen #24536

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a7340a0
Implement sort!(::Eigen)
Nov 8, 2017
468709f
Add tests
Nov 8, 2017
e5aa710
Remove redundant `Base.LinAlg.`
Nov 8, 2017
5af6876
Let default sorting be `eigsortby`
Nov 8, 2017
0594363
Add kw..., call sort in test
Nov 9, 2017
447ea8e
Define copy and refine docstrings for Eigen
Nov 9, 2017
c65c90f
Add (@ref)s and manual entries
Nov 9, 2017
53b0be2
More tests
Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove redundant Base.LinAlg.
  • Loading branch information
Evey Dee committed Nov 8, 2017
commit e5aa7109819fe6eb9f296a1200e2430b5711972d
4 changes: 2 additions & 2 deletions base/linalg/eigen.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ julia> F[:values]
4.0
```
"""
function sort!(F::Base.LinAlg.Eigen; kw...)
function sort!(F::Eigen; kw...)
perm = sortperm(F[:values]; kw...)
Copy link
Member

@stevengj stevengj Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we need a default by=eigsortby, if by wasn't supplied in kw, using e.g. the eigsortby function from #21598.

Otherwise it will fail with no method matching isless for complex eigenvalues.

Copy link
Contributor Author

@dalum dalum Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually intentional, but maybe the error message is confusing? I wanted to avoid choosing any particular sorting order for complex eigenvalues, since I generally need by=angle and others might want real, imag or eigsortby.

Copy link
Contributor Author

@dalum dalum Nov 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also see a good argument for having some default, and eigsortby is a pretty good candidate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be done by dispatching on the eltype of the eigenvalues, defaulting to reim, without the need to define eigsortby.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely the caller should be able to supply their own by, but I think there should be a default, and reim seems like a good a choice for complex values (since it will match the sorting of real values in the case of complex numbers that are purely real).

permute!(F[:values], perm)
Base.permutecols!!(F[:vectors], perm)
return F
end

sort(F::Base.LinAlg.Eigen; kw...) = sort!(deepcopy(F), kw...)
sort(F::Eigen; kw...) = sort!(deepcopy(F), kw...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worthwhile to define a copy method for Eigen. Such methods exists for e.g. Cholesky and LU factorizations. Something like:

Base.copy(F::Eigen) = Eigen(copy(F.values), copy(F.vectors))


isposdef(A::Union{Eigen,GeneralizedEigen}) = isreal(A.values) && all(x -> x > 0, A.values)

Expand Down