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

Reduce the range of elements sorted by partialsort #47191

Merged
merged 5 commits into from Oct 24, 2022

Conversation

LilithHafner
Copy link
Member

missing means the end of the vector (e.g. PartialQuickSort(missing, missing) === QuickSort). There is no need to sort the elements begin:k-1 for partialsort(k).

Fixup for #45222

julia> @btime partialsort(rand(10_000), 10_000)
@btime sort(rand(10_000), alg=PartialQuickSort(10_000))
  526.527 μs (8 allocations: 312.69 KiB)
0.9998857291289974

julia> @btime sort(rand(10_000), alg=PartialQuickSort(10_000))
@btime sort(rand(10_000), alg=PartialQuickSort(missing, 10_000))[10_000]
  517.977 μs (8 allocations: 312.69 KiB)
 0.9999893337886981

julia> @btime sort(rand(10_000), alg=PartialQuickSort(missing, 10_000))[10_000]
  521.084 μs (8 allocations: 312.69 KiB)
 0.999986247898864

julia> @btime sort(rand(10_000), alg=PartialQuickSort(10_000, 10_000))[10_000]
  67.384 μs (8 allocations: 312.69 KiB)
 0.9997135092971121

@LilithHafner LilithHafner added domain:sorting Put things in order performance Must go faster labels Oct 17, 2022
@petvana
Copy link
Member

petvana commented Oct 17, 2022

I may miss something, but this PR seems to be in a conflict with the original documentation, and thus breaking

PartialQuickSort{T <: Union{Integer,OrdinalRange}}

  Indicate that a sorting function should use the partial quick sort algorithm. Partial quick sort
  returns the smallest k elements sorted from smallest to largest, finding them and sorting them using
  QuickSort.

@LilithHafner
Copy link
Member Author

That's probably why I overlooked it in the last PR.

However, partialsort(v, ::Integer) and friends only return a single element and are not documented to sort the beginning of the vector, so we may still use PartialQuickSort(k, k) internally.

julia> partialsort(rand(1000), 500)
0.5000542824047202

I'll make the corresponding changes.

@petvana
Copy link
Member

petvana commented Oct 17, 2022

Thanks. Shouldn't we also include the removed documentation for PartialQuickSort(k::Integer)?

test/sorting.jl Outdated Show resolved Hide resolved
Co-authored-by: Petr Vana <petvana@centrum.cz>
@LilithHafner
Copy link
Member Author

I'm not a huge fan of PartialQuickSort(k) meaning "sort the first k elements" while partialsort!(v, k) means "put the kth element in place", so I'm inclined to leave the docstring out and replace it with a comment noting that PartialQuickSort(k) = PartialQuickSort(missing, k) is needed for compatability reasons.

Is the undocumentation of a feature (provided it is still supported) something we can do before 2.0?

@petvana
Copy link
Member

petvana commented Oct 23, 2022

Ok. Can we at least deprecate it if we remove the docs? Smth like this?

@deprecate PartialQuickSort(k::Integer) PartialQuickSort(1:k)

Results in this

$ julia --depwarn=yes             |

julia> import Base: PartialQuickSort

julia> @deprecate PartialQuickSort(k::Integer) PartialQuickSort(1:k)
PartialQuickSort

julia> PartialQuickSort(5)
┌ Warning: `PartialQuickSort(k::Integer)` is deprecated, use `PartialQuickSort(1:k)` instead.
│   caller = ip:0x0
└ @ Core :-1
PartialQuickSort{UnitRange{Int64}}(1:5)

@LilithHafner
Copy link
Member Author

I decided to do that separately because I think both PRs stand on their own and they have substantially different motivations.

@LilithHafner
Copy link
Member Author

It took a few turns to get here, but I believe this PR is currently a simple performance improvement and the CI failures are unrelated.

sortperm(v, k::Integer) will no longer sort the beginning of v, but it was never documented to do so.

@petvana
Copy link
Member

petvana commented Oct 24, 2022

If CI failures are really unrelated then the PR seems ready to be merged.

@LilithHafner LilithHafner merged commit a8bf137 into master Oct 24, 2022
@LilithHafner LilithHafner deleted the LilithHafner-patch-3 branch October 24, 2022 11:08
@LilithHafner
Copy link
Member Author

Typically folks express that sentiment with an approving PR review, but a comment works too :)

@LilithHafner LilithHafner mentioned this pull request Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:sorting Put things in order performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants