Bugfix timedelta notimplemented eq #21394

Sup3rGeo · 2018-06-08T20:21:47Z

closes Timedelta always returning False for equality tests for incompatible types #20829
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Sup3rGeo · 2018-06-09T14:49:56Z

@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?

jbrockmendel · 2018-06-09T16:03:11Z

Seems reasonable. What other tests are affected? This may be a good opportunity to flesh them out.

…unt that python 2 does not raise TypeError for uncompatible types.

pep8speaks · 2018-06-09T19:13:40Z

Hello @Sup3rGeo! Thanks for updating the PR.

In the file pandas/tests/scalar/timedelta/test_timedelta.py, following are the PEP8 issues :

Line 109:80: E501 line too long (81 > 79 characters)

Comment last updated on October 23, 2018 at 19:08 Hours UTC

codecov · 2018-06-09T19:14:08Z

Codecov Report

Merging #21394 into master will not change coverage.
The diff coverage is n/a.

@@           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% <ø> (ø)`	⬆️
#single	`42.29% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a0c91e...301615e. Read the comment docs.

Sup3rGeo · 2018-06-09T19:16:03Z

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 TypeErrors for python 3, letting python 2 comparisons (lt gt le ge) act as it is the default (not raising anything).

jbrockmendel · 2018-06-09T22:57:08Z

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?

Sup3rGeo · 2018-06-09T23:15:58Z

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.

jbrockmendel · 2018-06-10T01:33:26Z

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.

Sup3rGeo · 2018-06-10T12:46:33Z

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.

jreback · 2018-06-13T11:06:31Z

IIRC there is a bug in the cpython impl for comparisons with timedelta in py2, maybe @cpcloud remembers where it actually is.

