ENH: .equals for Extension Arrays #30652

dwhu · 2020-01-03T18:01:16Z

closes API: .equals for Extension Arrays #27081
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

--- Jan 3 10:00 PST

Hey team,

Looking for a bit of feedback on this API design and appropriate testing for this PR before I get too deep into it. Could you provide some guidance regarding how to appropriately handle updating this test?

pandas/pandas/tests/extension/base/ops.py

Line 162 in 6f03e76

def test_direct_arith_with_series_returns_not_implemented(self, data):

It appears to contradict the issue #27081.

Appreciate the help,
Dave

charlesdong1991 · 2020-01-03T18:23:34Z

thanks for your contribution! @dwhu

in general here, if it is NotImplemted, and you implement some new functionalities, you could remove this one, and replace it with the corresponding tests to test your new implementations!

Good luck! and hope this helps

dwhu · 2020-01-03T18:44:35Z

in general here, if it is NotImplemted, and you implement some new functionalities, you could remove this one, and replace it with the corresponding tests to test your new implementations!

@charlesdong1991 - Sounds good. Thanks for the feedback. Wanted to make sure I wasn't missing something or breaking a behavior contract by adding this behavior.

pandas/core/arrays/base.py

TomAugspurger

I think most of the __eq__ implementations you've added can be removed.

@jschendel is in the middle of a PR fixing IntervalArray.__eq__, so ignore that for now if it's causing failures.

We'll need some base tests in pandas/tests/extensions/base/ops.py ensuring that __eq__ and __ne__ are implemented correctly. We'll want a few tests in tests/extensions/base/methods.py ensuring that .equals is tested. These tests are a bit tricky to write, so let us know if you have issues.

TomAugspurger · 2020-01-03T19:24:04Z

pandas/core/arrays/base.py

+        bool
+        """
+
+        raise AbstractMethodError(self)


Can this be return ~(self == other)?

TomAugspurger · 2020-01-03T19:25:44Z

pandas/core/arrays/base.py

+            Whether the arrays are equivalent.
+
+        """
+        return ((self == other) | (self.isna() == other.isna())).all()


I think this also needs to ensure that the type of other matches the type of self. We don't want pd.array([1, 2]).equals(np.array([1, 2])) to be True (I think).

TomAugspurger · 2020-01-03T19:27:29Z

pandas/core/arrays/boolean.py

+        if not isinstance(other, BooleanArray):
+            return NotImplemented
+        return (
+            hasattr(other, "_data")


BooleanArray should already have a __eq__ method (see create_comparison_method Was this needed?

Not needed. Missed that in my WIP PR.

Adding __eq__ to ExtensionArray Abstract method doc string. Adding ne implementation to EA base class. Also removing other implementations. Updating EA equals method and adding tests. pandas-devGH-27081

…chendel

…chendel change.

TomAugspurger · 2020-01-03T21:57:20Z

pandas/core/arrays/base.py

        for i in range(len(self)):
            yield self[i]

+    def __eq__(self, other: ABCExtensionArray) -> bool:


The type of other should be Any I think. Passing a list / etc here is valid, it just returns False.

Make sure to update the type in the docstring too.

TomAugspurger · 2020-01-03T21:57:30Z

pandas/core/arrays/base.py

+
+        raise AbstractMethodError(self)
+
+    def __ne__(self, other: ABCExtensionArray) -> bool:


Same comment about the type on other.

TomAugspurger · 2020-01-03T21:59:32Z

pandas/core/arrays/base.py

+        bool
+        """
+
+        raise AbstractMethodError(self)


Actually, does this need to be abstract? Or can we define it as something like

# check that the types, length match, etc. # check that NA values in self match NA values in other... return np.all(np.array(self) == np.array(other))

Then subclasses can override with implementations that may be faster?

Good suggestion. I like that much better.

TomAugspurger · 2020-01-03T22:01:27Z

pandas/core/arrays/base.py

+            Whether the arrays are equivalent.
+
+        """
+        return isinstance(other, self.__class__) and (


Perhaps type(self) == type(other)? It's not clear to me whether a subclass should compare .equals to a parent class. Thoughts @jreback?

Will want some length checking here too.

Added type(self) == type(other) and length checks.

TomAugspurger · 2020-01-03T22:02:44Z

pandas/core/arrays/integer.py

+    def __eq__(self, other):
+        return (
+            isinstance(other, IntegerArray)
+            and hasattr(other, "_data")


We already have IntegerArray.__eq__ on master. This shouldn't be necessary.

TomAugspurger · 2020-01-03T22:04:37Z

pandas/tests/extension/base/methods.py

+        ser = pd.Series(cls._from_sequence(data, dtype=data.dtype))
+        na_ser = pd.Series(cls._from_sequence([na_value], dtype=data.dtype))
+
+        assert data.equals(data)


make all these assert <condition> is True, to ensure that True or False is actually returned

I ran into a funny issue doing this: numpy/numpy#5942
I'm going to keep this as is now to keep the test consistent within itself.

.equals() returns a np.bool_ type. As result when you compare it to bool type True using is (i.e. some_np_bool_True_val is True == False

Coercing np.bool_ to bool using the bool() constructor did not work either. == True causes PEP8 errors asking me to use assert <condition> or assert <condition> is True.

Open to suggestions if you have any.

TomAugspurger · 2020-01-03T22:05:27Z

pandas/tests/extension/base/methods.py

+        assert not na_ser.equals(ser)
+        assert not ser.equals(na_ser)
+        assert not ser.equals(0)
+        assert not na_ser.equals(0)


Add test for other inputs like an ndarray.

Add a test for unequal length things.

pandas/tests/extension/base/ops.py

TomAugspurger · 2020-01-03T22:06:38Z

pandas/tests/extension/base/ops.py

        other = pd.Series([data[0]] * len(data))
        self._compare_other(s, data, op_name, other)
-
-    def test_direct_arith_with_series_returns_not_implemented(self, data):


Undo this change.

Same as the other comment. The whole PR is intended to break this test since we implement __eq__ and __ne__.

Same as the other comment. The whole PR is intended to break this test since we implement eq and ne.

The reason this needs to be kept is that, although __eq__ is a required method on an ExtensionArray now, we still require it to return NotImplemented when other is a Series or Index.

pandas-devGH-27081

WillAyd · 2020-02-12T00:35:15Z

@dwhu looks like CI is red - can you take a look and fix up the merge conflict?

WillAyd · 2020-03-14T21:56:14Z

Closing as I think this has gone stale but ping if you'd like to pick back up. Thanks!

jorisvandenbossche · 2020-05-01T14:59:21Z

@dwhu I am going to look into further updating this, if that's OK with you

jorisvandenbossche · 2020-05-01T19:07:00Z

OK, I updated this with the following things:

Made __eq__ into an abstract method so it is required to implement (another option could be to return NotImplemented as default, but that's actually wrong, as it results in not doing element-wise comparison for EA == EA, but just returning False)
Fixed the equals imlementation: it became a bit longer to ensure we check for BooleanArray (if the == comparison returns a nullable boolean array, we need to fill NAs with False)
Added ExtensionBlock.equals (overriding the base Block method) to ensure we dispatch to the new EA method
A bit of clean-up of the tests

Should be ready for a new review now @TomAugspurger @jbrockmendel

TomAugspurger

Looks nice. Can you add a release note?

TomAugspurger · 2020-05-01T19:18:23Z

pandas/core/arrays/base.py


+    def __eq__(self, other: Any) -> Union[np.ndarray, "ExtensionArray"]:
+        """
+        Return for `self == other` (element-wise equality).


I think we should have some guidance here that if other is a Series then you should return NotImplemented.

could use ops.common.unpack_zerodim_and_defer

could use ops.common.unpack_zerodim_and_defer

This method is to be implemented by EA authors, so those can't use that helper (unless we expose somewhere a public version of this).

(we could of course use that for our own EAs, but this PR is not changing any existing __eq__ implementation at the moment)

I think we should have some guidance here that if other is a Series then you should return NotImplemented.

Added a comment about that

TomAugspurger · 2020-05-01T19:19:35Z

pandas/core/arrays/base.py

+        if not type(self) == type(other):
+            return False


We should verify that this is the behavior we want. Namely

other array-likes are not equivalent, even if they are all equal.

subclasses are not equivalent, even if they are all equal.

The first seems fine. Not sure about the second.

I was planning to open an issue about this after this PR (and keep it strict here), because this is right now a bit inconsistent within pandas, and might require a more general discussion / clean-up (eg Series.equals is more strict (requires same dtype) than Index.equals ...)
But we can certainly also have the discussion here.

Happy to be strict for now.

I made it even more strict (same dtype, not just same class), and added a test for that.
Will open an issue for the general discussion.

I opened an issue for this at #33940

In the end, it seems mainly to come to whether the dtype should exactly be equal or not.

Since for EAs, the dtype is right now tied to the array class, using equal dtype for now also implies the same class (no additional check to allow sublcasses).

TomAugspurger · 2020-05-01T19:22:24Z

pandas/tests/extension/base/methods.py

            else:
                data.repeat(repeats, **kwargs)
+
+    def test_equals(self, data, na_value, as_series):


Can you also include DataFrame in this parameterization, or do it somewhere else? Just to ensure coverage of the changes to blocks.py.

jschendel · 2020-05-01T19:38:57Z

pandas/core/arrays/base.py

+        """
+        raise AbstractMethodError(self)
+
+    def __ne__(self, other: Any) -> Union[np.ndarray, "ExtensionArray"]:


This should allow us to remove the equivalent IntervalArray definition here:

pandas/pandas/core/arrays/interval.py

Lines 609 to 610 in d149f41

def __ne__(self, other):

return ~self.__eq__(other)

Union[np.ndarray, "ExtensionArray"] -> ArrayLike

@jschendel done!

jbrockmendel · 2020-05-01T22:55:48Z

pandas/core/arrays/base.py

+                # boolean array with NA -> fill with False
+                equal_values = equal_values.fillna(False)
+            equal_na = self.isna() & other.isna()
+            return (equal_values | equal_na).all().item()


is the .item() necessary?

is the .item() necessary?

It's to have a python bool, instead of a numpy bool, as result.
I added a comment to the tests to make it explicit this is the reason we are asserting with is True/False (and later on we don't inadvertedly "clean" that up)

thanks for clarifying. i was recently reminded that np.array(np.timedelta64(1234, "ns")).item() gives an int instead of timedelta64, so im now cautious around .item()

jorisvandenbossche · 2020-05-02T08:00:41Z

@simonjayhawkins how do I solve this typing error?

pandas/core/arrays/base.py:338: error: Return type "Union[Any, ExtensionArray]" of "__eq__" incompatible with return type "bool" in supertype "object"

So our annotation of __eq__ is incompatible with the base "object" class, but we do this on purpose. So is there another way to solve this than to suppress the error in some way?

jorisvandenbossche · 2020-05-05T11:37:18Z

Any more comments on this PR?

jreback · 2020-05-05T11:43:16Z

will look soon

jbrockmendel · 2020-05-06T18:01:14Z

pandas/core/arrays/base.py

            yield self[i]

-    def __eq__(self, other: Any) -> ArrayLike:
+    def __eq__(self, other: Any) -> ArrayLike:  # type: ignore[override] # NOQA


im not familiar with the ignore[overrride] # NOQA pattern. do we use that elsewhere?

I thought I posted the mypy issue, but apparently forgot.

So the problem here is that mypy expects a "bool" return for __eq__ since the base object is typed that way in the stubs. In python/mypy#2783, the recommended way to solve this is the above with # type: ignore[override].

But flake8 doesn't like that, hence also the #NOQA (PyCQA/pyflakes#475).

This was already done in another PR as well:

pandas/pandas/core/frame.py

Line 5062 in 7aa710a

def sort_values( # type: ignore[override] # NOQA # issue 27237

jorisvandenbossche · 2020-05-07T20:23:45Z

@jreback you wanted to take a look here?

jreback

lgtm. a couple of minor comment.s

jreback · 2020-05-07T21:41:05Z

doc/source/whatsnew/v1.1.0.rst

  such as ``dict`` and ``list``, mirroring the behavior of :meth:`DataFrame.update` (:issue:`33215`)
 - :meth:`~pandas.core.groupby.GroupBy.transform` and :meth:`~pandas.core.groupby.GroupBy.aggregate` has gained ``engine`` and ``engine_kwargs`` arguments that supports executing functions with ``Numba`` (:issue:`32854`, :issue:`33388`)
 - :meth:`~pandas.core.resample.Resampler.interpolate` now supports SciPy interpolation method :class:`scipy.interpolate.CubicSpline` as method ``cubicspline`` (:issue:`33670`)
+- The ``ExtensionArray`` class has now an ``equals`` method, similarly to ``Series.equals()`` (:issue:`27081`).


you can add the doc reference here

jreback · 2020-05-07T21:43:17Z

pandas/core/internals/blocks.py

        return [self.make_block_same_class(result, placement=self.mgr_locs)]

+    def equals(self, other) -> bool:
+        if self.dtype != other.dtype or self.shape != other.shape:


why do you need this check here? (its already done on the .values no?)

Hmm, good question. I just copied that from the base Block.equals method (so it's done there as well). There, array_equivalent is used. Maybe that is less strict and the extra check is needed, which is not needed here.

At least from the docstring of array_equivalent (not sure how up to date that is), using that method indeed requires a more strict check in advance of calling it:

pandas/pandas/core/dtypes/missing.py

Lines 360 to 363 in 3ed7dff

in corresponding locations. False otherwise. It is assumed that left and

right are NumPy arrays of the same dtype. The behavior of this function

(particularly with respect to NaNs) is not defined if the dtypes are

different.

But so, for EA.equals that is not needed, so will remove that check here.

jorisvandenbossche · 2020-05-09T07:56:31Z

Travis failures are unrelated (network issues, S3)

Co-authored-by: Joris Van den Bossche <[email protected]>

TomAugspurger reviewed Jan 3, 2020

View reviewed changes

pandas/core/arrays/base.py Outdated Show resolved Hide resolved

TomAugspurger reviewed Jan 3, 2020

View reviewed changes

dwhu added 2 commits January 3, 2020 13:40

ENH: .equals for Extension Arrays

36c8b88

ENH: Updating eq and ne methods for extension arrays.

786963c

Adding __eq__ to ExtensionArray Abstract method doc string. Adding ne implementation to EA base class. Also removing other implementations. Updating EA equals method and adding tests. pandas-devGH-27081

dwhu force-pushed the gh-27081 branch from 1919814 to 786963c Compare January 3, 2020 21:44

Removing interval.py's __eq__ implementation due to conflict with @js…

6800315

…chendel change.

dwhu changed the title ~~[WIP] ENH: .equals for Extension Arrays~~ ENH: .equals for Extension Arrays Jan 3, 2020

dwhu requested a review from TomAugspurger January 3, 2020 21:54

TomAugspurger added the ExtensionArray Extending pandas with custom dtypes or arrays. label Jan 3, 2020

TomAugspurger reviewed Jan 3, 2020

View reviewed changes

dwhu added 2 commits January 3, 2020 14:28

ENH: Making EA eq and ne typed as Any.

a3e7b7f

pandas-devGH-27081

ENH: Adding default implementation to ExtensionArray equals() and tests.

860013f

pandas-devGH-27081

dwhu requested a review from TomAugspurger January 3, 2020 23:33

Merge remote-tracking branch 'upstream/master' into pandas-devgh-27081

fc3d2c2

WillAyd closed this Mar 14, 2020

jorisvandenbossche reopened this May 1, 2020

Merge remote-tracking branch 'upstream/master' into pandas-devgh-27081

3da5726

jorisvandenbossche added 4 commits May 1, 2020 17:39

correct __eq/ne__ to be element-wise

c5027dd

fix equals implementation (& instead of ==)

375664c

base tests

b6ad2fb

ensure to dispatch Series.equals to EA.equals

365362a

jorisvandenbossche added this to the 1.1 milestone May 1, 2020

TomAugspurger reviewed May 1, 2020

View reviewed changes

jschendel reviewed May 1, 2020

View reviewed changes

jbrockmendel reviewed May 1, 2020

View reviewed changes

jorisvandenbossche added 3 commits May 2, 2020 09:23

Merge remote-tracking branch 'upstream/master' into pandas-devgh-27081

aae2f94

feedback: docs, whatsnew, dataframe test, strict dtype test

8d052ad

add to reference docs

9ee034e

remove IntervalArray.__ne__

38501e6

jorisvandenbossche mentioned this pull request May 2, 2020

API: how strict should the equals() method be? #33940

Open

type ignore following mypy issue (mypy/2783)

dccec7f

jbrockmendel reviewed May 6, 2020

View reviewed changes

jorisvandenbossche added 2 commits May 7, 2020 22:07

Merge remote-tracking branch 'upstream/master' into pandas-devgh-27081

0b1255f

try again without type: ignore

b8be858

jreback requested changes May 7, 2020

View reviewed changes

updates

4c7273f

jorisvandenbossche merged commit f21bc99 into pandas-dev:master May 9, 2020

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020

ENH: .equals for Extension Arrays (pandas-dev#30652)

67b0cc4

Co-authored-by: Joris Van den Bossche <[email protected]>


		raise AbstractMethodError(self)

		def __ne__(self, other: ABCExtensionArray) -> bool:

	in corresponding locations. False otherwise. It is assumed that left and
	right are NumPy arrays of the same dtype. The behavior of this function
	(particularly with respect to NaNs) is not defined if the dtypes are
	different.

Uh oh!

ENH: .equals for Extension Arrays #30652

ENH: .equals for Extension Arrays #30652

Uh oh!

Conversation

dwhu commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charlesdong1991 commented Jan 3, 2020

Uh oh!

dwhu commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TomAugspurger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Feb 12, 2020

Uh oh!

WillAyd commented Mar 14, 2020

Uh oh!

jorisvandenbossche commented May 1, 2020

Uh oh!

jorisvandenbossche commented May 1, 2020

Uh oh!

TomAugspurger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dwhu commented Jan 3, 2020 •

edited

Loading

dwhu commented Jan 3, 2020 •

edited

Loading