ENH: added selectperm and selectperm! based on new PartialQuickSort #10800
Conversation
sglyon
changed the title
|
Cc: @kmsquire |
| @@ -1,5 +1,4 @@ | |||
| export | |||
| # Modules | |||
| export # Modules | |||
There was a problem hiding this comment.
Yep, I'll revert that line
| end | ||
|
|
||
| # partially sort until the end of the range | ||
| PartialQuickSort(r::OrdinalRange) = PartialQuickSort(last(r)) |
There was a problem hiding this comment.
If we're going to include this, would it be good to support arbitrary ordinal ranges?
There was a problem hiding this comment.
I'm not sure I understand what you mean by arbitrary ordinal range, but I think that kinda happens when we use this algorithm on 443 inside selectperm!. There I say sort until the end of the range and then only return sorted indices within the range (return ix[k])
I agree it would probably be better to support this in the sort algorithm directly, but I didn't see an obvious way to extend @carlobaldassi code to handle it there.
|
Ping. @kmsquire I see that you have been self-assigned to take care of this. How can I help? |
|
Bumping was good. :-) let me take a closer look at the PR. |
|
|
||
| # TODO: Not type stable. If k is an int, this will return an Int, of it is | ||
| # an OrdinalRange it will return a Vector{Int}. This, however, seems | ||
| # to be the same behavior as as `select` |
There was a problem hiding this comment.
Actually, your description shows that this function is type-stable.
There was a problem hiding this comment.
Ha yes it sure is. Sorry for the inaccurate comment there.
|
Right now, Duplication is sometimes okay and sometimes necessary, but in this case, it would probably be better to refactor and reuse parts of I've already worked through part of the refactor (mostly to make sure it made sense). I can submit that as a pull request to this patch, or as a separate PR (and close this one), or I can help you work through it if you'd like, @spencerlyon2. I'll add a couple of more comments inline. |
| sort!(v, lo, j, PartialQuickSort(k), o) | ||
| end | ||
| end | ||
| jk = min(j, lo + k - 1) |
|
@kmsquire -- thanks for all the notes. Very helpful. Sorry the PR was a bit sloppy. I was mostly just pulling code from a few different sources to jump start conversation about having this functionality in base. I don't think there is any reason to try to fix this PR up if you already have a working version that leverages |
|
Just a comment that I have been working on this, and will submit a WIP PR soon. |
|
Included in #12303. |
This pull request implements 2 main things:
PartialQuickSort(k)algorithm based on the work by @carlobaldassi in this gistselectperm/selectperm!that are tosortperm/sortperm!whatselect/select!are tosort/sort!I have tried to add minimal tests and docs. I don't remember how to add things to the helpdb, so the entries from the
sort.rstwill not appear at the REPL.Some things can certainly be cleaned up. Comments welcome.
closes #10767