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

Simplify and increase generality of sorting #397

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

Conversation

LilithHafner
Copy link

I believe that the only functional change here is that instead of ignoring alg, it is passed on to the level sorting subroutine.
This also future-proofs the implementation, to an extent. For example, future versions of julia may include a scratch keyword argument that can be passed into sort!. This implementation would automatically support that keyword and let the level sorting subroutine utilize a provided scratch space as soon as it is introduced.

@nalimilan
Copy link
Member

Sorry for the super long delay. Generally, I prefer listing keyword arguments here so that users get a direct error about the function they called if they misspell it, rather than something coming from lower in the stack, which could look like it's a bug in CategoricalArrays. That could be even worse if we manage to abbreviate stacktraces to show only user code.

@LilithHafner
Copy link
Author

That's a valid concern, though the error message itself will remain pretty similar to this:

julia> sort!(ca, x=1)
ERROR: MethodError: no method matching sort!(::CategoricalVector{Int64, UInt32, Int64, CategoricalValue{Int64, UInt32}, Union{}}; x::Int64)

Closest candidates are:
  sort!(::CategoricalVector; alg, lt, by, rev, order) got unsupported keyword argument "x"
   @ CategoricalArrays ~/.julia/packages/CategoricalArrays/0yLZN/src/array.jl:1103
  sort!(::AbstractVector{T}; alg, lt, by, rev, order, scratch) where T got unsupported keyword argument "x"
   @ Base sort.jl:1367
  sort!(::AbstractVector, ::Integer, ::Integer, ::Base.Sort.QuickSortAlg, ::Base.Order.Ordering) got unsupported keyword argument "x"
   @ Base sort.jl:2030
  ...

It's a tradeoff between generality/composability/code simplicity vs cutting one frame out of the stack trace. I don't use this package much, so I'll leave that value judgment up to you.

fwiw, Base.sort! currently supports the (undocumented and internal) keyword argument scratch which can be used to avoid allocations. Adding scratch to this explicit list would be bad because it is internal, but blindly passing args along as this PR does would support scratch without depending on Base internals.

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.

None yet

2 participants