REF: dont consolidate in BlockManager.equals #34962

jbrockmendel · 2020-06-24T01:57:30Z

Avoid side-effects.

Side-note: with EA.equals recently added to the interface, should we update array_equivalent to use it? ATM array_equivalent starts by casting both inputs to ndarray.

jorisvandenbossche

With this change, we no longer use the Block.equals methods?

jorisvandenbossche · 2020-06-24T16:31:26Z

pandas/core/internals/managers.py

+                return False
+            elif isinstance(left, ExtensionArray):
+                if not left.equals(right):
+                    return False


this can be return left.equals(right) ?

And same for array_equivalent below?

yes, will update

no, wait, not here. this is an "all" check, so we only return on False.

Ah, yes, it's inside the for loop ..

jorisvandenbossche · 2020-06-24T16:33:23Z

Side-note: with EA.equals recently added to the interface, should we update array_equivalent to use it?

Probably, yes

jbrockmendel · 2020-06-24T17:53:52Z

With this change, we no longer use the Block.equals methods?

I think this is correct, will double-check and update.

…f-mgr-equals-2

jreback · 2020-06-24T23:22:51Z

pandas/core/internals/managers.py

-        return all(
-            block.equals(oblock) for block, oblock in zip(self_blocks, other_blocks)
-        )
+        if self.ndim == 1:


can you give some comments here (or at least a comment about the ordering of the checks)

jreback · 2020-06-24T23:22:58Z

pandas/core/internals/managers.py

+            else:
+                return array_equivalent(blk.values, rblk.values)
+
+        for i in range(len(self.items)):


comment added

…f-mgr-equals-2

jbrockmendel · 2020-07-05T20:51:27Z

gentle ping

jreback · 2020-07-05T21:14:29Z

pandas/tests/internals/test_internals.py

@@ -389,7 +389,7 @@ def test_copy(self, mgr):

            # copy assertion we either have a None for a base or in case of
            # some blocks it is an array (e.g. datetimetz), but was copied
-            assert cp_blk.equals(blk)
+            tm.assert_equal(cp_blk.values, blk.values)


should equals also work here?

No, Block.equals is removed

* REF: dont consolidate in BlockManager.equals * doctest fixup * Remove Block.equals * simplify, comments

TomAugspurger · 2020-07-08T11:22:08Z

This had a large performance regression in https://pandas.pydata.org/speed/pandas/index.html#frame_methods.Equals.time_frame_float_equal?commits=77f4a4d673e2d2b3f7396a94818771d332bd913c. Is that expected?

jorisvandenbossche · 2020-07-08T11:45:32Z

Some slowdown can probably expected for this case since it went from block-wise to column-wise (while having a consolidated block).

But I think a large part of the slowdown is due to array_equivalent being much slower for certain dtypes right now. For example, the FloatBlock had a custom equals method not using array_equivalent but:

def equals(self, other) -> bool:
        if self.dtype != other.dtype or self.shape != other.shape:
            return False
        left, right = self.values, other.values
        return ((left == right) | (np.isnan(left) & np.isnan(right))).all()

and this is quite a bit faster:

In [5]: a = np.random.randn(N)      

In [6]: %timeit ((a == a) | (np.isnan(a) & np.isnan(a))).all()    
5.28 µs ± 366 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [8]: %timeit pd.core.dtypes.missing.array_equivalent(a, a)    
57.7 µs ± 2.2 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

jorisvandenbossche · 2020-07-08T11:46:55Z

I suppose that a similar "fastpath" could be added to array_equivalent, though?

Also for DatetimeTZBlock, there was an override of equals for performance:

    def equals(self, other) -> bool:
        # override for significant performance improvement
        if self.dtype != other.dtype or self.shape != other.shape:
            return False
        return (self.values.view("i8") == other.values.view("i8")).all()

jbrockmendel · 2020-07-08T16:42:49Z

I think the thing to do here is optimize array_equivalent.

jbrockmendel · 2020-07-08T16:43:31Z

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: dont consolidate in BlockManager.equals #34962

REF: dont consolidate in BlockManager.equals #34962

jbrockmendel commented Jun 24, 2020

jorisvandenbossche left a comment

jorisvandenbossche Jun 24, 2020

jbrockmendel Jun 24, 2020

jbrockmendel Jun 24, 2020

jorisvandenbossche Jun 25, 2020

jorisvandenbossche commented Jun 24, 2020

jbrockmendel commented Jun 24, 2020

jreback Jun 24, 2020

jreback Jun 24, 2020

jbrockmendel Jun 26, 2020

jbrockmendel commented Jul 5, 2020

jreback Jul 5, 2020

jbrockmendel Jul 5, 2020

TomAugspurger commented Jul 8, 2020

jorisvandenbossche commented Jul 8, 2020 •

edited

Loading

jorisvandenbossche commented Jul 8, 2020

jbrockmendel commented Jul 8, 2020

jbrockmendel commented Jul 8, 2020

REF: dont consolidate in BlockManager.equals #34962

REF: dont consolidate in BlockManager.equals #34962

Conversation

jbrockmendel commented Jun 24, 2020

jorisvandenbossche left a 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 commented Jun 24, 2020

jbrockmendel commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jul 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Jul 8, 2020

jorisvandenbossche commented Jul 8, 2020 • edited Loading

jorisvandenbossche commented Jul 8, 2020

jbrockmendel commented Jul 8, 2020

jbrockmendel commented Jul 8, 2020

jorisvandenbossche commented Jul 8, 2020 •

edited

Loading