`replace` does not respect target type

[nalimilan]

Currently replace calls recode and always returns a CategoricalArray, even if the target values are not CategoricalValues. This is OK for recode as it's specific to CategoricalArrays, but ideally replace should respect the target type. Unfortunately, the behavior of replace on arrays is to call promote_type on the source element type and on target values' types. This would give weird arrays such as Array{Union{CategoricalValue{String,UInt32}, Int}}. We could use a different approach which would choose the element type based on the actual values, like broadcast. But that would trade an inconsistency with Base for another inconsistency.

For example, the following should ideally return a Vector{Int} (see this thread):

julia> a = CategoricalArray(["X", "X", "Y", "Z", "Y", "Y", "Z"])
7-element CategoricalArray{String,1,UInt32}:
 "X"
 "X"
 "Y"
 "Z"
 "Y"
 "Y"
 "Z"

julia> replace(a, "X"=>1, "Y"=>2, "Z"=>3)
7-element CategoricalArray{Union{Int64, String},1,UInt32}:
 1
 1
 2
 3
 2
 2
 3

Cc: @bkamins

[bkamins]

I was thinking about it when I answered on SO and concluded that the design of replace was intentional because the contract for replace is:

Return a copy of collection A where ...

Which implies that the container type of returned value should be the same as if we did a copy (optionally doing type promotion).

I think that the crucial point of replace is that normally it is assumed that only some of the values in the source are replaced.

The operation requested on SO assumes we are doing mapping of all values. I think it is a valid use case but intuitively I would expect a different function for it (that would in particular check that a full mapping specification is provided). But maybe it would be not that useful. Not sure. Maybe it would be enough to add a kwarg to replace (and also recode) allowing user to decide if the result should be categorical or not?

[nalimilan]

The part of the docstring (which I wrote IIRC!) that I was worried about is:

The element type of the result is chosen using promotion (see promote_type) based on the element type of A and on the types of the new values in pairs.

Clearly we don't respect this currently. But it's not easy to respect.

Maybe it would be enough to add a kwarg to replace (and also recode) allowing user to decide if the result should be categorical or not?

That wouldn't fix the inconsistency with the docstring, but yeah that could be useful. In particular that would allow the reverse choice, i.e. request a CategoricalArray when the input is an Array (this is common when you have integer-coded inputs that you want to turn into explicit categories).

[bkamins]

I agree that meeting the contract is hard, that is why I thought of adding a kwarg that would allow to choose from one of possible intentions of the user. Also having a kwarg will make it easier for users to notice that there is a choice (and an issue, as I think many users could overlook the problem).

[nalimilan]

Let's keep the current behavior for 1.0. We may add a keyword argument at some point, but maybe it's not needed as unwrap.(replace(...)) is reasonably convenient and efficient.