Conversation
nalimilan
left a comment
nalimilan
left a comment
There was a problem hiding this comment.
Thanks. That's what I had in mind, though I'm still not completely certain whether CategoricalArray + Array should promote to the former or the latter... Both could be justified, though what this PR does is most likely the best choice.
While you're at it, I would generalize this to any AbstractArray.
| @test typeof(x) == CA{Int, 1, UInt8} | ||
| @test typeof(ux) == CA{Int, 1, CategoricalArrays.DefaultRefType} | ||
|
|
||
| @test promote_type(typeof(CA([3,2,1])), typeof([1,2])) == CA{Int,1,DefaultRefType} |
There was a problem hiding this comment.
Use the types directly instead of creating the objects and calling typeof.
| @inbounds A.refs[I...] = get!(A.pool, convert(T, v)) | ||
| end | ||
|
|
||
| @inline function promote_rule{T,N,R,T2}(::Type{CatArray{T,N,R}}, ::Type{Array{T2}}) |
There was a problem hiding this comment.
You don'tneed @inline. Also, you should add a type parameter <:CatArray and promote to it instead of CatArray (which is a union type). Instead of T and T2, the rest of the code base uses S and T.
|
|
||
| @test promote_type(typeof(CA([3,2,1])), typeof([1,2])) == CA{Int,1,DefaultRefType} | ||
| @test promote_type(typeof(CA([3,2,1])), typeof(NullableArray([1,3]))) == NullableCategoricalArray{Int,1,DefaultRefType} | ||
| end |
There was a problem hiding this comment.
Could you also test cases in which the element types are different, e.g. Int and Float64? Would be good to test promoting (Nullable)CategoricalArray with CA, and check in particular that the reference type is as expected.
|
What happens if another implementation of abstractarray wants to use promote_rule to promote any abstractarray to itself? Then we would be back to depend on the order of arguments. I don't have an example so perhaps this argument is moot. But I wanted to at least consider for a moment if this is a good design choice. |
|
Yes, that's a good question. A good example to consider is I think the solution is to fix the ambiguity by providing a special method for the |
|
Bump. Do you want to finish this PR? |
2 participants
@nalimilan is this what you had in mind? To promote arrays with and without
NullabletoCategoricalArrayorNullableCategoricalArray. Or do you want to capture any and allAbstractArray?