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

rename QuickerSort to ScratchQuickSort #48160

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

LilithHafner
Copy link
Member

From #47973 (comment) and discussion on slack.

fwiw, the term we use in sorting is "scratch space" and the (undocumented) keyword argument is named "scratch".

@LilithHafner LilithHafner added sorting Put things in order backport 1.9 Change should be backported to release-1.9 labels Jan 6, 2023
@LilithHafner LilithHafner changed the title rename QuickerSort to QuickBufferSort rename QuickerSort Jan 7, 2023
@LilithHafner
Copy link
Member Author

I actually prefer QuickScratchSort. It reflects the strong similarity with QuickSort, reflects the out of place nature in a neutral way, and uses terminology consistent with the keyword argument name scratch.

@petvana, I'm curious what your objection to renaming QuickerSort => QuickBufferSort is and whether it still applies to QuickScratchSort.

@petvana
Copy link
Member

petvana commented Jan 7, 2023

I'm not a native speaker. But QuickBufferSort sounds like an improved version of BufferSort (and it is not). Moreover, whether in-place or stability is a more important feature is questionable. I'd use StableQuickSort as it seems to be already used in literature; the only problem is the name collision. May an alternative be BufferQuickSort or AllocatingQuickSort? You may ignore my comment if you see it as irrelevant.

@LilithHafner
Copy link
Member Author

Thank you! The perspective of a non-native speaker is important to me because I want this name to make sense to non-native speaker users.

StableQuickSort is not ideal because this algorithm could be adapted to a slightly faster unstable algorithm by deleting this line and a few others. I don't like this possible future: sort(v; alg=StableQuickSort, stable=false).

AllocatingQuickSort is not correct because this algorithm need not allocate if passed a preallocated scratch space:

julia> @btime sort!(rand!(x); alg=QuickSort) setup=(x=rand(Int, 1000));
  33.422 μs (0 allocations: 0 bytes)

julia> @btime sort!(rand!(x); alg=Base.Sort.QuickerSort(), scratch) setup=(x=rand(Int, 1000); scratch=rand(Int, 1000));
  21.655 μs (0 allocations: 0 bytes)

julia> VERSION
v"1.9.0-beta2"

Perhaps ScratchQuickSort? This no longer implies that it is a quick variant of ScratchSort (does not exist) and also uses the same terminology as the keyword argument.

@petvana
Copy link
Member

petvana commented Jan 8, 2023

+1 for ScratchQuickSort

Comment on lines +961 to 962
`ScratchQuickSort` is like `QuickSort`, but utilizes scratch space to operate faster and allow
for the possibility of maintaining stability.
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels right

@LilithHafner LilithHafner changed the title rename QuickerSort rename QuickerSort to ScratchQuickSort Jan 12, 2023
@LilithHafner LilithHafner merged commit 9707594 into master Jan 13, 2023
@LilithHafner LilithHafner deleted the QuickerSort-to-QuickBufferSort branch January 13, 2023 18:01
KristofferC pushed a commit that referenced this pull request Jan 13, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants