[ArrayManager] Implement concat with axis=1 (merge/join) #39841

jorisvandenbossche · 2021-02-16T12:10:09Z

The (hopefully) "easy" part of #39612 for which there seems less discussion: this separates the pieces from #39612 that are needed to get concat with axis=1 working (concat_axis=0), which at least enables the join / merge usecases.

jorisvandenbossche · 2021-02-16T12:11:27Z

.github/workflows/ci.yml

@@ -153,6 +153,7 @@ jobs:
      run: |
        source activate pandas-dev
        pytest pandas/tests/frame/methods --array-manager
+        pytest pandas/tests/reshape/merge --array-manager


Only reshape/merge and not reshape/concat, as there are too many tests still failing in the concat tests (because axis=0 is not yet handled properly)

pandas/core/internals/array_manager.py

jbrockmendel · 2021-02-16T22:04:20Z

pandas/core/internals/concat.py

+        mgrs.append(mgr)
+
+    # concatting along the rows -> concat the reindexed arrays
+    # TODO(ArrayManager) doesn't yet preserve the correct dtype


does this comment apply to both cases or just concat_axis==1? (this ambiguity is why i like to put comments inside below the "if"/"else")

Moved the comment (yes, it applies only to the axis==1)

pandas/core/internals/concat.py

jbrockmendel · 2021-02-16T22:09:30Z

pandas/tests/reshape/merge/test_merge.py

+            for arr in data1._mgr.arrays:
+                arr.flags.writeable = False
+        else:
+            data1._mgr.blocks[0].values.flags.writeable = False


could implement BlockManager.arrays as a property yielding the values for compat