jreback · 2018-06-13T11:01:48Z

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`)


move to 0.24

pandas/tests/scalar/timedelta/test_timedelta.py

jreback · 2018-06-13T11:03:21Z

pandas/tests/scalar/timedelta/test_timedelta.py

@@ -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):


test_compare_custom_object

jreback · 2018-06-13T11:03:35Z

pandas/tests/scalar/timedelta/test_timedelta.py

+
+    @pytest.mark.parametrize("val", [
+        "string", 1])
+    def test_raise_comparisons_unknown_types(self, val):


test_compare_unknown_type

jreback · 2018-06-13T11:07:11Z

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.

…gfix-timedelta-notimplemented-eq

Sup3rGeo · 2018-07-01T12:56:03Z

I will further investigate the errors.

Sup3rGeo · 2018-07-01T15:38:17Z

I might need some help to debug the failing test on CircleCI. Any idea how can I reproduce it locally on my machine?

jreback · 2018-09-25T16:30:46Z

can you rebase / update

…notimplemented-eq # Conflicts: # doc/source/whatsnew/v0.24.0.txt # pandas/_libs/tslibs/timedeltas.pyx

doc/source/whatsnew/v0.24.0.txt

pandas/tests/scalar/timedelta/test_timedelta.py

Sup3rGeo · 2018-10-08T11:50:42Z

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.

jreback · 2018-10-10T12:26:35Z

this looks fine. can you rebase. ping on green.

…gfix-timedelta-notimplemented-eq

Sup3rGeo · 2018-10-11T22:17:55Z

@jreback sorry what do you mean by "ping on green"?

jbrockmendel · 2018-10-11T22:21:57Z

@Sup3rGeo when all the CI checks have green checkmarks (as opposed to red "x"s) write a comment "ping" to get his attention

Sup3rGeo · 2018-10-23T08:49:15Z

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

jreback · 2018-10-23T09:19:11Z

merge master and push again

…notimplemented-eq

Sup3rGeo · 2018-10-24T07:14:40Z

Oh, seems like it's green now.

jreback · 2018-10-24T12:27:25Z

@jbrockmendel if you'd have a look. merge when satisified.

jbrockmendel · 2018-10-24T16:25:32Z

LGTM. Thanks @Sup3rGeo

jorisvandenbossche · 2018-10-26T08:34:41Z

doc/source/whatsnew/v0.24.0.txt

@@ -193,6 +193,7 @@ Other Enhancements
 - :func:`~DataFrame.to_csv`, :func:`~Series.to_csv`, :func:`~DataFrame.to_json`, and :func:`~Series.to_json` now support ``compression='infer'`` to infer compression based on filename extension (:issue:`15008`).
  The default compression for ``to_csv``, ``to_json``, and ``to_pickle`` methods has been updated to ``'infer'`` (:issue:`22004`).
 - :func:`to_timedelta` now supports iso-formated timedelta strings (:issue:`21877`)
+- Comparing :class:`Timedelta` with unknown types now return ``NotImplemented`` instead of ``False`` (:issue:`20829`)


NotImplemented -> raises a TypeError (that is what the user sees)
Fixed in #23355

* Return NotImplemented for unknown types in comparison. * Added tests and updated whatsnew. * Removed excess space LINT * TST/BUG Always return NotImplemented. Updated tests to take into account that python 2 does not raise TypeError for uncompatible types. * Line too long. * Fixed version_info call in test. * Moved changes to v0.24.0 * Skipped test based on PY2 * Updated test names. * Removed unused import * Fixed whatsnew placement and commented test.

* Revert #21394 * Typo fixup

* Revert pandas-dev#21394 * Typo fixup

* Return NotImplemented for unknown types in comparison. * Added tests and updated whatsnew. * Removed excess space LINT * TST/BUG Always return NotImplemented. Updated tests to take into account that python 2 does not raise TypeError for uncompatible types. * Line too long. * Fixed version_info call in test. * Moved changes to v0.24.0 * Skipped test based on PY2 * Updated test names. * Removed unused import * Fixed whatsnew placement and commented test.

* Revert pandas-dev#21394 * Typo fixup

Victor added 3 commits June 8, 2018 17:09

Return NotImplemented for unknown types in comparison.

5d78297

Added tests and updated whatsnew.

e656738

Removed excess space LINT

049dbe9

gfyoung added Bug Timedelta Timedelta data type labels Jun 8, 2018

TST/BUG Always return NotImplemented. Updated tests to take into acco…

1e64ece

…unt that python 2 does not raise TypeError for uncompatible types.

Line too long.

adec659

Fixed version_info call in test.

dd185e6

jreback requested changes Jun 13, 2018

View reviewed changes

victor added 4 commits July 1, 2018 13:49

Merge branch 'master' of https://github.com/pandas-dev/pandas into bu…

a6ccefd

…gfix-timedelta-notimplemented-eq

Moved changes to v0.24.0

782568f

Skipped test based on PY2

ba307b4

Updated test names.

81648ed

Removed unused import

aa766f0

Merge remote-tracking branch 'upstream/master' into bugfix-timedelta-…

d86c7ce

…notimplemented-eq # Conflicts: # doc/source/whatsnew/v0.24.0.txt # pandas/_libs/tslibs/timedeltas.pyx

jreback requested changes Oct 7, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.txt Outdated Show resolved Hide resolved

pandas/tests/scalar/timedelta/test_timedelta.py Outdated Show resolved Hide resolved

Fixed whatsnew placement and commented test.

16c8222

jreback added this to the 0.24.0 milestone Oct 10, 2018

Merge branch 'master' of https://github.com/pandas-dev/pandas into bu…

ab6384f

…gfix-timedelta-notimplemented-eq

Merge remote-tracking branch 'upstream/master' into bugfix-timedelta-…

301615e

…notimplemented-eq

jreback approved these changes Oct 24, 2018

View reviewed changes

jbrockmendel merged commit 5dd4cae into pandas-dev:master Oct 24, 2018

Sup3rGeo deleted the bugfix-timedelta-notimplemented-eq branch October 24, 2018 16:43

jorisvandenbossche mentioned this pull request Oct 26, 2018

DOC: fixup whatsnew note for GH21394 #23355

Merged

jorisvandenbossche reviewed Oct 26, 2018

View reviewed changes

jbrockmendel mentioned this pull request Dec 4, 2018

RLS: 0.24.0 #24060

Closed

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Dec 15, 2018

Revert pandas-dev#21394

cf9a155

jreback pushed a commit that referenced this pull request Dec 15, 2018

Revert #21394 (#24289)

e3b6683

* Revert #21394 * Typo fixup

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Revert pandas-dev#21394 (pandas-dev#24289)

fdacb39

* Revert pandas-dev#21394 * Typo fixup

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Revert pandas-dev#21394 (pandas-dev#24289)

bf67998

* Revert pandas-dev#21394 * Typo fixup

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix timedelta notimplemented eq #21394

Bugfix timedelta notimplemented eq #21394

Sup3rGeo commented Jun 8, 2018 •

edited

Loading

Sup3rGeo commented Jun 9, 2018 •

edited

Loading

jbrockmendel commented Jun 9, 2018

pep8speaks commented Jun 9, 2018 •

edited

Loading

codecov bot commented Jun 9, 2018 •

edited

Loading

Sup3rGeo commented Jun 9, 2018 •

edited

Loading

jbrockmendel commented Jun 9, 2018

Sup3rGeo commented Jun 9, 2018

jbrockmendel commented Jun 10, 2018

Sup3rGeo commented Jun 10, 2018 •

edited

Loading

jreback commented Jun 13, 2018

jreback Jun 13, 2018

jreback Jun 13, 2018

jreback Jun 13, 2018

jreback commented Jun 13, 2018

Sup3rGeo commented Jul 1, 2018

Sup3rGeo commented Jul 1, 2018

jreback commented Sep 25, 2018

Sup3rGeo commented Oct 8, 2018

jreback commented Oct 10, 2018

Sup3rGeo commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

Sup3rGeo commented Oct 23, 2018

jreback commented Oct 23, 2018

Sup3rGeo commented Oct 24, 2018

jreback commented Oct 24, 2018

jbrockmendel commented Oct 24, 2018

jorisvandenbossche Oct 26, 2018

Bugfix timedelta notimplemented eq #21394

Bugfix timedelta notimplemented eq #21394

Conversation

Sup3rGeo commented Jun 8, 2018 • edited Loading

Sup3rGeo commented Jun 9, 2018 • edited Loading

jbrockmendel commented Jun 9, 2018

pep8speaks commented Jun 9, 2018 • edited Loading

Comment last updated on October 23, 2018 at 19:08 Hours UTC

codecov bot commented Jun 9, 2018 • edited Loading

Codecov Report

Sup3rGeo commented Jun 9, 2018 • edited Loading

jbrockmendel commented Jun 9, 2018

Sup3rGeo commented Jun 9, 2018

jbrockmendel commented Jun 10, 2018

Sup3rGeo commented Jun 10, 2018 • edited Loading

jreback commented Jun 13, 2018

jreback Jun 13, 2018

Choose a reason for hiding this comment

jreback Jun 13, 2018

Choose a reason for hiding this comment

jreback Jun 13, 2018

Choose a reason for hiding this comment

jreback commented Jun 13, 2018

Sup3rGeo commented Jul 1, 2018

Sup3rGeo commented Jul 1, 2018

jreback commented Sep 25, 2018

Sup3rGeo commented Oct 8, 2018

jreback commented Oct 10, 2018

Sup3rGeo commented Oct 11, 2018

jbrockmendel commented Oct 11, 2018

Sup3rGeo commented Oct 23, 2018

jreback commented Oct 23, 2018

Sup3rGeo commented Oct 24, 2018

jreback commented Oct 24, 2018

jbrockmendel commented Oct 24, 2018

jorisvandenbossche Oct 26, 2018

Choose a reason for hiding this comment

Sup3rGeo commented Jun 8, 2018 •

edited

Loading

Sup3rGeo commented Jun 9, 2018 •

edited

Loading

pep8speaks commented Jun 9, 2018 •

edited

Loading

codecov bot commented Jun 9, 2018 •

edited

Loading

Sup3rGeo commented Jun 9, 2018 •

edited

Loading

Sup3rGeo commented Jun 10, 2018 •

edited

Loading