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

fix categorical for views #234

Open
bkamins opened this issue Dec 12, 2019 · 4 comments
Open

fix categorical for views #234

bkamins opened this issue Dec 12, 2019 · 4 comments

Comments

@bkamins
Copy link
Member

bkamins commented Dec 12, 2019

The problem is:

julia> c = categorical([1,2,3], ordered=true)
3-element CategoricalArray{Int64,1,UInt32}:
 1
 2
 3

julia> c
3-element CategoricalArray{Int64,1,UInt32}:
 1
 2
 3

julia> isordered(c)
true

julia> cv = view(c, :)
3-element view(::CategoricalArray{Int64,1,UInt32,Int64,CategoricalValue{Int64,UInt32},Union{}}, :) with eltype CategoricalValue{Int64,UInt32}:
 1
 2
 3

julia> isordered(cv)
true

julia> isordered(categorical(cv))
false

The core of the issue is how _isordered function is implemented (as it is not extensible now unfortunately). Probably a temporary fix is to define _isordered for SubArrays.

@nalimilan
Copy link
Member

I think we can define _isordered to call isordered for any AbstractArray{<:CategoricalValue} which implements that function -- maybe falling back to checking whether each entry is ordered.

@bkamins
Copy link
Member Author

bkamins commented Dec 12, 2019

I thought of making _isordered check if isordered is defined for the passed argument and if yes reuse it and return false otherwise.

The problem is that AbstractArray{<:CategoricalValue} can contain values from different categorical arrays (even if they are all ordered they can even have conflicting orders). So if it does not define isordered I would fallback to false.

@nalimilan
Copy link
Member

Yes, if isordered is not defined then the result will probably not be very useful. (Though in theory values from different pools could have compatible orders.)

@bkamins
Copy link
Member Author

bkamins commented Dec 12, 2019

Though in theory values from different pools could have compatible orders

Yes, but checking this would have a big performance overhead, and as noted - I am not sure if it would be useful.

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

No branches or pull requests

2 participants