Yeah, but that would not help for this specific case (as those arrays would not be the actual arrays stored in the BlockManager, and setting the flag wouldn't have an impact on the actual values).

as those arrays would not be the actual arrays stored in the BlockManager

@property def arrays(self): for blk in self.blocks: yield blk.values

Ah, like that. I wouldn't find it great API design, as the same attribute/property would return something quite different (at least what you can do with it is different), but I am fine adding it for testing convenience

Added + test simplified

jbrockmendel · 2021-02-16T22:10:01Z

small comments, generally looks good

jreback · 2021-02-16T23:53:25Z

pandas/core/internals/array_manager.py

@@ -754,10 +754,12 @@ def reindex_indexer(
        # ignored keywords
        consolidate: bool = True,
        only_slice: bool = False,
+        # ArrayManager specific keywords
+        do_integrity_check: bool = True,


i think we call this integrity_check elsewhere

The BlockManager/ArrayManager constructors call this do_integrity_check, so it's consistent with that. We don't have integrity_check, but we do have verify_integrity elsewhere (and this is actually used more, also in public APIs).

I think it might be good to change do_integrity_check to verify_integrity, but given that currently do_integrity_check is consistently used within the (already existing) internals code, that's something for a different PR, I would say.

right that's what i mean. can you normalize all to verify_integrity

can you normalize all to verify_integrity

Since that's not strictly related to concat or ArrayManager, will do that in a follow-up PR

jreback · 2021-02-16T23:54:36Z

pandas/core/internals/concat.py

+        arrays = list(itertools.chain.from_iterable([mgr.arrays for mgr in mgrs]))
+        return ArrayManager(arrays, [axes[1], axes[0]], do_integrity_check=False)
+
+
 def concatenate_block_managers(


can you at a higher level call concatenate_array_managers instead? (e.g. instead of dispatching thru the BM)

That would require adding if/else checking the manager type in each of the places where currently concatenate_array_managers is called (3 places), so that would not improve the code there.

If you want I can create a new concatenate_managers in this file that simply does the dispatch to concatenate_block_managers / concatenate_array_managers, and call that instead of concatenate_block_managers outside of the /internals

ok maybe rename this then to concatenate_managers? to make it much more obvious.

jreback · 2021-02-17T00:00:29Z

OT should we create a Internal - ArrayManager label?

jreback

small comments

jreback · 2021-02-19T01:22:09Z

pandas/tests/reshape/merge/test_merge.py

@@ -1381,7 +1391,9 @@ def test_merge_readonly(self):
            np.arange(20).reshape((5, 4)) + 1, columns=["a", "b", "x", "y"]
        )

-        data1._mgr.blocks[0].values.flags.writeable = False
+        for arr in data1._mgr.arrays:


cabn you comment on this (I know what you are doing by seeing what you added, but is non-obvious here i think)

jorisvandenbossche · 2021-02-23T10:13:37Z

The windows failure is also happening on master, and the linux failure is a flaky resource warning, so both unrelated.

Thanks for the reviews! Going to merge now, and will do the verify_integrity rename in a follow-up.

jbrockmendel · 2021-02-23T21:21:51Z

pandas/core/internals/managers.py

@@ -282,6 +282,18 @@ def get_dtypes(self):
        dtypes = np.array([blk.dtype for blk in self.blocks])
        return algos.take_nd(dtypes, self.blknos, allow_fill=False)

+    @property
+    def arrays(self):


[ArrayManager] ENH: implement concat with axis=1 (merge/join)

072ed39

jorisvandenbossche changed the title ~~[ArrayManager] ENH: implement concat with axis=1 (merge/join)~~ [ArrayManager] Implement concat with axis=1 (merge/join) Feb 16, 2021

jorisvandenbossche commented Feb 16, 2021

View reviewed changes

jorisvandenbossche mentioned this pull request Feb 16, 2021

[ArrayManager] REF: Implement concat with reindexing #39612

Merged

jbrockmendel reviewed Feb 16, 2021

View reviewed changes

pandas/core/internals/array_manager.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Feb 16, 2021

View reviewed changes

pandas/core/internals/concat.py Show resolved Hide resolved

jbrockmendel reviewed Feb 16, 2021

View reviewed changes

jreback requested changes Feb 16, 2021

View reviewed changes

jreback added Internals Related to non-user accessible pandas implementation Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Feb 17, 2021

jorisvandenbossche added 8 commits February 17, 2021 10:35

Merge remote-tracking branch 'upstream/master' into am-concat-axis-0

dd14d2d

Merge remote-tracking branch 'upstream/master' into am-concat-axis-0

4da4433

address feedback

31cfb0d

Merge remote-tracking branch 'upstream/master' into am-concat-axis-0

480b32d

add array property

3488d63

simplify do_integrity_check

437c1d2

clean-up unused using_array_manager

2597cd5

remove keyword-only indicator

31d1305

jreback reviewed Feb 19, 2021

View reviewed changes

jorisvandenbossche added 3 commits February 23, 2021 09:50

Merge remote-tracking branch 'upstream/master' into am-concat-axis-0

b6a6b54

concatenate_block_managers -> concatenate_managers

3f5fc38

add comment

54692ca

jorisvandenbossche merged commit e19403d into pandas-dev:master Feb 23, 2021

jorisvandenbossche deleted the am-concat-axis-0 branch February 23, 2021 10:13

This was referenced Feb 23, 2021

CLN: rename do_integrity_check to verify_integrity in internals #39989

Merged

[ArrayManager] TST: get tests running for /tests/frame #39700

Merged

jbrockmendel reviewed Feb 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ArrayManager] Implement concat with axis=1 (merge/join) #39841

[ArrayManager] Implement concat with axis=1 (merge/join) #39841

jorisvandenbossche commented Feb 16, 2021

jorisvandenbossche Feb 16, 2021

jbrockmendel Feb 16, 2021

jorisvandenbossche Feb 17, 2021

jbrockmendel Feb 16, 2021

jorisvandenbossche Feb 17, 2021

jbrockmendel Feb 17, 2021

jorisvandenbossche Feb 17, 2021

jorisvandenbossche Feb 18, 2021 •

edited

Loading

jbrockmendel commented Feb 16, 2021

jreback Feb 16, 2021

jorisvandenbossche Feb 17, 2021 •

edited

Loading

jreback Feb 19, 2021

jorisvandenbossche Feb 23, 2021

jorisvandenbossche Feb 23, 2021

jreback Feb 16, 2021

jorisvandenbossche Feb 17, 2021 •

edited

Loading

jreback Feb 19, 2021

jreback commented Feb 17, 2021

jreback left a comment

jreback Feb 19, 2021

jorisvandenbossche commented Feb 23, 2021

jbrockmendel Feb 23, 2021

[ArrayManager] Implement concat with axis=1 (merge/join) #39841

[ArrayManager] Implement concat with axis=1 (merge/join) #39841

Conversation

jorisvandenbossche commented Feb 16, 2021

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

jorisvandenbossche Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

jbrockmendel commented Feb 16, 2021

Choose a reason for hiding this comment

jorisvandenbossche Feb 17, 2021 • edited Loading

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

jorisvandenbossche Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 17, 2021

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Feb 23, 2021

Choose a reason for hiding this comment

jorisvandenbossche Feb 18, 2021 •

edited

Loading

jorisvandenbossche Feb 17, 2021 •

edited

Loading

jorisvandenbossche Feb 17, 2021 •

edited

Loading