Tests for TDI issues already fixed #19044

jbrockmendel · 2018-01-03T01:04:07Z

Fix a couple others caused by failing to return NotImplemented or to override names correctly.

closes #17250
xref #19042
closes #19043

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

fix a couple others caused by failing to return NotImplemented or to override names correctly

gfyoung · 2018-01-03T01:26:03Z

doc/source/whatsnew/v0.23.0.txt

@@ -300,7 +300,9 @@ Conversion
 - Bug in :class:`FY5253` where ``datetime`` addition and subtraction incremented incorrectly for dates on the year-end but not normalized to midnight (:issue:`18854`)
 - Bug in :class:`DatetimeIndex` where adding or subtracting an array-like of ``DateOffset`` objects either raised (``np.array``, ``pd.Index``) or broadcast incorrectly (``pd.Series``) (:issue:`18849`)
 - Bug in :class:`Series` floor-division where operating on a scalar ``timedelta`` raises an exception (:issue:`18846`)
-
+- Bug in :class:`Series`` with ``dtype='timedelta64[ns]`` where addition or subtraction of ``TimedeltaIndex`` had results cast to ``dtype='int64'`` (:issue:`17250`)
+- Bug in :class:`TimedeltaIndex` where multiplication or division by a ``Series`` would return a ``TimedeltaIndex`` instead of a ``Series`` (issue:`18963`)


I think you meant #19042?

Yes, thanks.

gfyoung · 2018-01-03T01:26:56Z

pandas/core/indexes/base.py

+                if (isinstance(other, ABCSeries) and
+                        self.dtype == "timedelta64[ns]"):
+                    # GH#19042
+                    return NotImplemented


Explain why you're doing this in a comment.

I'll expand the comment. This actually needs to be done for all Index classes because the problem is wide-spread, but I really want to avoid scope creep.

Fair enough. I agree that we should try to keep the scope well-defined, though I might consider adding tests that you can xfail for now and patch in a follow-up (if you can add / make tests generic enough to apply to all Index classes).

gfyoung · 2018-01-03T01:27:33Z

@jbrockmendel : Triple threat PR, very nice! 👍

codecov · 2018-01-03T02:26:21Z

Codecov Report

❗ No coverage uploaded for pull request base (master@856c92b). Click here to learn what that means.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##             master   #19044   +/-   ##
=========================================
  Coverage          ?   91.53%           
=========================================
  Files             ?      148           
  Lines             ?    48691           
  Branches          ?        0           
=========================================
  Hits              ?    44569           
  Misses            ?     4122           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.9% <83.33%> (?)`
#single	`41.62% <66.66%> (?)`

Impacted Files	Coverage Δ
pandas/core/ops.py	`91.89% <100%> (ø)`
pandas/core/indexes/timedeltas.py	`90.92% <50%> (ø)`

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 856c92b...10296e8. Read the comment docs.

…ectly

pep8speaks · 2018-01-03T03:13:23Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 05, 2018 at 16:45 Hours UTC

jreback · 2018-01-03T11:21:33Z

pandas/core/indexes/base.py

@@ -4020,6 +4020,11 @@ def _add_numeric_methods_binary(cls):

        def _make_evaluate_binop(op, opstr, reversed=False, constructor=Index):
            def _evaluate_numeric_binop(self, other):
