Repository: pandas-dev/pandas
Pull request: Bugfix timedelta notimplemented eq
- [x] closes #20829 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
Timedelta always returning False for equality tests for incompatible types
#### Code Sample, a copy-pastable example if possible

```python
import datetime
import pandas

class CustomClass:

    def __init__(self):
        pass
       
    def __eq__(self, other):
        raise Exception("Custom Class eq")

        
custom = CustomClass()
var1 = datetime.timedelta(seconds=1)
var2 = pandas.Timedelta("1s")    

# Following code raises CustomClass exception
var1 == custom

# Following code returns False, does not call CustomClass.__eq__
var2 == custom
```
#### Problem description

I am trying to implement a version of `pytest.approx` for Timedeltas, which basically returns a class with a custom `__eq__` implementation.

`Timedelta("5s") == AproxTimedelta(...)

However it does not work with pandas Timedelta because it always return False, so the `__eq__` of AproxTimedelta object is never called.

This is not the behavior for python datetime timedelta, as can be seen in the example script.

#### Expected Output

`Exception("Custom Class eq")` raised for both cases

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit: None
python: 3.6.3.final.0
python-bits: 64
OS: Windows
OS-release: 10
machine: AMD64
processor: Intel64 Family 6 Model 158 Stepping 9, GenuineIntel
byteorder: little
LC_ALL: None
LANG: None
LOCALE: None.None
pandas: 0.22.0
pytest: 3.5.0
pip: 10.0.0
setuptools: 38.5.1
Cython: 0.28.2
numpy: 1.14.1
scipy: 1.0.0
pyarrow: None
xarray: None
IPython: 6.3.1
sphinx: 1.7.1
patsy: None
dateutil: 2.6.1
pytz: 2017.2
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: 2.1.1
openpyxl: 1.7.0
xlrd: 1.1.0
xlwt: None
xlsxwriter: None
lxml: None
bs4: None
html5lib: None
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: 2.10
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
</details>

Hints from issue discussion:
Can you try on master?

```python
custom = CustomClass()
var1 = datetime.timedelta(seconds=1)
var2 = pandas.Timedelta("1s")

# Following code raises CustomClass exception
var1 == custom

# Following code returns False, does not call CustomClass.__eq__
var2 == custom

## -- End pasted text --
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
<ipython-input-1-82804cdda1a5> in <module>()
     16
     17 # Following code raises CustomClass exception
---> 18 var1 == custom
     19
     20 # Following code returns False, does not call CustomClass.__eq__

<ipython-input-1-82804cdda1a5> in __eq__(self, other)
      8
      9     def __eq__(self, other):
---> 10         raise Exception("Custom Class eq")
     11
     12

Exception: Custom Class eq

```
Same thing on master.

Note that in the example we want exceptions both in `var1 == custom` and `var2 == custom`.
Ah sorry I didn't get to that because of the first exception.

cc @jbrockmendel whose been active here recently. I think this is may be non-trivial to support.
Tom is right, this would be non-trivial to support.  There is no branch of `Timedelta.__eq__` (actually `Timedelta.__richcmp__`) that returns `NotImplemented`.

That said, if you were careful to always write `custom == td` instead of `td == custom` then it `custom.__eq__` would get called first instead of `td.__eq__`.
@jbrockmendel I was taking a look at the code, shouldn't it be just a matter of changing this branch:

https://github.com/pandas-dev/pandas/blob/c85ab083919b59ce84c220d5baf7d34ff4a0bcf2/pandas/_libs/tslibs/timedeltas.pyx#L695-L702

To return `NotImplemented` ?

```
 else: 
     return NotImplemented
