Bugfix timedelta notimplemented eq
State: closed

- [x] closes #20829 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: Timedelta
description: Timedelta data type

Comments:
@jbrockmendel following up from the issue discussion, the tests look good it seems?

Except for python 2 that actually has different ordering behavior than python 3, this is why it it fails on python2:
https://docs.python.org/3.0/whatsnew/3.0.html#ordering-comparisons
http://portingguide.readthedocs.io/en/latest/comparisons.html

What do you think? Could we just then add a special case checking for python 2 that raises TypeError?
Seems reasonable.  What other tests are affected?  This may be a good opportunity to flesh them out.
Hello @Sup3rGeo! Thanks for updating the PR.

 - In the file [`pandas/tests/scalar/timedelta/test_timedelta.py`](https://github.com/Sup3rGeo/pandas/blob/301615eb7c5e9a10e624163fc8af404a7a69278c/pandas/tests/scalar/timedelta/test_timedelta.py), following                are the PEP8 issues :
> [Line 109:80](https://github.com/Sup3rGeo/pandas/blob/301615eb7c5e9a10e624163fc8af404a7a69278c/pandas/tests/scalar/timedelta/test_timedelta.py#L109): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (81 > 79 characters)




##### Comment last updated on October 23, 2018 at 19:08 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=h1) Report
> Merging [#21394](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/5a0c91e5a5bc61c71da08d6968d973255829678d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21394/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #21394   +/-   ##
=======================================
  Coverage   92.22%   92.22%           
=======================================
  Files         169      169           
  Lines       50897    50897           
=======================================
  Hits        46940    46940           
  Misses       3957     3957
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.65% <ø> (ø)` | :arrow_up: |
| #single | `42.29% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=footer). Last update [5a0c91e...301615e](https://codecov.io/gh/pandas-dev/pandas/pull/21394?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I gave it a second thought and I think we should not have a special case for python 2 because if we raise manually from the `__richcmp__` function than we still won't allow the other object to try to implement the comparison.

So this last commit is only testing for `TypeError`s for python 3, letting python 2 comparisons (lt gt le ge) act as it is the default (not raising anything). 
Take a look at the Travis log; the test is raising before getting to the behavior you care about.

In nearly all cases we want the behavior to be identical in py2 and py3.  Is there a compelling reason to break that rule in this case?
Basically the comparisons are implemented differently in python 2 and python 3, so with the changes in this PR we are just following python itself. Which I believe is good for the following reasons:

- `pandas.Timedelta` won't try to change the default python behavior on comparisons, so no surprises.
- We don't break the ability of the other member of the comparison to implement it in a meaningful way (very important)

The way `pandas.Timedelta` is currently implemented in the released versions breaks item 2 because it does not follow item 1.
> won't try to change the default python behavior on comparisons, so no surprises.

I'd argue that the default python2 behavior is itself surprising.  Having `pd.Timedelta` behave differently in py2 vs py3 seems very undesirable.  That said, it's not me you need to convince, it's @jreback.

I agree with you about python 2 having this surprising behavior, and that it would be desirable to have them work the same. Its just that given that our current implementation also brings undesired side-effects, if we have to choose between two evils, I would say it is better to just expose these intrinsic python2-3 differences to the user.

IMHO it breaks less "contracts" (and then surely this python2-3 differences aren't to blame pandas, but something users naturally have to deal with when working with different backward-incompatible python versions, just like the different strings behavior and so on).

Well we might not 100% agree on this but thanks for the support so far @jbrockmendel. Let's see what @jreback thinks.
IIRC there is a bug in the cpython impl for comparisons with timedelta in py2, maybe @cpcloud remembers where it actually is.
so I think something broke as the circle tests are failing. needs to very carefully look at all failures as this is a far reaching change.
I will further investigate the errors.
I might need some help to debug the failing test on CircleCI. Any idea how can I reproduce it locally on my machine?
can you rebase / update
I tried installing ubuntu in a virtual machine and running the same scripts the the failing CircleCI build, and the failing test is still passing.
this looks fine. can you rebase. ping on green.
@jreback sorry what do you mean by "ping on green"?
@Sup3rGeo when all the CI checks have green checkmarks (as opposed to red "x"s) write a comment "ping" to get his attention
@jreback, @jbrockmendel I think I am in a dead end because I cannot manage to reproduce this failure locally. I would need some help/pointers here.
merge master and push again
Oh, seems like it's green now.
@jbrockmendel if you'd have a look. merge when satisified.
LGTM.  Thanks @Sup3rGeo