fix unique() behaviour, add unique!() #358
Conversation
|
This does sound like a good solution. |
alyst
changed the title
|
gentle bump |
src/array.jl
Outdated
| @@ -854,7 +841,15 @@ end | |||
| Drop levels which do not appear in categorical array `A` (so that they will no longer be | |||
| returned by [`levels`](@ref DataAPI.levels)). | |||
| """ | |||
| droplevels!(A::CategoricalArray) = levels!(A, intersect(levels(A), uniquelevels(A))) | |||
| function droplevels!(A::CategoricalArray) | |||
| # FIXME temporary code until PR#359 is merged | |||
There was a problem hiding this comment.
Just a note so that we don't forget this before merging (x-ref #359).
There was a problem hiding this comment.
I've just rebased the PR against the updated droplevels!() and squashed the commits, so it should be ready for merging (when it's time for a new minor release).
so it conforms to the semantics of the Base.unique() This is a breaking change that requires a new minor release.
|
bump |
|
This is breaking so it requires tagging a new "major" release. Given the disruption it will create in the ecosystem, I'd like to be sure it's worth it. But so far we don't have lots of breaking changes to make: breaking |
|
The list of breaking things is in
breaking
. The major decision is with There are the following aspects:
In summary - I think we could go for it, but we should announce on Slack and Discourse that we plan to make this change and wait for a response. The decision as for |
A draft of a solution for #357, this PR renamesunique(::CategoricalArray)touniquelevels(::CategoricalArray)as it better reflects what this method doesThis PR restores the
Base.unique()behavior -- makesunique(x::CategoricalArray)return the categorical array of the same type asxand containing the unique values ofxin the order of their first appearance.The internal implementation of
unique()is cleaned up and optimized a bit (avoid intermediate array sorting, generalize toAbstractCategoricalArray).The PR also adds
Base.unique!(x::CategoricalArray).The newunique()is not yet covered by unit tests. I'll do that if the overall approach would be considered ok.If you need the old
unique(x::CategoricalArray)behavior, useunwrap.(unique(x)).As this is backward-incompatible change, the new minor version would be required.
Needs to be rebased once #359 lands.Fixes #357, #129.