Repository: pandas-dev/pandas
Pull request: ENH: .equals for Extension Arrays
- [x] closes #27081
- [x] tests added / passed
- [x] passes `black pandas`
- [x] 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? https://github.com/pandas-dev/pandas/blob/6f03e76f9d47ecfcfdd44641de6df1fc7dd57a01/pandas/tests/extension/base/ops.py#L162 It appears to contradict the issue #27081.

Appreciate the help,
Dave


Problem statement:
API: .equals for Extension Arrays
We don't have a good method of testing for equality between EA's

```
In [19]: from pandas.tests.extension.decimal.array import DecimalArray                                                                                                                                                                                           

In [20]: from decimal import Decimal                                                                                                                                                                                                                             

In [21]: x = DecimalArray([Decimal('1'),Decimal('Nan')])                                                                                                                                                                                                         

In [22]: x == x                                                                                                                                                                                                                                                  
Out[22]: array([ True, False])

In [23]: x = pd.Series([1,np.nan], dtype='Int64').array                                                                                                                                                                                                          

In [24]: x == x                                                                                                                                                                                                                                                  
Out[24]: array([ True, False])

```

These *happen* to work with Series now because the null checks are handled at a higher level.
```
In [26]: x.equals(x)                                                                                                                                                                                                                                             
Out[26]: True

In [27]: x = pd.Series(DecimalArray([Decimal('1'),Decimal('Nan')]))                                                                                                                                                                                              

In [28]: x.equals(x)                                                                                                                                                                                                                                             
Out[28]: True
```
we could provide a default implementation that should just work in the presence of NA.

```
def equals(self, other):
    return ((self == other) | (self.isna() == other.isna())).all()
```

actually we should *also* implement a default ``__eq__``

Hints from issue discussion:
cc @jorisvandenbossche @TomAugspurger 
actually we define ``__eq__`` if you subclass from ExtensionArrayScalarMixin, but we need to change to use the above (and ``__ne__``).
+1 on adding `equals` and such a default implementation.

We might only need to add `__eq__` as a required abstract method, to ensure people implement it (otherwise the default python one will always return True or False based on identity, and thus give wrong results for such an `equals` method)
+1 for `.equals`, and making `__eq__` abstract so that the expected
array-like `__eq__` must be implemented.

On Thu, Jun 27, 2019 at 2:13 PM Joris Van den Bossche <
notifications@github.com> wrote:

> +1 on adding equals and such a default implementation.
>
> We might only need to add __eq__ as a required abstract method, to ensure
> people implement it (otherwise the default python one will always return
> True or False based on identity, and thus give wrong results for such an
> equals method)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/issues/27081?email_source=notifications&email_token=AAKAOIVGP7LWBI23LK4Q53LP4UGMFA5CNFSM4H36BQPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYDB7Y#issuecomment-506474751>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIRFMUGIIV7TO5FSW4DP4UGMFANCNFSM4H36BQPA>
> .
>

Removing the milestone as I think this is not 0.25.0 critical?
no this is slightly non trivial 
Should the \_\_eq__ on ExtensionScalarOpsMixin be changed to take care nan, or should ExtensionArray expose a .equals() that does that?

I believe DecimalArray is the only class that inherits ExtensionScalarOpsMixin so to cover all EA's I think a .equals() on EA is more appropriate?

@jreback 
Hi, As this is labeled `good first issue`, I would like to take this on. Is this still needed ?

Commit message: Removing interval.py's __eq__ implementation due to conflict with @jschendel change.

Patch to review:
diff --git a/pandas/core/arrays/base.py b/pandas/core/arrays/base.py
index 96a4eb1b3bf32..6aabd6df24e32 100644
--- a/pandas/core/arrays/base.py
+++ b/pandas/core/arrays/base.py
@@ -78,6 +78,7 @@ class ExtensionArray:
     dropna
     factorize
     fillna
+    equals
     isna
     ravel
     repeat
@@ -104,6 +105,7 @@ class ExtensionArray:
     * _from_sequence
     * _from_factorized
     * __getitem__
+    * __eq__
     * __len__
     * dtype
     * nbytes
@@ -350,6 +352,38 @@ def __iter__(self):
         for i in range(len(self)):
             yield self[i]
 
+    def __eq__(self, other: ABCExtensionArray) -> bool:
+        """
+        Whether the two arrays are equivalent.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            The array to compare to this array.
+
+        Returns
+        -------
+        bool
+        """
+
+        raise AbstractMethodError(self)
+
+    def __ne__(self, other: ABCExtensionArray) -> bool:
+        """
+        Whether the two arrays are not equivalent.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            The array to compare to this array.
+
+        Returns
+        -------
+        bool
+        """
+
+        return ~(self == other)
+
     # ------------------------------------------------------------------------
     # Required attributes
     # ------------------------------------------------------------------------
