BUG: Fix concat of frames with extension types (no reindexed columns)

[jorisvandenbossche]

This PR tries to fix the case of concatting dataframes with extension dtypes and non-extension dtypes, like this example (giving object dtype on master, instead of preserving Int64 dtype):

In [15]: df1 = pd.DataFrame({"a": pd.array([1, 2, 3], dtype="Int64")}) 
    ...: df2 = pd.DataFrame({"a": np.array([4, 5, 6])})  

In [16]: pd.concat([df1, df2]) 
Out[16]: 
   a
0  1
1  2
2  3
0  4
1  5
2  6

In [17]: pd.concat([df1, df2]).dtypes 
Out[17]: 
a    object
dtype: object

Up to now, the _get_common_dtype machinery of the ExtensionArrays was only used for concatting Series (axis=0 in concat_compat). Now I also try to make use of this for DataFrame columns, if there is any extension dtype in the to-concat arrays. This is checked in _concatenate_join_units. And we need to handle the dimension-mismach. I decided to handle that inside the internals code (reducing the dimension for non-EAs, and if the concat result is not an EA, adding back a dimension), so concat_compat does not need to be aware of this.

Partly overlapping with #33522 (cc @TomAugspurger), but this PR does not yet fix the original case the other PR is fixing: concat with reindexed (thus all missing) columns becoming object instead of preserving object dtype.

[pep8speaks]

Hello @jorisvandenbossche! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-18 13:27:00 UTC

[jorisvandenbossche]

There is a sparse-related failing test. This is because right now we have some buggy behaviour for concatting sparse on master -> #34338, #34336)

[jorisvandenbossche]

this change is to ensure DatetimeBlock and DatetimeTZBlock don't incorrectly take this "uniform join units" code path

[jorisvandenbossche]

cc @TomAugspurger @jbrockmendel

[TomAugspurger]

Sorry for the delay.

I didn't immediately see the reason for your changesin dtypes/concat, but I suppose it's related to

Now I also try to make use of this for DataFrame columns, if there is any extension dtype in the to-concat arrays.

So I think I'm following things now.

Do we need a whatsnew note?

[jorisvandenbossche]

I didn't immediately see the reason for your changesin dtypes/concat, but I suppose it's related to

So the change in dtypes/concat.py is if any_ea and axis == 0: changed to if any_ea:, ensuring that we now take the EA-specific code path for both axis=0 and axis=1.

Now, the axis is a bit confusing here, as we are actually always concatting the EAs in the same way (making one long EA), but the axis comes from the "higher" level whether we are concatting series or frames.
Now, when it comes to concatting EAs, this axis thus doesn't matter.

So that is indeed as you say related to the fact that I now also take the EA-specific code path for DataFrame columns (which result in axis=1) and not only for Series (axis=0).

When it comes to concat_compat and concatting EAs, the axis basically can be ignored.

[jorisvandenbossche]

Do we need a whatsnew note?

I added a general note about concat with extension arrays (that also references the previous PR that made this possible for Series).

[jorisvandenbossche]

This should be ready for review / merge now.

[jreback]

I find it odd that the logic is now split here and concat_compat. I would strongly encourage to put this logic there.

[jorisvandenbossche]

Having this here actually ensures a clear separation of concerns between dtypes/concat.py and internals/concat.py.

The concat_compat from dtypes/concat.py only needs to worry about concatting 1D arrays (for the case of ExtensionArrays, to be clear), and the code here in internals deals with ensuring the result is the proper dimension depending on how it is put in the BlockManager.

This keeps this BlockManager-related logic inside the internals, and ensures that concat_compat / dtypes/concat.py doesn't need to care about internals-specific details.

[jreback]

my point is you should have a concat_ea which you call here (right near concat_compat), otherwise the logic is scatttered. the *dispatching) logic is fine, the problem is the operational logic does not belong here. (alternative is to pass anotherr arg to concat_compat to do what you are doing here).

[jorisvandenbossche]

If there is a different concat_ea, then at each location where concat_compat is called, we would need to do a if any_ea: concat_ea(..); else: concat_compat(...)

What do you mean exactly with the "operational logic does not belong here". What part of the code below is the "operational" logic?

[jreback]

every bit you added is operational logic.

pls do something like this

elif any(isinstance(t, ExtensionArray) for t in to_concat):
    values = concat_ea(...)

[jreback]

this is moving L324-330. As I have said before you are mixing 2 types of logic here by adding this.

[jorisvandenbossche]

To make sure I understand you correctly. You want to see the following function be put in dtypes/concat.py:

def concat_ea_frame(to_concat):
    to_concat = [t if isinstance(t, ExtensionArray) else t[0, :] for t in to_concat]
    concat_values = concat_compat(to_concat, axis=concat_axis)
    if not isinstance(concat_values, ExtensionArray):
        # if the result of concat is not an EA but an ndarray, reshape to
        # 2D to put it a non-EA Block
        concat_values = np.atleast_2d(concat_values)
    return concat_values

and called her. Is that correct?

I can do that if it ends this discussion, but just to note: this would create a function in dtypes/concat.py with logic tied to the internals (reshaping to conform to block's dimenions is something only needs to be known to the internals code) and that would only be used in the internals (here, in internals/concat.py)

[jreback]

yes this is exactly inline with concat_dateime and concat_compat. I am surprised that you think this should be anywhere else.

[jbrockmendel]

with logic tied to the internals (reshaping to conform to block's dimenions is something only needs to be known to the internals code) and that would only be used in the internals

i agree with Joris; internals-specific logic belongs in internals

[jreback]

i guess

[TomAugspurger]

Planning to merge tomorrow if there are no objections.

[TomAugspurger]

Sorry @jorisvandenbossche, one more merge? :)

[jbrockmendel]

@jorisvandenbossche IIRC there was some discussion of implementing Categorical._concat_same_type directly in the method and having union_categoricals defer to the method rather than the other way around. Is that awaiting consensus or just awaiting implementation?

[jorisvandenbossche]

@jbrockmendel the discussion was here: #33607 (comment), AFAIK just awaiting someone doing it