Simplify and increase generality of sorting#397
Simplify and increase generality of sorting#397LilithHafner wants to merge 1 commit intoJuliaData:masterfrom
Conversation
|
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. |
|
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, |
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
scratchkeyword argument that can be passed intosort!. This implementation would automatically support that keyword and let the level sorting subroutine utilize a provided scratch space as soon as it is introduced.