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.

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

Implement sorting for Eigen #24536

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Implement sort!(::Eigen)
  • Loading branch information
Evey Dee committed Nov 8, 2017
commit a7340a0bdae7247af4b7d1169b01436784159299
29 changes: 29 additions & 0 deletions base/combinatorics.jl
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,35 @@ isperm(p::Tuple{}) = true
isperm(p::Tuple{Int}) = p[1] == 1
isperm(p::Tuple{Int,Int}) = ((p[1] == 1) & (p[2] == 2)) | ((p[1] == 2) & (p[2] == 1))

#swap columns i and j of a, in-place
function swapcols!(a::AbstractMatrix, i, j)
i == j && return
cols = indices(a,2)
(i in cols && j in cols) || throw(BoundsError())
for k in indices(a,1)
@inbounds a[k,i],a[k,j] = a[k,j],a[k,i]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

would be clearer with spaces after the outer commas

end
end
# like permute!! applied to each row of a, in-place in a (overwriting p).
function permutecols!!(a::AbstractMatrix, p::AbstractVector{<:Integer})
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

might be generally useful to have and export, along with corresponding permutecols!, permutecols, permuterows!!, permuterows! and permuterows (yikes, that's a lot variations).

count = 0
start = 0
while count < length(p)
ptr = start = findnext(!iszero, p, start+1)
next = p[start]
count += 1
while next != start
swapcols!(a, ptr, next)
p[ptr] = 0
ptr = next
next = p[next]
count += 1
end
p[ptr] = 0
end
a
end

function permute!!(a, p::AbstractVector{<:Integer})
count = 0
start = 0
Expand Down
34 changes: 34 additions & 0 deletions base/linalg/eigen.jl
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,40 @@ function getindex(A::Union{Eigen,GeneralizedEigen}, d::Symbol)
throw(KeyError(d))
end

"""
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)`.
Copy link
Member

Choose a reason for hiding this comment

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

Why point to sort!(::AbstractVector)? Other docstrings for sort! will be listed in the same place anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you do something like ?> sort!(F) to filter out all the noise. 🙂 Or in the manual.

Copy link
Member

Choose a reason for hiding this comment

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

Even then, it's not clear what this reference means: does this function support all keyword arguments accepted by sort!(::AbstractVector)? Is it just that they are similar? Also, the generic docstring is defined for any v, not just ::AbstractVector.

Copy link
Contributor Author

@dalum dalum Nov 9, 2017

Choose a reason for hiding this comment

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

You are right. It is because it accepts the same keyword arguments. I will change the wording to make it more clear. Also, AFAIU, the "generic" docstring is a fallback to all docstrings, but the one currently being shown is only for AbstractVector, despite the missing ::AbstractVector in the signature of the docstring.


# 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])
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]
2-element Array{Float64,1}:
4.0
1.0

julia> sort!(F)
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]
2-element Array{Float64,1}:
1.0
4.0
```
"""
function sort!(F::Base.LinAlg.Eigen; kw...)
perm = sortperm(F[:values]; kw...)
Copy link
Member

@stevengj stevengj Nov 8, 2017

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

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

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...)

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

"""
Expand Down
2 changes: 1 addition & 1 deletion base/linalg/linalg.jl
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import Base: USE_BLAS64, abs, acos, acosh, acot, acoth, acsc, acsch, adjoint, as
csch, eltype, exp, eye, findmax, findmin, fill!, floor, getindex, hcat, imag, indices,
inv, isapprox, isone, IndexStyle, kron, length, log, map, ndims, oneunit, parent,
power_by_squaring, print_matrix, promote_rule, real, round, sec, sech, setindex!, show, similar,
sin, sincos, sinh, size, sqrt, tan, tanh, transpose, trunc, typed_hcat, vec
sin, sincos, sinh, size, sort, sort!, sqrt, tan, tanh, transpose, trunc, typed_hcat, vec
using Base: hvcat_fill, iszero, IndexLinear, _length, promote_op, promote_typeof,
@propagate_inbounds, @pure, reduce, typed_vcat
# We use `_length` because of non-1 indices; releases after julia 0.5
Expand Down