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! #8792

Merged
merged 1 commit into from
Oct 24, 2014
Merged

Added sortperm! #8792

merged 1 commit into from
Oct 24, 2014

Conversation

kmsquire
Copy link
Member

One change which I'm unsure of here is loosening the types allowed for lt(p::Perm, a, b) from Int to Integer. I did it with the idea that, if a user is providing a preallocated array, she could choose to use a different integer type (e.g., a large Int32 array might have better cache performance than Int64).

That said, restricting the sortperm array type to Vector{Int} would be fine as well. Thoughts?

Cc: @ViralBShah, @StefanKarpinski

* Allows preallocation (and therefore, reuse) of an index array
* Useful for #8316
@johnmyleswhite
Copy link
Member

This makes me very happy. Thanks, Kevin!

@ViralBShah
Copy link
Member

This is so awesome! I think Integer is ok, so long as it does not have a performance impact, because there is value in being able to use Int32 for performance gains where worthwhile.

@StefanKarpinski
Copy link
Member

Very nice.

ViralBShah added a commit that referenced this pull request Oct 24, 2014
@ViralBShah ViralBShah merged commit 1000471 into master Oct 24, 2014
@ViralBShah ViralBShah deleted the kms/sortperm! branch October 24, 2014 10:43
@@ -341,6 +342,14 @@ sortperm(v::AbstractVector; alg::Algorithm=DEFAULT_UNSTABLE,
lt::Function=isless, by::Function=identity, rev::Bool=false, order::Ordering=Forward) =
sort!([1:length(v)], alg, Perm(ord(lt,by,rev,order),v))

function sortperm!{I<:Integer}(x::Vector{I}, v::AbstractVector; alg::Algorithm=DEFAULT_UNSTABLE,
Copy link
Member

Choose a reason for hiding this comment

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

I guess x can be an AbstractVector. Any reason it should not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that should be fine. I actually started with that, and then changed it for some reason that I forget.

@ViralBShah
Copy link
Member

What about workspace allocation in the various sort algorithms? Presumably MergeSort has a workspace. Can we make that reusable by passing a work array too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants