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

jorisvandenbossche · 2020-05-23T13:11:22Z

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 · 2020-05-23T13:11:28Z

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)

pandas/core/arrays/integer.py

jorisvandenbossche · 2020-05-23T13:14:55Z

pandas/core/internals/concat.py

@@ -443,7 +452,7 @@ def _is_uniform_join_units(join_units: List[JoinUnit]) -> bool:
    #  cannot necessarily join
    return (
        # all blocks need to have the same type
-        all(isinstance(ju.block, type(join_units[0].block)) for ju in join_units)
+        all(type(ju.block) is type(join_units[0].block) for ju in join_units)


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

pandas/tests/extension/base/reshaping.py

jorisvandenbossche · 2020-05-28T20:40:12Z

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?

pandas/tests/reshape/test_concat.py

jorisvandenbossche · 2020-05-29T07:31:50Z

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 · 2020-06-06T08:57:10Z

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 · 2020-06-06T08:57:23Z

This should be ready for review / merge now.

doc/source/whatsnew/v1.1.0.rst

jreback · 2020-06-08T01:42:00Z

pandas/core/internals/concat.py

@@ -319,6 +319,15 @@ def _concatenate_join_units(join_units, concat_axis, copy):
                    concat_values = concat_values.copy()
            else:
                concat_values = concat_values.copy()
+    elif any(isinstance(t, ExtensionArray) for t in to_concat):
+        # concatting with at least one EA means we are concatting a single column


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

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.

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).

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?

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(...)

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

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)

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

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

pandas/tests/indexing/test_indexing.py

doc/source/whatsnew/v1.1.0.rst

TomAugspurger · 2020-06-17T18:18:54Z

Planning to merge tomorrow if there are no objections.

Co-authored-by: Tom Augspurger <TomAugspurger@users.noreply.github.com>

TomAugspurger · 2020-06-18T13:16:09Z

Sorry @jorisvandenbossche, one more merge? :)

…pandas-dev#34339)

jbrockmendel · 2020-09-06T20:43:51Z

@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 · 2020-09-07T10:52:47Z

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

jorisvandenbossche added 5 commits May 23, 2020 09:00

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

e130060

Merge remote-tracking branch 'upstream/master' into EA-concat-frame

ea33cfd

clean-up

f83338a

fix concat of DatetimeBlock / DatetimeTZBlock to not take 'uniform' path

5822694

fix base extension tests

3d32bc8

jorisvandenbossche added Bug ExtensionArray Extending pandas with custom dtypes or arrays. Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 23, 2020

jorisvandenbossche added this to the 1.1 milestone May 23, 2020

jorisvandenbossche commented May 23, 2020

View reviewed changes

TomAugspurger reviewed May 28, 2020

View reviewed changes

pandas/tests/reshape/test_concat.py Show resolved Hide resolved

jorisvandenbossche added 3 commits June 6, 2020 09:44

Merge remote-tracking branch 'upstream/master' into EA-concat-frame

951e356

fix test

d269229

add whatsnew

63a6570

jreback requested changes Jun 8, 2020

View reviewed changes

jorisvandenbossche added 2 commits June 15, 2020 08:52

Merge remote-tracking branch 'upstream/master' into EA-concat-frame

e731802

update whatsnew

d8faef5

TomAugspurger approved these changes Jun 17, 2020

View reviewed changes

doc/source/whatsnew/v1.1.0.rst Outdated Show resolved Hide resolved

jreback approved these changes Jun 17, 2020

View reviewed changes

Update doc/source/whatsnew/v1.1.0.rst

2e1f1a6

Co-authored-by: Tom Augspurger <TomAugspurger@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into EA-concat-frame

94fe89d

jorisvandenbossche merged commit f64df3a into pandas-dev:master Jun 19, 2020

jorisvandenbossche deleted the EA-concat-frame branch June 19, 2020 09:16

jorisvandenbossche mentioned this pull request Jun 19, 2020

BUG: Fixed concat with reindex and extension types #33522

Merged

wesbarnett pushed a commit to wesbarnett/pandas that referenced this pull request Jun 20, 2020

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

c5b8006

…pandas-dev#34339)

jorisvandenbossche mentioned this pull request Jul 15, 2020

Overview of impact / achievements of the CZI grant (round 1) TomAugspurger/pandas-czi#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jorisvandenbossche commented May 23, 2020

pep8speaks commented May 23, 2020 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche May 23, 2020

jorisvandenbossche commented May 28, 2020

TomAugspurger left a comment

jorisvandenbossche commented May 29, 2020

jorisvandenbossche commented Jun 6, 2020

jorisvandenbossche commented Jun 6, 2020

jreback Jun 8, 2020

jorisvandenbossche Jun 10, 2020

jreback Jun 14, 2020

jorisvandenbossche Jun 14, 2020

jreback Jun 14, 2020

jreback Jun 14, 2020

jorisvandenbossche Jun 14, 2020

jreback Jun 14, 2020

jbrockmendel Jun 14, 2020

jreback Jun 15, 2020

TomAugspurger commented Jun 17, 2020 •

edited

Loading

TomAugspurger commented Jun 18, 2020

jbrockmendel commented Sep 6, 2020

jorisvandenbossche commented Sep 7, 2020

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

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

Conversation

jorisvandenbossche commented May 23, 2020

pep8speaks commented May 23, 2020 • edited Loading

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

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented May 28, 2020

TomAugspurger left a comment

Choose a reason for hiding this comment

jorisvandenbossche commented May 29, 2020

jorisvandenbossche commented Jun 6, 2020

jorisvandenbossche commented Jun 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Jun 17, 2020 • edited Loading

TomAugspurger commented Jun 18, 2020

jbrockmendel commented Sep 6, 2020

jorisvandenbossche commented Sep 7, 2020

pep8speaks commented May 23, 2020 •

edited

Loading

TomAugspurger commented Jun 17, 2020 •

edited

Loading