@@ -657,6 +691,25 @@ def searchsorted(self, value, side="left", sorter=None):
         arr = self.astype(object)
         return arr.searchsorted(value, side=side, sorter=sorter)
 
+    def equals(self, other: ABCExtensionArray) -> bool:
+        """
+        Return if another array is equivalent to this array.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            Array to compare to this Array.
+
+        Returns
+        -------
+        boolean
+            Whether the arrays are equivalent.
+
+        """
+        return isinstance(other, self.__class__) and (
+            ((self == other) | (self.isna() == other.isna())).all()
+        )
+
     def _values_for_factorize(self) -> Tuple[np.ndarray, Any]:
         """
         Return an array and missing value suitable for factorization.
diff --git a/pandas/core/arrays/integer.py b/pandas/core/arrays/integer.py
index 0922f4ac6f71d..4b0d4d65924cd 100644
--- a/pandas/core/arrays/integer.py
+++ b/pandas/core/arrays/integer.py
@@ -376,6 +376,15 @@ def __getitem__(self, item):
 
         return type(self)(self._data[item], self._mask[item])
 
+    def __eq__(self, other):
+        return (
+            isinstance(other, IntegerArray)
+            and hasattr(other, "_data")
+            and self._data == other._data
+            and hasattr(other, "_mask")
+            and self._mask == other._mask
+        )
+
     def _coerce_to_ndarray(self, dtype=None, na_value=lib._no_default):
         """
         coerce to an ndarary of object dtype
diff --git a/pandas/tests/extension/base/methods.py b/pandas/tests/extension/base/methods.py
index 973088cb72e7a..6b2144563a74e 100644
--- a/pandas/tests/extension/base/methods.py
+++ b/pandas/tests/extension/base/methods.py
@@ -358,3 +358,18 @@ def test_repeat_raises(self, data, repeats, kwargs, error, msg, use_numpy):
                 np.repeat(data, repeats, **kwargs)
             else:
                 data.repeat(repeats, **kwargs)
+
+    def test_equals(self, data, na_value):
+        cls = type(data)
+        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)
+        assert ser.equals(ser)
+        assert na_ser.equals(na_ser)
+
+        assert not data.equals(na_value)
+        assert not na_ser.equals(ser)
+        assert not ser.equals(na_ser)
+        assert not ser.equals(0)
+        assert not na_ser.equals(0)
diff --git a/pandas/tests/extension/base/ops.py b/pandas/tests/extension/base/ops.py
index 20d06ef2e5647..0ba5795ea891d 100644
--- a/pandas/tests/extension/base/ops.py
+++ b/pandas/tests/extension/base/ops.py
@@ -132,10 +132,8 @@ class BaseComparisonOpsTests(BaseOpsUtil):
     def _compare_other(self, s, data, op_name, other):
         op = self.get_op_from_name(op_name)
         if op_name == "__eq__":
-            assert getattr(data, op_name)(other) is NotImplemented
             assert not op(s, other).all()
         elif op_name == "__ne__":
-            assert getattr(data, op_name)(other) is NotImplemented
             assert op(s, other).all()
 
         else:
@@ -158,13 +156,3 @@ def test_compare_array(self, data, all_compare_operators):
         s = pd.Series(data)
         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):
-        # EAs should return NotImplemented for ops with Series.
-        # Pandas takes care of unboxing the series and calling the EA's op.
-        other = pd.Series(data)
-        if hasattr(data, "__eq__"):
-            result = data.__eq__(other)
-            assert result is NotImplemented
-        else:
-            raise pytest.skip(f"{type(data).__name__} does not implement __eq__")
diff --git a/pandas/tests/extension/json/array.py b/pandas/tests/extension/json/array.py
index 17bc2773aad19..7c22667e449ff 100644
--- a/pandas/tests/extension/json/array.py
+++ b/pandas/tests/extension/json/array.py
@@ -110,6 +110,13 @@ def __setitem__(self, key, value):
                     assert isinstance(v, self.dtype.type)
                     self.data[k] = v
 
+    def __eq__(self, other):
+        return (
+            isinstance(other, JSONArray)
+            and hasattr(other, "data")
+            and self.data == other.data
+        )
+
     def __len__(self) -> int:
         return len(self.data)
 
