Add CategoricalValue(x, source) and disallow mixed isless and <
#346
Conversation
`isless` and `<` between `CategoricalValue` and other types can breaks
transitivity. Require converting the value to a `CategoricalValue`,
attaching it to a pool, using
`CategoricalValue(x, source::Union{CategoricalValue, CategoricalArray)`.
Deprecate `CategoricalValue(i::Integer, pool::CategoricalPool)` in favor
of `pool[i]` or `CategoricalValue(pool, i)`, as the order is more logical
and it avoids a potential ambiguity if `source::CategoricalPool` is allowed
in the future.
src/missingarray.jl
Outdated
| @@ -34,10 +34,10 @@ in(x::Missing, y::CategoricalArray{>:Missing}) = !all(v -> v > 0, y.refs) | |||
|
|
|||
| function Missings.replace(a::CategoricalArray{S, N, R, V, C}, replacement::V) where {S, N, R, V, C} | |||
| pool = copy(a.pool) | |||
| v = C(get!(pool, replacement), pool) | |||
| v = C(pool, get!(pool, replacement)) | |||
| Missings.replace(a, v) | |||
| end | |||
|
|
|||
| function collect(r::Missings.EachReplaceMissing{<:CategoricalArray{S, N, R, C}}) where {S, N, R, C} | |||
There was a problem hiding this comment.
it is a bit confusing that you use C to mean what is named V above. Maybe make it consistent?
There was a problem hiding this comment.
Indeed. Better always use the same names as in the type definition.
| pool::CategoricalPool{T, R, CategoricalValue{T, R}} | ||
| ref::R |
There was a problem hiding this comment.
smart - I was thinking how you have changed it - and it is just super simple :).
There was a problem hiding this comment.
Actually I wanted to change their order for a long time, but there's never been a strong enough reason to do it. :-)
| @@ -140,8 +153,11 @@ function Base.isless(x::CategoricalValue, y::CategoricalValue) | |||
| end | |||
| end | |||
|
|
|||
| Base.isless(x::CategoricalValue, y::SupportedTypes) = levelcode(x) < levelcode(x.pool[get(x.pool, y)]) | |||
| Base.isless(y::SupportedTypes, x::CategoricalValue) = levelcode(x.pool[get(x.pool, y)]) < levelcode(x) | |||
| Base.isless(x::CategoricalValue, y::SupportedTypes) = | |||
There was a problem hiding this comment.
so you have decided to go forward fast and be breaking here? (I am OK with this, but usually you wanted one round of releases with deprecations 😄 )
There was a problem hiding this comment.
Yes maybe I can backport a deprecation to 0.9. Though the error message is basically the same as a deprecation so I wonder whether it's worth it.
There was a problem hiding this comment.
I am OK with error as commented.
islessand<betweenCategoricalValueand other types can breaks transitivity. Require converting the value to aCategoricalValue, attaching it to a pool, usingCategoricalValue(x, source::Union{CategoricalValue, CategoricalArray).Deprecate
CategoricalValue(i::Integer, pool::CategoricalPool)in favor ofpool[i]orCategoricalValue(pool, i), as the order is more logical and it avoids a potential ambiguity ifsource::CategoricalPoolis allowed in the future.Fixes #319.