feat: unstack receives kwarg fillvalue

[pstorozenko]

Closes #2698

I thought about checking every column separately for promote_type, but it would be a breaking change since right now value column with eltype T is always converted to Vector{Union{Missing, T}}. We may think about changing it in another PR.

[bkamins]

but it would be a breaking change

Why do you think so? I would assume that if fillvalue is missing which is the default and old behavior in your PR we would get the same (so this means that no old code would be broken - the result would stay the same).

[bkamins]

I thought about checking every column separately

Ah - after reviewing I see what you mean. What you have implemented is OK I think. One can run identity.(df) to do type narrowing later if needed.

However, it is important to test the additional scenarios I have mentioned I think. Especially for CategoricalVector check how levels behave in different combinations of inputs and in cases when original CategoricalVector is ordered/unordered.

Other than that - the PR looks good.

[bkamins]

@pstorozenko - merge conflicts require fixing + have you had time to add the tests I have suggested?

[bkamins]

@pstorozenko bump

[pstorozenko]

Sorry for not answering.
There are a few things I found when trying to create tests for Categorical Vector:

1. When dealing with vectors that are subtypes of CategoricalArrays.AbstractCategoricalArray we should create unstacked_val with fillvalue value and levels from valuecol + fillvalue.
2. When fillvalue is missing then it is not added to levels (that's how CategoricalArrays work AFAIK).

[bkamins]

Point 2 is correct.

Point 1 - there is a corner case when fillvalue is not allowed by CategoricalArray. As discussed - I will update the PR to check for corner cases. Thank you!

[bkamins]

@nalimilan - this definition is problematic for categorical columns the problem is that:

julia> using CategoricalArrays

julia> valuecol = categorical(["a"])
1-element CategoricalArray{String,1,UInt32}:
 "a"

julia> fillvalue = ""
""

julia> Nrow = 1
1

julia> similar(valuecol, promote_type(eltype(valuecol), typeof(fillvalue)), Nrow)
1-element Vector{String}:
 #undef

julia> promote_type(eltype(valuecol), typeof(fillvalue))
String

Is there a generic idiom that would (without introducing dependency on CategoricalArrays.jl) allow us to take the union of types and if it is OK (like it should be in the example above) produce CategoricalArray? Or maybe you think we should change the definition of similar in CategoricalArrays.jl?

[bkamins]

OK - I have thought about it. Given we currently allow:

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

julia> CategoricalValue(3, x)
CategoricalValue{Int64, UInt32} 3

the behavior we have now should be OK.

[nalimilan]

I think it's OK. If we wanted to change this we could adapt promote_type in CategoricalArrays, but it's orthogonal to this PR. There would certainly be advantages in promoting to CategoricalValue, though it could also create problems (e.g. when concatenating Vector{Int} and CategoricalArray{Union{String, Missing}}the resulting type would beAny` but CategoricalArrays doesn't handle that well).

[bkamins]

Yes - I think there would be more problems than benefits. We only need an easy way to create a "stand alone" CategoricalValue, as now it is CategoricalValue(scalar, categorical([scalar])) which is quite inconvenient.

[bkamins]

@nalimilan - this test in particular is something we should think of. My judgement is that it is OK to produce such an union.

[nalimilan]

I think it's OK. If we wanted to change this we could adapt promote_type in CategoricalArrays, but it's orthogonal to this PR. There would certainly be advantages in promoting to CategoricalValue, though it could also create problems (e.g. when concatenating Vector{Int} and CategoricalArray{Union{String, Missing}}the resulting type would beAny` but CategoricalArrays doesn't handle that well).

[nalimilan]

Maybe we should call this just fill? Or is that name useful in other contexts as a Boolean argument?

[bkamins]

No - this name is specific only to this function. I will rename it to fill.

[bkamins]

OK - I have renamed fillvalue to fill. The only potential problem would be if we used the fill function from Julia Base in the body of the function, but we do not use it. We only use fill! so all is OK.

[bkamins]

@nalimilan - any additional comments here?

[bkamins]

Thank you!