RFC: quicksort refactor, addition of selectperm #12303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kmsquire
changed the title
|
Nice. |
|
This looks great. Anything else need to be done? |
|
The docs for PartialQuickSort need to be updated (it can handle any OrdinalRange). I would also like to try only having PartialQuickSort take an OrdinalRange, but I'm not sure I'll get to that today. It shouldn't change any functionality, but it would simplify the code some. |
|
That could be done in a follow-on change, right? Seems better to do it separately then. |
|
👍 merge at will! |
* Break out select_pivot!, partition!, functions * Share these among different quicksort variants * Switch select to use PartialQuickSort
|
Thanks! Updated docs, squashed, and will merge when CI turns green again. I left @spencerlyon2's original commit in here. It passed CI previously, so I don't think it should be a problem. |
kmsquire
added a commit
that referenced
this pull request
Jul 24, 2015
RFC: quicksort refactor, addition of selectperm
|
Thanks @kmsquire. Hopefully it isn't an issue! |
|
:-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR picks up where #10800 left off:
select_pivot!andpartition!from quicksortPartialQuickSort(originally from @carlobaldassi)selectperm,selectperm!select(andselect!) to usePartialQuickSortPartialQuickSorthere handles bothIntandOrdinalRangetype parameters (which was not part of #10800).It could (and probably should) be changed to only handle
OrdinalRangeparameters, which would simplify the code (removing one extra function), at the cost of 1 extra comparison per (recursive) call.With limited testing, it seems that there are no performance regressions.
Cc: @spencerlyon2, @StefanKarpinski