diff --git a/pandas/tests/extension/test_sparse.py b/pandas/tests/extension/test_sparse.py
index 6ebe71e173ec2..3fe6335ec63f6 100644
--- a/pandas/tests/extension/test_sparse.py
+++ b/pandas/tests/extension/test_sparse.py
@@ -302,6 +302,10 @@ def test_searchsorted(self, data_for_sorting, as_series):
         with tm.assert_produces_warning(PerformanceWarning):
             super().test_searchsorted(data_for_sorting, as_series)
 
+    def test_equals(self, data, na_value):
+        self._check_unsupported(data)
+        super().test_equals(data, na_value)
+
 
 class TestCasting(BaseSparseTests, base.BaseCastingTests):
     pass


Code review comments:
diff_hunk: @@ -657,6 +691,25 @@ def searchsorted(self, value, side="left", sorter=None):
         arr = self.astype(object)
         return arr.searchsorted(value, side=side, sorter=sorter)
 
+    def equals(self, other: ABCExtensionArray) -> bool:
+        """
+        Return if another array is equivalent to this array.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            Array to compare to this Array.
+
+        Returns
+        -------
+        boolean
+            Whether the arrays are equivalent.
+
+        """
+        return isinstance(other, self.__class__) and (
path: pandas/core/arrays/base.py
text: @user2:
Perhaps `type(self) == type(other)`? It's not clear to me whether a subclass should compare `.equals` to a parent class. Thoughts @user1?


@user2:
Will want some length checking here too.

@author:
Added `type(self) == type(other)` and length checks.

diff_hunk: @@ -376,6 +376,15 @@ def __getitem__(self, item):
 
         return type(self)(self._data[item], self._mask[item])
 
+    def __eq__(self, other):
+        return (
+            isinstance(other, IntegerArray)
+            and hasattr(other, "_data")
path: pandas/core/arrays/integer.py
text: @user1:
We already have `IntegerArray.__eq__` on master. This shouldn't be necessary.

@author:
Removed.

diff_hunk: @@ -350,6 +352,38 @@ def __iter__(self):
         for i in range(len(self)):
             yield self[i]
 
+    def __eq__(self, other: ABCExtensionArray) -> bool:
+        """
+        Whether the two arrays are equivalent.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            The array to compare to this array.
+
+        Returns
+        -------
+        bool
+        """
+
+        raise AbstractMethodError(self)
path: pandas/core/arrays/base.py
text: @user1:
Actually, does this need to be abstract? Or can we define it as something like

```python
# 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?

@author:
Good suggestion. I like that much better.

diff_hunk: @@ -358,3 +358,18 @@ def test_repeat_raises(self, data, repeats, kwargs, error, msg, use_numpy):
                 np.repeat(data, repeats, **kwargs)
             else:
                 data.repeat(repeats, **kwargs)
+
+    def test_equals(self, data, na_value):
+        cls = type(data)
+        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)
path: pandas/tests/extension/base/methods.py
text: @user1:
make all these `assert <condition> is True`, to ensure that `True` or `False` is actually returned

@author:
I ran into a funny issue doing this: https://github.com/numpy/numpy/issues/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.

diff_hunk: @@ -158,13 +156,3 @@ def test_compare_array(self, data, all_compare_operators):
         s = pd.Series(data)
         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):
path: pandas/tests/extension/base/ops.py
text: @user1:
Undo this change.

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

@user2:
> 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.

diff_hunk: @@ -350,6 +352,38 @@ def __iter__(self):
         for i in range(len(self)):
             yield self[i]
 
+    def __eq__(self, other: ABCExtensionArray) -> bool:
+        """
+        Whether the two arrays are equivalent.
+
+        Parameters
+        ----------
+        other: ExtensionArray
+            The array to compare to this array.
+
+        Returns
+        -------
+        bool
+        """
+
+        raise AbstractMethodError(self)
+
+    def __ne__(self, other: ABCExtensionArray) -> bool:
path: pandas/core/arrays/base.py
text: @user1:
Same comment about the type on `other`.

@author:
Done.

diff_hunk: @@ -314,6 +314,20 @@ def __getitem__(self, item):
             return self._data[item]
         return type(self)(self._data[item], self._mask[item])
 
+    def __eq__(self, other):
+        if not isinstance(other, BooleanArray):
+            return NotImplemented
+        return (
+            hasattr(other, "_data")
path: pandas/core/arrays/boolean.py
text: @author:
Not needed. Missed that in my WIP PR.

diff_hunk: @@ -350,6 +352,38 @@ def __iter__(self):
         for i in range(len(self)):
             yield self[i]
 
+    def __eq__(self, other: ABCExtensionArray) -> bool:
path: pandas/core/arrays/base.py
text: @user1:
The type of `other` should be Any I think. Passing a list / etc here is valid, it just returns False.

@user1:
Make sure to update the type in the docstring too.

@author:
Done.