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

Rename select* functions to partialsort* and various related fixes #23051

Merged
merged 3 commits into from
Aug 27, 2017
Merged

Rename select* functions to partialsort* and various related fixes #23051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3588102
Rename select* functions to partialsort*
nalimilan Jul 31, 2017
8150f66
Make partialsort!() and partialsortperm!() return a view rather than …
nalimilan Jul 31, 2017
7a54a2f
Fix docstrings for partialsortperm() and partialsortperm!()
nalimilan Aug 25, 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
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,9 @@ Deprecated or removed
respectively. Similarly, `MPFR.get_version()`, has been renamed to `MPFR.version()` ([#23323]). Also,
`LinAlg.LAPACK.laver()` has been renamed to `LinAlg.LAPACK.version()` and now returns a `VersionNumber`.

* `select`, `select!`, `selectperm` and `selectperm!` have been renamed respectively to
`partialsort`, `partialsort!`, `partialsortperm` and `partialsortperm!` ([#23051]).

Command-line option changes
---------------------------

Expand Down
6 changes: 6 additions & 0 deletions base/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,12 @@ end

@deprecate IOContext(io::IO, key, value) IOContext(io, key=>value)

# issue #22791
@deprecate select partialsort
@deprecate select! partialsort!
@deprecate selectperm partialsortperm
@deprecate selectperm! partialsortperm!

# END 0.7 deprecations

# BEGIN 1.0 deprecations
Expand Down
8 changes: 4 additions & 4 deletions base/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ export
ones,
parent,
parentindexes,
partialsort,
partialsort!,
partialsortperm,
partialsortperm!,
permute,
permute!,
permutedims,
Expand All @@ -517,17 +521,13 @@ export
searchsorted,
searchsortedfirst,
searchsortedlast,
select!,
select,
shuffle,
shuffle!,
size,
slicedim,
sort!,
sort,
sortcols,
selectperm,
selectperm!,
sortperm,
sortperm!,
sortrows,
Expand Down
87 changes: 49 additions & 38 deletions base/sort.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ import
export # also exported by Base
# order-only:
issorted,
select,
select!,
searchsorted,
searchsortedfirst,
searchsortedlast,
# order & algorithm:
sort,
sort!,
selectperm,
selectperm!,
sortperm,
sortperm!,
partialsort,
partialsort!,
partialsortperm,
partialsortperm!,
sortrows,
sortcols,
# algorithms:
Expand Down Expand Up @@ -82,20 +82,25 @@ issorted(itr;
lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward) =
issorted(itr, ord(lt,by,rev,order))

function select!(v::AbstractVector, k::Union{Int,OrdinalRange}, o::Ordering)
function partialsort!(v::AbstractVector, k::Union{Int,OrdinalRange}, o::Ordering)
inds = indices(v, 1)
sort!(v, first(inds), last(inds), PartialQuickSort(k), o)
v[k]

if k isa Integer
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps using dispatch for this logic would be better than branching?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I'd agree with you, but in the present case that would force duplicating the two lines above, that's why I chose the if approach.

BTW, there seems to be a lack in the API: with getindex, you can either get an array or a scalar, but with view/@view you always get a subarray. Shouldn't there be a way to get the same behavior as getindex? Then one wouldn't have to check types manually like this (which could be hard or impossible in more complex cases where the index can be of any type.)

Copy link
Member

Choose a reason for hiding this comment

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

Would not

function partialsort!(v::AbstractVector, k::Union{Int,OrdinalRange}, o::Ordering)
    inds = indices(v, 1)
    sort!(v, first(inds), last(inds), PartialQuickSort(k), o)
    return _dispatchftw(v, k)
end
_dispatchftw(v, k::Integer) = v[k]
_dispatchftw(v, k) = view(v, k)

do the trick? Could even reuse the helper functions for the same purpose below :).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that would work, but I don't find it clearer than the existing version, do you? To understand what the code does you need to look at the definitions of methods for _dispatchftw, which does not have a generic meaning on its own.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that would work, but I don't find it clearer than the existing version, do you?

Noting that my subjective evaluation is practically meaningless, yes, I prefer this style :).

Perhaps a more meaningful question would be whether one or the approach is friendlier to the compiler?

Copy link
Member

Choose a reason for hiding this comment

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

There is always the third option, with the same lines of code:

function partialsort!(v::AbstractVector, k::Int, o::Ordering)
    inds = indices(v, 1)
    sort!(v, first(inds), last(inds), PartialQuickSort(k), o)
    return v[k]
end
function partialsort!(v::AbstractVector, k::OrdinalRange, o::Ordering)
    inds = indices(v, 1)
    sort!(v, first(inds), last(inds), PartialQuickSort(k), o)
    return view(v, k)
end

Copy link
Member

Choose a reason for hiding this comment

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

The harsh reality is, not all lines of code are equal.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I'm voting for status quo or perhaps change it to using the ternary operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's go with the current version then. I've rebased again, let's not wait for too long as there were already substantial conflicts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, @views seems to do the trick. See #23760.

return v[k]
else
return view(v, k)
end
end

"""
select!(v, k, [by=<transform>,] [lt=<comparison>,] [rev=false])
partialsort!(v, k, [by=<transform>,] [lt=<comparison>,] [rev=false])

Partially sort the vector `v` in place, according to the order specified by `by`, `lt` and
`rev` so that the value at index `k` (or range of adjacent values if `k` is a range) occurs
at the position where it would appear if the array were fully sorted via a non-stable
algorithm. If `k` is a single index, that value is returned; if `k` is a range, an array of
values at those indices is returned. Note that `select!` does not fully sort the input
values at those indices is returned. Note that `partialsort!` does not fully sort the input
array.

# Examples
Expand All @@ -108,7 +113,7 @@ julia> a = [1, 2, 4, 3, 4]
3
4

julia> select!(a, 4)
julia> partialsort!(a, 4)
4

julia> a
Expand All @@ -127,7 +132,7 @@ julia> a = [1, 2, 4, 3, 4]
3
4

julia> select!(a, 4, rev=true)
julia> partialsort!(a, 4, rev=true)
2

julia> a
Expand All @@ -139,17 +144,18 @@ julia> a
1
```
"""
select!(v::AbstractVector, k::Union{Int,OrdinalRange};
lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward) =
select!(v, k, ord(lt,by,rev,order))
partialsort!(v::AbstractVector, k::Union{Int,OrdinalRange};
lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward) =
partialsort!(v, k, ord(lt,by,rev,order))

"""
select(v, k, [by=<transform>,] [lt=<comparison>,] [rev=false])
partialsort(v, k, [by=<transform>,] [lt=<comparison>,] [rev=false])

Variant of [`select!`](@ref) which copies `v` before partially sorting it, thereby returning the
same thing as `select!` but leaving `v` unmodified.
Variant of [`partialsort!`](@ref) which copies `v` before partially sorting it, thereby returning the
same thing as `partialsort!` but leaving `v` unmodified.
"""
select(v::AbstractVector, k::Union{Int,OrdinalRange}; kws...) = select!(copymutable(v), k; kws...)
partialsort(v::AbstractVector, k::Union{Int,OrdinalRange}; kws...) =
partialsort!(copymutable(v), k; kws...)


# reference on sorted binary search:
Expand Down Expand Up @@ -667,36 +673,36 @@ julia> v
"""
sort(v::AbstractVector; kws...) = sort!(copymutable(v); kws...)

## selectperm: the permutation to sort the first k elements of an array ##
## partialsortperm: the permutation to sort the first k elements of an array ##

"""
selectperm(v, k, [alg=<algorithm>,] [by=<transform>,] [lt=<comparison>,] [rev=false])
partialsortperm(v, k, [alg=<algorithm>,] [by=<transform>,] [lt=<comparison>,] [rev=false])

Return a partial permutation of the vector `v`, according to the order specified by
`by`, `lt` and `rev`, so that `v[output]` returns the first `k` (or range of adjacent values
if `k` is a range) values of a fully sorted version of `v`. If `k` is a single index
(Integer), an array of the first `k` indices is returned; if `k` is a range, an array of
those indices is returned. Note that the handling of integer values for `k` is different
from [`select`](@ref) in that it returns a vector of `k` elements instead of just the `k` th
element. Also note that this is equivalent to, but more efficient than, calling
`sortperm(...)[k]`.
if `k` is a range) values of a fully sorted version of `v`. If `k` is a single index,
the index in `v` of the value which would be sorted at position `k` is returned;
if `k` is a range, an array with the indices in `v` of the values which would be sorted in
these positions is returned.

Note that this is equivalent to, but more efficient than, calling `sortperm(...)[k]`.
"""
selectperm(v::AbstractVector, k::Union{Integer,OrdinalRange}; kwargs...) =
selectperm!(similar(Vector{eltype(k)}, indices(v,1)), v, k; kwargs..., initialized=false)
partialsortperm(v::AbstractVector, k::Union{Integer,OrdinalRange}; kwargs...) =
partialsortperm!(similar(Vector{eltype(k)}, indices(v,1)), v, k; kwargs..., initialized=false)

"""
selectperm!(ix, v, k, [alg=<algorithm>,] [by=<transform>,] [lt=<comparison>,] [rev=false,] [initialized=false])
partialsortperm!(ix, v, k, [alg=<algorithm>,] [by=<transform>,] [lt=<comparison>,] [rev=false,] [initialized=false])

Like [`selectperm`](@ref), but accepts a preallocated index vector `ix`. If `initialized` is `false`
(the default), ix is initialized to contain the values `1:length(ix)`.
Like [`partialsortperm`](@ref), but accepts a preallocated index vector `ix`. If `initialized` is `false`
(the default), `ix` is initialized to contain the values `1:length(ix)`.
"""
function selectperm!(ix::AbstractVector{<:Integer}, v::AbstractVector,
k::Union{Int, OrdinalRange};
lt::Function=isless,
by::Function=identity,
rev::Bool=false,
order::Ordering=Forward,
initialized::Bool=false)
function partialsortperm!(ix::AbstractVector{<:Integer}, v::AbstractVector,
k::Union{Int, OrdinalRange};
lt::Function=isless,
by::Function=identity,
rev::Bool=false,
order::Ordering=Forward,
initialized::Bool=false)
if !initialized
@inbounds for i = indices(ix,1)
ix[i] = i
Expand All @@ -705,7 +711,12 @@ function selectperm!(ix::AbstractVector{<:Integer}, v::AbstractVector,

# do partial quicksort
sort!(ix, PartialQuickSort(k), Perm(ord(lt, by, rev, order), v))
return ix[k]

if k isa Integer
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here, perhaps using dispatch for this purpose would be better than branching?

return ix[k]
else
return view(ix, k)
end
end

## sortperm: the permutation to sort an array ##
Expand Down
4 changes: 2 additions & 2 deletions base/statistics.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,9 +599,9 @@ function median!(v::AbstractVector)
n = length(inds)
mid = div(first(inds)+last(inds),2)
if isodd(n)
return middle(select!(v,mid))
return middle(partialsort!(v,mid))
else
m = select!(v, mid:mid+1)
m = partialsort!(v, mid:mid+1)
return middle(m[1], m[2])
end
end
Expand Down
8 changes: 4 additions & 4 deletions doc/src/stdlib/sort.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ Base.issorted
Base.Sort.searchsorted
Base.Sort.searchsortedfirst
Base.Sort.searchsortedlast
Base.Sort.select!
Base.Sort.select
Base.Sort.selectperm
Base.Sort.selectperm!
Base.Sort.partialsort!
Base.Sort.partialsort
Base.Sort.partialsortperm
Base.Sort.partialsortperm!
```

## Sorting Algorithms
Expand Down
6 changes: 3 additions & 3 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3765,11 +3765,11 @@ end

module M15455
function rpm_provides(r::T) where T
push!([], select(r,T))
push!([], partialsort(r,T))
end
select(a,b) = 0
partialsort(a,b) = 0
end
@test M15455.select(1,2)==0
@test M15455.partialsort(1,2)==0

# check that medium-sized array is 64-byte aligned (#15139)
@test Int(pointer(Vector{Float64}(1024))) % 64 == 0
Expand Down
2 changes: 1 addition & 1 deletion test/ranges.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ end
@test sort!(UnitRange(1,2)) == UnitRange(1,2)
@test sort(1:10, rev=true) == collect(10:-1:1)
@test sort(-3:3, by=abs) == [0,-1,1,-2,2,-3,3]
@test select(1:10, 4) == 4
@test partialsort(1:10, 4) == 4

@test 0 in UInt(0):100:typemax(UInt)
@test last(UInt(0):100:typemax(UInt)) in UInt(0):100:typemax(UInt)
Expand Down
24 changes: 12 additions & 12 deletions test/sorting.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ end
@test !issorted([2,3,1])
@test issorted([1,2,3])
@test reverse([2,3,1]) == [1,3,2]
@test select([3,6,30,1,9],3) == 6
@test select([3,6,30,1,9],3:4) == [6,9]
@test selectperm([3,6,30,1,9], 3:4) == [2,5]
@test selectperm!(collect(1:5), [3,6,30,1,9], 3:4) == [2,5]
@test partialsort([3,6,30,1,9],3) == 6
@test partialsort([3,6,30,1,9],3:4) == [6,9]
@test partialsortperm([3,6,30,1,9], 3:4) == [2,5]
@test partialsortperm!(collect(1:5), [3,6,30,1,9], 3:4) == [2,5]
let a=[1:10;]
for r in Any[2:4, 1:2, 10:10, 4:2, 2:1, 4:-1:2, 2:-1:1, 10:-1:10, 4:1:3, 1:2:8, 10:-3:1]
@test select(a, r) == [r;]
@test selectperm(a, r) == [r;]
@test select(a, r, rev=true) == (11 .- [r;])
@test selectperm(a, r, rev=true) == (11 .- [r;])
@test partialsort(a, r) == [r;]
@test partialsortperm(a, r) == [r;]
@test partialsort(a, r, rev=true) == (11 .- [r;])
@test partialsortperm(a, r, rev=true) == (11 .- [r;])
end
end
@test sum(randperm(6)) == 21
Expand Down Expand Up @@ -204,10 +204,10 @@ let alg = PartialQuickSort(div(length(a), 10))
@test !issorted(d, rev=true)
end

@test select([3,6,30,1,9], 2, rev=true) == 9
@test select([3,6,30,1,9], 2, by=x->1/x) == 9
@test selectperm([3,6,30,1,9], 2, rev=true) == 5
@test selectperm([3,6,30,1,9], 2, by=x->1/x) == 5
@test partialsort([3,6,30,1,9], 2, rev=true) == 9
@test partialsort([3,6,30,1,9], 2, by=x->1/x) == 9
@test partialsortperm([3,6,30,1,9], 2, rev=true) == 5
@test partialsortperm([3,6,30,1,9], 2, by=x->1/x) == 5

## more advanced sorting tests ##

Expand Down