BUG: concat of series of dtype category converting to object dtype (GH8641)
State: closed

closes #8641 


Labels:
name: Bug
name: Error Reporting
description: Incorrect or improved errors from pandas
name: Categorical
description: Categorical Data Type

Comments:
cc @immerrr

maybe you can think of a better way to do this. 'feels' a bit hacky

It does indeed, let me have a closer look, I don't have my Emacs at hand right now.

There are two things that bother me.

One thing that bothers me is that concatenation code somehow crept back into Block class. Concatenating Categoricals (with ndarrays or with other categoricals) seems a valid scenario regardless of Blocks. It creates those really weird workflows when you need to create Blocks just to concatenate two values (I'm looking at you `DatetimeIndex.hour`). It seems simpler to just add proper special-case code to `core.common._concat_compat`. Or to put special-case concatenation close to their class definitions and do something like:

``` python
def concatenate_arrays(arrays):
    for fn in [pd.core.categorical.maybe_concatenate_categoricals,
               pd.core.sparse.array.maybe_concatenate_sparsearrays]:
        retval = fn(arrays)
        if retval is not None:
            return retval
    else:
        return np.concatenate([maybe_getvalues(a) for a in arrays])
```

This particular PR might probably get away with just fixing _concat_compat and leaving the rest of the code intact (if Series constructor supports Categorical data). And speaking of `_concat_compat`, it's quite possible that the special-casing of datetimes/timedeltas there is no longer necessary since numpy is now 1.7+.

The other thing is that Series being doubly special strikes back again: not only its first axis doesn't allow type variation (special case #1), but there is also an _implicit_ axis=1 which allows Series to be stacked _horizontally_ (special case #2). I wonder how much will it take to stop pretending and make Series **internally** a dataframe and if it will reduce the amount of special-casing. I also wonder if there's a place under the sun for type-varying (aka non-singleblock) Series, one use case that immediately comes to mind is that `frame.iterrows` can finally stop losing type information. 

I think your first point is very valid - though (aside from the obvious impl issues) why do you think concatenation should be functional rather than exist in Block? is their a conceptual/fundamental reason of just (the exhibited code complexity)?

you second point though I am confused
so you want to allow multiple blocks on the same axis? seems very complex and unwieldy. Maybe you can expound on the perceived benefits

axis=1 is not a special case of Series rather a special case of merge - one that is an impl detail and can be done trivially rather than going thru the merge code
not a function of Series itself

can u show the DatetimeIndex.hour issue/example? from above

so you are proposing (sort of in between 1 and 2) to effectively remove SingleBlockManager and make it a pure BlockManager

I think you could (and prob not lose perf though you do lose opportunity to do some optimizations - not sure how much that would affect it)

want to create an issue for that?

The second point is rather a thought experiment and I probably should have not mentioned it here. Let's take it off the table right away and move it to another issue (or probably to the mailing-list).

re: datetimeindex:

Provided I have an array of dates and I want their respective day values, the easiest way is to do that is via a DatetimeIndex:

``` python
In [21]: arr = np.array([datetime(2014, 1, 1), datetime(2014, 1, 2), datetime(2014, 1, 3)])

In [22]: pd.DatetimeIndex(arr).day
Out[22]: array([1, 2, 3], dtype=int32)
```

I find it weird that I have to create an _index_ to perform an operation on an array. I know, it's the case of Index fixing Array shortcomings and I haven't yet found time to fix this.

As for should the concatenation be functional or not, it's really a matter of preference, but as long as one cannot simply do `arrs[0].concatenate(arrs[1:])`, because `arrs[0]` may be an `ndarray` and those don't have a member function for that, I find it easier to write `concatenate(arrs)` than to add conditionals and loops to find the happy owner of `concatenate` method every time I need it.

And just to be clear, in terms of this particular PR I think it should work to implement special-casing for categorical concatenation in _concat_compat and revert most (maybe all) other changes.

And in general, I think that ideally Blocks should only maintain ref_locs and placement consistency with the data, concatenation rather belongs to array-related code.

@immerrr 

`Series(pd.date_range('20140101',periods=4)).dt.day` is new in 0.15.0

I agree with all of your points:
- going to fix `_compat_concat` instead (IIRC the datetime special casting in their was failing when I tried to take it out before, maybe let's have a look)
- agreed that Blocks are involved with merges but not direct block concatenation.

updated and pushed. pls take a look.

Looks solid. I mean, true, now there are noticeable logic blocks in concat_compat that can be moved to a sub-functions, but I'm not sure if it is that bad on readability. Maybe summon a fresh pair of eyes? :)

> Series(pd.date_range('20140101',periods=4)).dt.day is new in 0.15.0

Yeah, that's much better. Seems a tad slower because of all the stuff Series ctor does, but definitely looks more intuitive to me.

@immerrr ok, finally fixed this.

We _could_ dispatch to the sub-types to avoid having some of this code all in common

e.g. Categorical,SparseArray (maybe in tseries/common.py for timedelta/datetime)

?

@immerrr I fixed the impl to use my suggestion above, much cleaner IMHO.

> I fixed the impl to use my suggestion above, much cleaner IMHO.

Much cleaner indeed.  Although I'm still a bit worried that this PR does a bit too much corollary change (e.g. bool handling) without any tests for that change.

ok going to delay this
though all of the 'special cases' actually seem to be necessary just to get various things to pass

eg the bool and and int casting is a special case where np.concatenate does the wrong thing

and np.find_commom_type is basically useles

so prob need a systematic test of :
all dtypes in combination (by 2s,3s) with and without empties

@immerrr I updated hopefully to catch all of these empty-empty combo cases. pls have a look.

ok, I think fixed up. the only weird one is `float + bool` which is an existing condition. But all works as currently (and I think testing correctly).

lmk

Added couple more nitpicks, but generally looks fine.