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

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 1 commit
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
Prev Previous commit
Next Next commit
Make partialsort!() and partialsortperm!() return a view rather than … 
…a copy

Returning a copy (partially) defeats the purpose of these functions, which is
to avoid allocations.
  • Loading branch information
@nalimilan
nalimilan committed Aug 27, 2017
commit 8150f666fd76a6098e12f235ea8fbf7246867e9e
8 changes: 4 additions & 4 deletions base/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,10 +1743,10 @@ end
@deprecate IOContext(io::IO, key, value) IOContext(io, key=>value)

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

# END 0.7 deprecations

Expand Down
14 changes: 12 additions & 2 deletions base/sort.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,12 @@ issorted(itr;
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
Sponsor 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

"""
Expand Down Expand Up @@ -706,7 +711,12 @@ function partialsortperm!(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