Fix name setting in DTI/TDI add and sub #19744

jbrockmendel · 2018-02-17T23:01:13Z

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

codecov · 2018-02-17T23:50:44Z

Codecov Report

Merging #19744 into master will increase coverage by <.01%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##           master   #19744      +/-   ##
==========================================
+ Coverage   91.61%   91.61%   +<.01%     
==========================================
  Files         150      150              
  Lines       48887    48880       -7     
==========================================
- Hits        44786    44783       -3     
+ Misses       4101     4097       -4

Flag	Coverage Δ
#multiple	`89.99% <96.61%> (ø)`	⬆️
#single	`41.81% <35.59%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/common.py	`93.48% <ø> (+0.06%)`	⬆️
pandas/core/indexes/period.py	`93.04% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.56% <100%> (+0.24%)`	⬆️
pandas/core/indexes/timedeltas.py	`90.63% <100%> (+0.06%)`	⬆️
pandas/core/series.py	`94.46% <100%> (ø)`	⬆️
pandas/core/ops.py	`96.65% <95.23%> (-0.1%)`	⬇️
pandas/core/indexes/datetimelike.py	`97.1% <96%> (+0.04%)`	⬆️

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 740ad9a...249b7ba. Read the comment docs.

jreback · 2018-02-18T17:17:06Z

pandas/core/indexes/timedeltas.py

@@ -358,17 +358,13 @@ def _maybe_update_attributes(self, attrs):
    def _add_delta(self, delta):
        if isinstance(delta, (Tick, timedelta, np.timedelta64)):


should prob add a doc-string to these also making the guarantee that the name should not be set

jreback · 2018-02-18T17:17:33Z

pandas/core/indexes/datetimelike.py

@@ -726,6 +736,11 @@ def __sub__(self, other):
            else:  # pragma: no cover
                return NotImplemented

+            if result is not NotImplemented:
+                res_name = ops._get_series_op_result_name(self, other)
+                result = result.rename(name=res_name)


this copies again, just set the name

jreback · 2018-02-18T17:17:56Z

pandas/core/indexes/datetimelike.py

@@ -726,6 +736,11 @@ def __sub__(self, other):
            else:  # pragma: no cover
                return NotImplemented

+            if result is not NotImplemented:
+                res_name = ops._get_series_op_result_name(self, other)


maybe rename this to

get_op_result_name

jreback · 2018-02-18T17:19:16Z

pandas/core/indexes/datetimes.py


        if isinstance(delta, (Tick, timedelta, np.timedelta64)):
            new_values = self._add_delta_td(delta)
        elif is_timedelta64_dtype(delta):
            if not isinstance(delta, TimedeltaIndex):
                delta = TimedeltaIndex(delta)
-            else:
-                # update name when delta is Index
-                name = com._maybe_match_name(self, delta)


let's move _maybe_match_name to ops and put next to get_op_result_name (and should remove it in favor of get_op_result_name, but that might be slightly tricky).

OK. While I'm at it I'm going to move get_op_result_name with the utility functions near the top of the file instead of in the arithmetic-specific spot it is now.

jreback · 2018-02-18T17:20:46Z

pandas/tests/indexes/datetimes/test_arithmetic.py

        # GH#18824
        dti = pd.date_range('2017-01-01', periods=2, tz=tz)
-        other = box([pd.offsets.MonthEnd(), pd.offsets.Day(n=2)])


why are you removing all of the boxing? do these not work?

box is parametrized over [np.array, pd.Index], but the pd.Index case is being separated out into its own test that also checks names.

…like

jreback · 2018-02-20T01:54:07Z

pandas/core/ops.py

+    return name
+
+
+def _maybe_match_name(a, b):


do we need this as a standalone? (IOW can you replace the current usage with get_op_result_name)?

if not can you doc-string this

A lot of the uses can be replaced without affecting any existing behavior. I'll see if I can get them all in a way that doesn't necessitate new tests. If not I'll kill it off in the next pass.

Yep, all non-test usages can be replaced. I've done that and then also added a docstring. Getting rid of it and updating the tests to test get_op_result_name directly will wait for another round with narrower scope.

ok looks good. next pass let's try to remove this (I think you did, just need to transfer the tests)

jreback · 2018-02-20T01:54:59Z

pandas/core/series.py

@@ -1814,7 +1814,7 @@ def combine_first(self, other):
        this = self.reindex(new_index, copy=False)
        other = other.reindex(new_index, copy=False)
        # TODO: do we need name?
-        name = com._maybe_match_name(self, other)  # noqa


anyidea why the noqa is here?

I'd speculate that it's because name is defined and then for some reason not used, so flake8 should complain.

jreback · 2018-02-20T11:06:03Z

pandas/core/indexes/datetimelike.py

@@ -726,6 +736,11 @@ def __sub__(self, other):
            else:  # pragma: no cover
                return NotImplemented

+            if result is not NotImplemented:
+                res_name = ops.get_op_result_name(self, other)
+                result.rename(name=res_name, inplace=True)