+                if (isinstance(other, ABCSeries) and


I don't like this at all. either let's remove this for now (and the corresponding test) or simplemente this by an override in the appopriate subclass.

Removing this for now.

Overriding in subclass is a PITA b/c TimedeltaIndex._add_numeric_methods() is called below the definition of TimedeltaIndex and will override any __mul__ or __div__ methods we put in the class itself. So to override we need to implement it outside the class and pin it on after the call to add_numeric_methods. Since this is among my least favorite patterns, I'm punting on it.

jreback · 2018-01-03T11:24:02Z

pandas/core/ops.py

@@ -775,6 +775,12 @@ def wrapper(left, right, name=name, na_op=na_op):
            res_name = left.name

        result = wrap_results(safe_na_op(lvalues, rvalues))
+        try:


I would do this in _construct_result and simply pass in name = res_name or getattr(result, 'name', None)

jreback · 2018-01-03T11:24:45Z

pandas/tests/indexes/test_numeric.py

@@ -114,6 +114,22 @@ def test_numeric_compat(self):
        result = idx**2.0
        tm.assert_index_equal(result, expected)

+    @pytest.mark.xfail(reason="Index._add_numeric_methods_binary does not "


is there an issue number ? I don't actually like adding a lot of xfails, if its only 1 ok

Yah, 19042. I'm removing this test.

jreback · 2018-01-04T00:17:22Z

pandas/core/indexes/timedeltas.py

@@ -912,6 +915,22 @@ def delete(self, loc):
 TimedeltaIndex._add_datetimelike_methods()


+def _override_tdi_arith_methods():
+    # GH#19042


I would rather just not do this now. remove.

jreback · 2018-01-04T00:17:56Z

pandas/core/ops.py

-    return left._constructor(result, index=index, name=name, dtype=dtype)
+    out = left._constructor(result, index=index, name=name, dtype=dtype)
+
+    # If `result` has a non-None name and name is None, we need to override.


can you make this a doc-string instead

…x17250

jreback · 2018-01-04T15:23:31Z

doc/source/whatsnew/v0.23.0.txt

@@ -308,7 +308,10 @@ Conversion
 - Bug in :class:`DatetimeIndex` where adding or subtracting an array-like of ``DateOffset`` objects either raised (``np.array``, ``pd.Index``) or broadcast incorrectly (``pd.Series``) (:issue:`18849`)
 - Bug in :class:`Series` floor-division where operating on a scalar ``timedelta`` raises an exception (:issue:`18846`)
 - Bug in :class:`Index` constructor with ``dtype=CategoricalDtype(...)`` where ``categories`` and ``ordered`` are not maintained (issue:`19032`) 
-
+- Bug in :class:`Series`` with ``dtype='timedelta64[ns]`` where addition or subtraction of ``TimedeltaIndex`` had results cast to ``dtype='int64'`` (:issue:`17250`)
+- Bug in :class:`TimedeltaIndex` where division by a ``Series`` would return a ``TimedeltaIndex`` instead of a ``Series`` (issue:`19042`)


you crossed this out is that correct?

The original PR fixed both TimedeltaIndex.__mul__ and TimedeltaIndex.__div__. Then we narrowed the scope so it only fixed __div__.

ok, so is that issue closed by this PR? (you cross it out i the header of the PR)

Correct, #19042 is not fully closed by this PR.

ok, let's close #19042 with this, and pls create a new issue for the residual. (and ref this PR)

jreback · 2018-01-04T15:24:19Z

pandas/core/ops.py

+    the name argument is None, then passing name to the constructor will
+    not be enough; we still need to override the name attribute.
+    """
+    out = left._constructor(result, index=index, name=name, dtype=dtype)


let's just not pass the name at all to the constructor then (and just set as you are doing)

Sounds good.

…x17250

jreback · 2018-01-05T14:17:37Z

rebase and lgtm.

…x17250

jbrockmendel · 2018-01-05T18:57:18Z

ping

jreback · 2018-01-05T19:16:29Z

thanks @jbrockmendel

Tests for TDI issues already fixed

1cd1399

fix a couple others caused by failing to return NotImplemented or to override names correctly

gfyoung reviewed Jan 3, 2018

View reviewed changes

gfyoung added Bug Timedelta Timedelta data type Datetime Datetime data dtype labels Jan 3, 2018

implement xfail test for index classes that dont defer to Series corr…

4d2333f

…ectly

flake8 fixup

3b02815

jreback requested changes Jan 3, 2018

View reviewed changes

Suggested edits

0cd06e4

jreback requested changes Jan 4, 2018

View reviewed changes

jbrockmendel added 3 commits January 3, 2018 16:52

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

24062d0

…x17250

requested edits, un-fix __mul__

d6a5752

weaken tests for un-fixed methods

bf09444

jreback requested changes Jan 4, 2018

View reviewed changes

jbrockmendel added 2 commits January 4, 2018 09:19

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

57f794b

…x17250

requested edits

6f7e5b2

jbrockmendel mentioned this pull request Jan 4, 2018

Index * Series returns Index #19080

Closed

jreback added this to the 0.23.0 milestone Jan 5, 2018

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

10296e8

…x17250

jreback approved these changes Jan 5, 2018

View reviewed changes

jreback merged commit f2d8db1 into pandas-dev:master Jan 5, 2018

jbrockmendel deleted the fix17250 branch January 5, 2018 19:20

jbrockmendel mentioned this pull request Jan 5, 2018

DataFrame vs Series vs Index arithmetic Roundup #18824

Closed

59 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for TDI issues already fixed #19044

Tests for TDI issues already fixed #19044

jbrockmendel commented Jan 3, 2018 •

edited by jreback

Loading

gfyoung Jan 3, 2018

jbrockmendel Jan 3, 2018

gfyoung Jan 3, 2018 •

edited

Loading

jbrockmendel Jan 3, 2018

gfyoung Jan 3, 2018

gfyoung commented Jan 3, 2018

codecov bot commented Jan 3, 2018 •

edited

Loading

pep8speaks commented Jan 3, 2018 •

edited

Loading

jreback Jan 3, 2018

jbrockmendel Jan 3, 2018

jreback Jan 3, 2018

jbrockmendel Jan 3, 2018

jreback Jan 3, 2018

jbrockmendel Jan 3, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jreback Jan 4, 2018

jbrockmendel Jan 4, 2018

jreback Jan 4, 2018

jbrockmendel Jan 4, 2018 •

edited

Loading

jreback Jan 4, 2018

jreback Jan 4, 2018

jbrockmendel Jan 4, 2018

jreback commented Jan 5, 2018

jbrockmendel commented Jan 5, 2018

jreback commented Jan 5, 2018

Tests for TDI issues already fixed #19044

Tests for TDI issues already fixed #19044

Conversation

jbrockmendel commented Jan 3, 2018 • edited by jreback Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Jan 3, 2018

codecov bot commented Jan 3, 2018 • edited Loading

Codecov Report

pep8speaks commented Jan 3, 2018 • edited Loading

Comment last updated on January 05, 2018 at 16:45 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 5, 2018

jbrockmendel commented Jan 5, 2018

jreback commented Jan 5, 2018

jbrockmendel commented Jan 3, 2018 •

edited by jreback

Loading

gfyoung Jan 3, 2018 •

edited

Loading

codecov bot commented Jan 3, 2018 •

edited

Loading

pep8speaks commented Jan 3, 2018 •

edited

Loading

jbrockmendel Jan 4, 2018 •

edited

Loading