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

avoid applying fast_sortable on integer arrays #21

Closed
wants to merge 2 commits into from
Closed

avoid applying fast_sortable on integer arrays #21

wants to merge 2 commits into from

Conversation

piever
Copy link
Contributor

@piever piever commented Mar 9, 2019

The whole idea of fast_sortable is to return a AbstractVector{<:Integer} with the same sorting as the original. If the original was already of integer eltype we don't actually need to do anything.

@piever
Copy link
Contributor Author

piever commented Apr 3, 2019

Bump?

@@ -428,6 +428,8 @@ function Base.vcat(a::PooledArray{T, <:Integer, 1}, b::PooledArray{S, <:Integer,
return PooledArray(RefArray(newrefs), convert(Dict{U, refT}, newlabels))
end

fast_sortable(y::PooledArray{<:Integer}) = y
Copy link
Member

Choose a reason for hiding this comment

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

@piever Should we additionally check isbitstype(eltype(y)) and otherwise use the "slow" path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I imagine non-isbits comparison can be quite slow, even though in practice I doubt anyone has a PooledArray{BigInt} or things like that. I've changed that to the slow path. The important part (for me) is that fast_sortable is idempotent which should always be the case as sortperm returns isbits integers.

@piever
Copy link
Contributor Author

piever commented Jun 7, 2019

Unrelated, but I'm glad the package was moved to JuliaData and is getting more maintenance!

@bkamins
Copy link
Member

bkamins commented Dec 1, 2020

@piever - could you please resolve the merge conflicts in this PR as I would like to finalize it.

@nalimilan - could you please have a quick look at the PR before merging.

Thank you both!

@piever
Copy link
Contributor Author

piever commented Dec 2, 2020

Rebased in #46 (sorry about the confusion, but the original branch had been deleted!)

@piever piever closed this Dec 2, 2020
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.

3 participants