simply set .name (and same above)

jreback · 2018-02-20T11:06:41Z

pandas/core/indexes/datetimes.py

            return self + other[0]
        else:
            warnings.warn("Adding/subtracting array of DateOffsets to "
                          "{} not vectorized".format(type(self)),
                          PerformanceWarning)
            return self.astype('O') + np.array(other)
+            # TODO: pass freq='infer' like we do in _sub_offset_array?


what is the purpose of these comments (esp here, after the return)?

a) to make sure it gets seen during review, b) as a note to myself or the next pass

jreback · 2018-02-20T11:08:53Z

pandas/tests/test_common.py

        Series([1], name='x'), Series(
            [2], name='x'))
    assert (matched == 'x')

-    matched = com._maybe_match_name(


next pass, move these to test_ops.py (new)

…like

jbrockmendel · 2018-02-21T03:49:10Z

Once this goes through the remaining cases on todo list are: array[offsets] for PeriodIndex, array[int], and array[timedelta64], and last some object-dtype corner cases.

jreback

couple items for next pass. merging.

jreback · 2018-02-21T23:51:54Z

pandas/core/ops.py

+    return name
+
+
+def _maybe_match_name(a, b):


ok looks good. next pass let's try to remove this (I think you did, just need to transfer the tests)

jreback · 2018-02-21T23:52:24Z

pandas/core/series.py

@@ -1814,7 +1814,7 @@ def combine_first(self, other):
        this = self.reindex(new_index, copy=False)
        other = other.reindex(new_index, copy=False)
        # TODO: do we need name?
-        name = com._maybe_match_name(self, other)  # noqa
+        name = ops.get_op_result_name(self, other)  # noqa


clearly this is not used, so on next pass can you remove

jbrockmendel added 3 commits February 17, 2018 14:57

Delay name setting to ensure we do it consistently

6bb94fe

Whatsnew note

191b2fb

update GH reference

21a5864

jreback requested changes Feb 18, 2018

View reviewed changes

jreback added Compat pandas objects compatability with Numpy or Python functions Datetime Datetime data dtype Timedelta Timedelta data type labels Feb 18, 2018

jbrockmendel mentioned this pull request Feb 18, 2018

Split+Parametrize Timedelta tests #19736

Merged

jbrockmendel added 6 commits February 18, 2018 11:17

docstrings, rename get_series_op_result_name-->get_op_result_name

0b51e00

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

205b09c

…like

move maybe_match_name from com to ops

27ed3bb

fix missing copy/paste

d67d8e5

fix rename inplace

9bb2f9a

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

a10a9fb

…like

jreback requested changes Feb 20, 2018

View reviewed changes

replace uses of _maybe_match_name with get_op_result_name

34b2514

jreback requested changes Feb 20, 2018

View reviewed changes

jbrockmendel added 2 commits February 20, 2018 08:08

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

e78d361

…like

dont use result.rename

249b7ba

jbrockmendel mentioned this pull request Feb 21, 2018

Implement add_offset_array for PeriodIndex #19826

Merged

jreback approved these changes Feb 21, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Feb 21, 2018

jreback merged commit 80241e6 into pandas-dev:master Feb 21, 2018

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Fix name setting in DTI/TDI __add__ and __sub__ (pandas-dev#19744)

e975455

jbrockmendel deleted the dtlike branch June 22, 2018 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix name setting in DTI/TDI add and sub #19744

Fix name setting in DTI/TDI add and sub #19744

jbrockmendel commented Feb 17, 2018

codecov bot commented Feb 17, 2018 •

edited

Loading

jreback Feb 18, 2018

jreback Feb 18, 2018

jreback Feb 18, 2018

jreback Feb 18, 2018

jbrockmendel Feb 18, 2018

jreback Feb 18, 2018

jbrockmendel Feb 18, 2018

jreback Feb 20, 2018

jreback Feb 20, 2018

jbrockmendel Feb 20, 2018

jbrockmendel Feb 20, 2018

jreback Feb 21, 2018

jreback Feb 20, 2018

jbrockmendel Feb 20, 2018

jreback Feb 20, 2018

jreback Feb 20, 2018

jbrockmendel Feb 20, 2018

jreback Feb 20, 2018

jbrockmendel commented Feb 21, 2018

jreback left a comment

jreback Feb 21, 2018

jreback Feb 21, 2018

		@@ -358,17 +358,13 @@ def _maybe_update_attributes(self, attrs):
		def _add_delta(self, delta):
		if isinstance(delta, (Tick, timedelta, np.timedelta64)):

Fix name setting in DTI/TDI __add__ and __sub__ #19744

Fix name setting in DTI/TDI __add__ and __sub__ #19744

Conversation

jbrockmendel commented Feb 17, 2018

codecov bot commented Feb 17, 2018 • edited Loading

Codecov Report

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

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 commented Feb 21, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix name setting in DTI/TDI add and sub #19744

Fix name setting in DTI/TDI add and sub #19744

codecov bot commented Feb 17, 2018 •

edited

Loading