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
Prev Previous commit
Next Next commit
Add (@ref)s and manual entries
  • Loading branch information
Evey Dee committed Nov 9, 2017
commit c65c90f2f5e2ffcb08d2f75b0317e6f842984a27
7 changes: 4 additions & 3 deletions base/linalg/eigen.jl
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ 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 `sort` for a variant that returns a sorted copy leaving `F` itself unmodified.
for comparison. See [`sort`](@ref) 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...)
Expand All @@ -49,8 +50,8 @@ 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.
comparison. See [`sort!`](@ref) for a variant that sorts `F` in place. See
[`sort(::AbstractVector)`](@ref) 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
Expand Down
2 changes: 2 additions & 0 deletions doc/src/stdlib/linalg.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ Base.LinAlg.eigs(::Any, ::Any)
Base.LinAlg.svds
Base.LinAlg.peakflops
Base.LinAlg.stride1
Base.sort(::Base.LinAlg.Eigen)
Base.sort!(::Base.LinAlg.Eigen)
```

## Low-level matrix operations
Expand Down
4 changes: 2 additions & 2 deletions doc/src/stdlib/sort.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ can be specified via the `lt` keyword.
## Sorting Functions

```@docs
Base.sort!
Base.sort
Base.sort!(::AbstractVector)
Base.sort(::AbstractVector)
Base.sortperm
Base.Sort.sortperm!
Base.Sort.sortrows
Expand Down