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
Define copy and refine docstrings for Eigen
  • Loading branch information
Evey Dee committed Nov 9, 2017
commit 447ea8e363be9a50256d3b48dbd79926260d951f
39 changes: 24 additions & 15 deletions base/linalg/eigen.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,37 +34,44 @@ eigsortby(λ::Complex) = reim(λ)
sort!(F::Base.LinAlg.Eigen; kw...)

Sort the eigenvectors and eigenvalues of an eigenfactorization `F` in place using the eigenvalues
for comparison. See also `sort!(::AbstractVector)`.
for comparison. See `sort` for a variant that returns a sorted copy leaving `F` itself unmodified.
"""
function sort!(F::Eigen; by=eigsortby, kw...)
if !issorted(F[:values], by=by, kw...)
perm = sortperm(F[:values]; by=by, kw...)
permute!(F[:values], perm)
Base.permutecols!!(F[:vectors], perm)
end
return F
end

"""
sort(F::Base.LinAlg.Eigen; kw...)

Sort the eigenvectors and eigenvalues of an eigenfactorization `F` using the eigenvalues for
comparison. See `sort!` for a variant that sorts `F` in place. See `sort(::AbstractVector)` for a
description of possible keyword arguments.

# Examples
Copy link
Member

Choose a reason for hiding this comment

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

Should the example perhaps be attached to the sort method instead? It is a fairly common pattern throughout the documentation to have a more extensive docstring for the non-inplace method, and just reference that there is an inplace equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense. But for sort(::Vector), it actually references sort! as the extensive docstring. Maybe this should be changed?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay, I guess we don't have an official policy, in any case it is unnecessary to repeat information, so one or the other should have a more extensive docstring. Perhaps you can simply add a short docstring for the non-inplace version instead then, and reference this. I guess most of the time you want to use sort! anyway.

Also, why the reference to sort!(::AbstractVector)? For docs about the keywords?

```jldoctest
julia> F = eigfact([4.0 1.0; 0 1.0])
julia> F1 = eigfact([4.0 1.0; 0 1.0])
Base.LinAlg.Eigen{Float64,Float64,Array{Float64,2},Array{Float64,1}}([4.0, 1.0], [1.0 -0.316228; 0.0 0.948683])

julia> F[:values]
julia> F1[:values]
2-element Array{Float64,1}:
4.0
1.0

julia> sort!(F)
julia> F2 = sort(F1)
Base.LinAlg.Eigen{Float64,Float64,Array{Float64,2},Array{Float64,1}}([1.0, 4.0], [-0.316228 1.0; 0.948683 0.0])

julia> F[:values]
julia> F2[:values]
2-element Array{Float64,1}:
1.0
4.0
```
"""
function sort!(F::Eigen; by=eigsortby, kw...)
if !issorted(F[:values], by=by, kw...)
perm = sortperm(F[:values]; by=by, kw...)
permute!(F[:values], perm)
Base.permutecols!!(F[:vectors], perm)
end
return F
end

sort(F::Eigen; kw...) = sort!(deepcopy(F), kw...)
sort(F::Eigen; kw...) = sort!(copy(F), kw...)

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

Expand Down Expand Up @@ -474,3 +481,5 @@ convert(::Type{AbstractMatrix}, F::Eigen) = F.vectors * Diagonal(F.values) / F.v
convert(::Type{AbstractArray}, F::Eigen) = convert(AbstractMatrix, F)
convert(::Type{Matrix}, F::Eigen) = convert(Array, convert(AbstractArray, F))
convert(::Type{Array}, F::Eigen) = convert(Matrix, F)

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