```
It might be reasonable to return `NotImplemented` instead of raising `TypeError` (though tracking down the affected tests and changing the expected error message would be a hassle), but the `Py_EQ` and `Py_NE` branches we wouldn't want to change because we need to accommodate _non_-custom classes.

Also if `ApproxTimedelta` happens to subclass `timedelta`, then the comparison wouldn't go through the linked code, but would instead go through lines 674 and 704.

Have you considered using `mock` to patch `Timedelta.__eq__`?  That might be easier on your end.
> but the Py_EQ and Py_NE branches we wouldn't want to change because we need to accommodate non-custom classes.

Could you elaborate a bit more on than? What would be an example?

> Have you considered using mock to patch Timedelta.__eq__? That might be easier on your end.

Yep that's a good idea for time being!

Another question related to it: If I also happen to have a custom Timedelta subclass (for instance allowing to sum a timedelta with "1s" directly). Is there an easy way to make pandas use this custom when working with TimedeltaIndexes?
> Could you elaborate a bit more on than? What would be an example?

Timedelta(whatever) == 6 --> False
Timedelta(whatever) != "foo" --> True

> Is there an easy way to make pandas use this custom when working with TimedeltaIndexes?

Try `pd.Index([custom_timedelta], dtype='o')`
I thought python would fallback automatically to identity comparison if both classes returned NotImplemented for a comparison.

Is it a different situation for pandas Timedelta?
@Sup3rGeo Off the top of my head I can't think of any reason why Timedelta would behave differently, but the check-for-eq-then-ne-then-raise pattern is present in a handful of places in pandas.

Maybe try making the substitution you have in mind and see if it causes any test failures?

Commit message: Fixed version_info call in test.

Patch to review:
diff --git a/doc/source/whatsnew/v0.23.1.txt b/doc/source/whatsnew/v0.23.1.txt
index 5a1bcce9b5970..2193fd5769854 100644
--- a/doc/source/whatsnew/v0.23.1.txt
+++ b/doc/source/whatsnew/v0.23.1.txt
@@ -59,6 +59,7 @@ Data-type specific
 
 - Bug in :meth:`Series.str.replace()` where the method throws `TypeError` on Python 3.5.2 (:issue: `21078`)
 - Bug in :class:`Timedelta`: where passing a float with a unit would prematurely round the float precision (:issue: `14156`)
+- Bug in :class:`Timedelta`: Comparison with unknown types now return NotImplemented as typically expected (:issue: `20829`)
 - Bug in :func:`pandas.testing.assert_index_equal` which raised ``AssertionError`` incorrectly, when comparing two :class:`CategoricalIndex` objects with param ``check_categorical=False`` (:issue:`19776`)
 
 Sparse
diff --git a/pandas/_libs/tslibs/timedeltas.pyx b/pandas/_libs/tslibs/timedeltas.pyx
index 87dc371195b5b..8a5ccd9aeec06 100644
--- a/pandas/_libs/tslibs/timedeltas.pyx
+++ b/pandas/_libs/tslibs/timedeltas.pyx
@@ -680,26 +680,12 @@ cdef class _Timedelta(timedelta):
                     if is_timedelta64_object(other):
                         other = Timedelta(other)
                     else:
-                        if op == Py_EQ:
-                            return False
-                        elif op == Py_NE:
-                            return True
-
-                        # only allow ==, != ops
-                        raise TypeError('Cannot compare type {!r} with type ' \
-                                        '{!r}'.format(type(self).__name__,
-                                                      type(other).__name__))
+                        return NotImplemented
                 if util.is_array(other):
                     return PyObject_RichCompare(np.array([self]), other, op)
                 return PyObject_RichCompare(other, self, reverse_ops[op])
             else:
-                if op == Py_EQ:
-                    return False
-                elif op == Py_NE:
-                    return True
-                raise TypeError('Cannot compare type {!r} with type ' \
-                                '{!r}'.format(type(self).__name__,
-                                              type(other).__name__))
+                return NotImplemented
 
         return cmp_scalar(self.value, ots.value, op)
 
diff --git a/pandas/tests/scalar/timedelta/test_timedelta.py b/pandas/tests/scalar/timedelta/test_timedelta.py
index 205fdf49d3e91..374783b1c64e1 100644
--- a/pandas/tests/scalar/timedelta/test_timedelta.py
+++ b/pandas/tests/scalar/timedelta/test_timedelta.py
@@ -1,4 +1,5 @@
 """ test the scalar Timedelta """
+import sys
 import pytest
 
 import numpy as np
@@ -42,8 +43,10 @@ def test_ops_error_str(self):
             with pytest.raises(TypeError):
                 left + right
 
-            with pytest.raises(TypeError):
-                left > right
+            # GH 20829: python 2 comparison naturally does not raise TypeError
+            if sys.version_info >= (3, 0):
+                with pytest.raises(TypeError):
+                    left > right
 
             assert not left == right
             assert left != right
@@ -103,6 +106,54 @@ def test_compare_timedelta_ndarray(self):
         expected = np.array([False, False])
         tm.assert_numpy_array_equal(result, expected)
 
+    def test_custom_comparison_object(self):
+        # GH20829
+        class CustomClass(object):
+
+            def __init__(self, cmp_result=None):
+                self.cmp_result = cmp_result
+
+            def generic_result(self):
+                if self.cmp_result is None:
+                    return NotImplemented
+                else:
+                    return self.cmp_result
+
+            def __eq__(self, other):
+                return self.generic_result()
+
+            def __gt__(self, other):
+                return self.generic_result()
+
+        t = Timedelta('1s')
+
+        assert not (t == "string")
+        assert not (t == 1)
+        assert not (t == CustomClass())
+        assert not (t == CustomClass(cmp_result=False))
+
+        assert t < CustomClass(cmp_result=True)
+        assert not (t < CustomClass(cmp_result=False))
+
+        assert t == CustomClass(cmp_result=True)
+
+    @pytest.mark.parametrize("val", [
+        "string", 1])
+    def test_raise_comparisons_unknown_types(self, val):
+        # GH20829
+        t = Timedelta('1s')
+        if sys.version_info >= (3, 0):
+            # python 2 does not raises TypeError for comparisons
+            # of different types
+            with pytest.raises(TypeError):
+                t >= val
+            with pytest.raises(TypeError):
+                t > val
+            with pytest.raises(TypeError):
+                t <= val
+            with pytest.raises(TypeError):
+                t < val
+
 
 class TestTimedeltas(object):
 


Code review comments:
diff_hunk: @@ -42,8 +43,10 @@ def test_ops_error_str(self):
             with pytest.raises(TypeError):
                 left + right
 
-            with pytest.raises(TypeError):
-                left > right
+            # GH 20829: python 2 comparison naturally does not raise TypeError
path: pandas/tests/scalar/timedelta/test_timedelta.py
text: @user1:
use compat.PY3

@author:
Done, but as your comment was in the code comment (which didn't change), it remained here after my changes.

diff_hunk: @@ -59,6 +59,7 @@ Data-type specific
 
 - Bug in :meth:`Series.str.replace()` where the method throws `TypeError` on Python 3.5.2 (:issue: `21078`)
 - Bug in :class:`Timedelta`: where passing a float with a unit would prematurely round the float precision (:issue: `14156`)
+- Bug in :class:`Timedelta`: Comparison with unknown types now return NotImplemented as typically expected (:issue: `20829`)
path: doc/source/whatsnew/v0.23.1.txt
text: @user1:
move to 0.24

diff_hunk: @@ -103,6 +106,54 @@ def test_compare_timedelta_ndarray(self):
         expected = np.array([False, False])
         tm.assert_numpy_array_equal(result, expected)
 
+    def test_custom_comparison_object(self):
+        # GH20829
+        class CustomClass(object):
+
+            def __init__(self, cmp_result=None):
+                self.cmp_result = cmp_result
+
+            def generic_result(self):
+                if self.cmp_result is None:
+                    return NotImplemented
+                else:
+                    return self.cmp_result
+
+            def __eq__(self, other):
+                return self.generic_result()
+
+            def __gt__(self, other):
+                return self.generic_result()
+
+        t = Timedelta('1s')
+
+        assert not (t == "string")
+        assert not (t == 1)
+        assert not (t == CustomClass())
+        assert not (t == CustomClass(cmp_result=False))
+
+        assert t < CustomClass(cmp_result=True)
+        assert not (t < CustomClass(cmp_result=False))
+
+        assert t == CustomClass(cmp_result=True)
+
+    @pytest.mark.parametrize("val", [
path: pandas/tests/scalar/timedelta/test_timedelta.py
text: @user1:
use a skip with the compat.PY3 here