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

added sortperm(v, alg;....) signature #24772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added sortperm(v, alg;....) signature #24772

wants to merge 1 commit into from

Conversation

xiaodaigh
Copy link

This is to open the door for specialized algorithms for sortperm e.g. sortperm for Radixsort see JuliaCollections/SortingAlgorithms.jl#25

This is to open the door for specialized algorithms for sortperm e.g. sortperm for Radixsort see JuliaCollections/SortingAlgorithms.jl#25
@nalimilan
Copy link
Member

Why do you need this for sortperm but not for sort? Can't you take the same approach as existing code in SortingAlgorithms does with sort!, i.e. override sortperm!?

@xiaodaigh
Copy link
Author

xiaodaigh commented Nov 25, 2017

@nalimilan sort(v, alg = RadixSort) is already working hence no need to change. But sortperm(v, alg = RadixSort) is really slow as it's still using the sortperm in Base not on a specialized algorithm. Both Base.sortperm and Base.sortperm! reduces to using sort!, which in the case of Radixsort (at least) is not the optimal way to sortperm, as can be shown in my PR to SortingAlgorithms

@LilithHafner LilithHafner added the sorting Put things in order label Jul 19, 2022
@tecosaur tecosaur added the awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants