BUG: fix index op names and pinning #19723

jbrockmendel · 2018-02-16T02:09:12Z

This is a moderately big one, but changes very little.

Fixes incorrect Index ops names and several methods that are defined but not pinned to the class (#19716).

Avoids the need to pass reversed and str_rep in a bunch of places, cleans up some of the Index classmethods.

Removes the no-longer-needed catching of AttributeError in RangeIndex ops.

The part that may be controversial (and almost certainly needs new tests) is that Index.__rsub__ is implemented, since it doesn't currently exist. This implements it in "the obvious way", but the fact that Index.__sub__ isn't defined in the obvious way suggests this might not be desired. Comments welcome.

closes Incorrectly assigned Index ops/names #19716
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2018-02-16T03:25:12Z

Codecov Report

Merging #19723 into master will increase coverage by 0.01%.
The diff coverage is 92.36%.

@@            Coverage Diff             @@
##           master   #19723      +/-   ##
==========================================
+ Coverage   91.57%   91.58%   +0.01%     
==========================================
  Files         150      150              
  Lines       48903    48890      -13     
==========================================
- Hits        44785    44778       -7     
+ Misses       4118     4112       -6

Flag	Coverage Δ
#multiple	`89.96% <92.36%> (ø)`	⬆️
#single	`41.83% <77.86%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.56% <100%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.63% <100%> (ø)`	⬆️
pandas/core/indexes/period.py	`93.02% <100%> (+0.14%)`	⬆️
pandas/core/indexes/datetimelike.py	`97.33% <100%> (+0.43%)`	⬆️
pandas/core/indexes/range.py	`95.63% <85.71%> (-0.11%)`	⬇️
pandas/core/indexes/base.py	`96.53% <91.3%> (+0.11%)`	⬆️
pandas/core/frame.py	`97.23% <0%> (ø)`	⬆️
... and 1 more

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 b585e3b...0824382. Read the comment docs.

jreback

so leave out the rsub changes for now. the refactor part is ok

jreback · 2018-02-16T18:50:45Z

pandas/core/indexes/datetimelike.py

-                    raise TypeError("cannot subtract TimedeltaIndex and {typ}"
-                                    .format(typ=type(other).__name__))
-                return self._add_delta(-other)
+                assert not is_timedelta64_dtype(other)


this is redundant

Will remove.

jreback · 2018-02-16T18:53:00Z

its also not clear the changes are actually being tested.

jbrockmendel · 2018-02-16T19:13:54Z

its also not clear the changes are actually being tested.

The added parameters in tests.indexes.test_base test the relevant changes here. That test would fail under the status-quo.

jbrockmendel · 2018-02-16T19:14:28Z

so leave out the rsub changes for now. the refactor part is ok

OK. That will require special-casing to escape that case in tests.indexes.test_base; we can revisit later.

…prep

jbrockmendel · 2018-02-19T17:00:33Z

This one isn’t particularly conducive to splitting into smaller parts, but I could try if that’ll help.

jreback · 2018-02-19T17:34:28Z

pandas/tests/indexes/test_base.py

 def test_generated_op_names(opname, indices):
    index = indices
+    if type(index) is pd.Index and opname == 'rsub':


use isinstance, or is this a very specific check if some is NOT a Index. maybe better to use ABCIndex and/or ABCIndexClass

This is specific to pd.Index, not subclasses.

pls change this

jreback

need some tests for iadd/isub, in particular

#17067 (2 things going on in that issue(

e.g.

s = pd.Series([1, 2, 3])
s.index.name = "foo"

s.index += 1
assert s.index.name == "foo"

jbrockmendel · 2018-02-20T03:47:02Z

The reset_index thing looks unrelated. Am I reading it wrong?

jreback · 2018-02-20T03:56:14Z

the reset_index is unrelated
the i ops are the relevant parts

jbrockmendel · 2018-02-20T17:09:30Z

This may be all set.

jreback · 2018-02-20T23:58:01Z

pandas/core/indexes/datetimelike.py

+        def __iadd__(self, other):
+            # alias for __add__
+            return self.__add__(other)
+        cls.__iadd__ = __iadd__


why do we need this double assignment ?

If we just set cls.__iadd__ = __add__ then when we check for Index.__iadd__.__name__ we'll get __add__ instead of __iadd__. Not a big deal, but its cheap to make it pretty.

jreback · 2018-02-20T23:59:17Z

pandas/tests/indexes/test_base.py

 def test_generated_op_names(opname, indices):
    index = indices
+    if type(index) is pd.Index and opname == 'rsub':


pls change this

jbrockmendel · 2018-02-21T00:14:50Z

pls change this

Sure, to issubclass(index, ABCIndex)? Not worth arguing about, but whats the upside? It's definitely not clearer (especially b/c it is easy for newcomers to not be aware that ABCIndex doesnt include subclasses)

…prep

jorisvandenbossche · 2018-02-21T12:51:23Z

It's definitely not clearer (especially b/c it is easy for newcomers to not be aware that ABCIndex doesnt include subclasses)

I for one didn't know (and wouldn't know directly from reading code, and find it also surprising) that ABCIndex is only the Index class and not its subclasses. type(..) is Index is much more explicit IMO.

jorisvandenbossche · 2018-02-21T13:02:29Z

I for one didn't know (and wouldn't know directly from reading code, and find it also surprising) that ABCIndex is only the Index class and not its subclasses. type(..) is Index is much more explicit IMO.

But I have to agree it is clear from the abc code (I just don't interact much with that part of the code base that needs to use ABCs). I only intuitively would have expected the names to be the other way around :-)

jorisvandenbossche

Does this need a whatsnew note?

jbrockmendel · 2018-02-21T18:22:43Z

Does this need a whatsnew note?

I guess its technically user-facing, but it seems like it would be mostly clutter. I'm fine either way.

jreback · 2018-02-22T00:19:34Z

needs a rebase

…prep

jbrockmendel · 2018-02-22T06:34:05Z

I messed up most recent merge, will fix in AM.

jreback · 2018-02-23T01:36:39Z

lgtm. ping on green. (I think just appeveyor left).

jbrockmendel · 2018-02-23T04:53:48Z

ping

jreback · 2018-02-23T11:36:45Z

thanks

jbrockmendel added 4 commits February 15, 2018 17:57

fix index op names and pinning

e20da51

whitespace fixup

885dd68

revert non-central

25af8eb

revert changes handled in other PR

c5fdf53

jreback requested changes Feb 16, 2018

View reviewed changes

jreback added Datetime Datetime data dtype API Design Numeric Operations Arithmetic, Comparison, and Logical operations Timedelta Timedelta data type labels Feb 16, 2018

jbrockmendel added 4 commits February 16, 2018 12:26

revert Index.__rsub__

a68826d

remove assertion

eb23db0

unwrap line

c3d9d9f

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

ef845af

…prep

jreback reviewed Feb 19, 2018

View reviewed changes

jreback requested changes Feb 19, 2018

View reviewed changes

test for iadd and isub preserving name

88c9638

jreback requested changes Feb 20, 2018

View reviewed changes

jbrockmendel added 2 commits February 20, 2018 16:17

make things less obvious

ba2d4ca

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

5773fd8

…prep

jorisvandenbossche changed the title ~~Fix index op names and pinning~~ BUG: fix index op names and pinning Feb 21, 2018

jorisvandenbossche reviewed Feb 21, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Feb 22, 2018

jbrockmendel added 2 commits February 21, 2018 16:26

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

f6412e5

…prep

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

f2e852c

…prep

jbrockmendel added 2 commits February 22, 2018 08:51

fix rebase screwup

4c5e54f

fix error caused by leftover reversed overlap with built-in name

0824382

jreback approved these changes Feb 23, 2018

View reviewed changes

jreback merged commit 0ffc4b5 into pandas-dev:master Feb 23, 2018

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

BUG: fix index op names and pinning (pandas-dev#19723)

d5c6167

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix index op names and pinning #19723

BUG: fix index op names and pinning #19723

jbrockmendel commented Feb 16, 2018

codecov bot commented Feb 16, 2018 •

edited

Loading

jreback left a comment

jreback Feb 16, 2018

jbrockmendel Feb 16, 2018

jreback commented Feb 16, 2018

jbrockmendel commented Feb 16, 2018

jbrockmendel commented Feb 16, 2018

jbrockmendel commented Feb 19, 2018

jreback Feb 19, 2018

jbrockmendel Feb 19, 2018

jreback Feb 20, 2018

jreback left a comment

jbrockmendel commented Feb 20, 2018

jreback commented Feb 20, 2018

jbrockmendel commented Feb 20, 2018

jreback Feb 20, 2018

jbrockmendel Feb 21, 2018

jreback Feb 20, 2018

jbrockmendel commented Feb 21, 2018

jorisvandenbossche commented Feb 21, 2018

jorisvandenbossche commented Feb 21, 2018

jorisvandenbossche left a comment

jbrockmendel commented Feb 21, 2018

jreback commented Feb 22, 2018

jbrockmendel commented Feb 22, 2018

jreback commented Feb 23, 2018

jbrockmendel commented Feb 23, 2018

jreback commented Feb 23, 2018

BUG: fix index op names and pinning #19723

BUG: fix index op names and pinning #19723

Conversation

jbrockmendel commented Feb 16, 2018

codecov bot commented Feb 16, 2018 • edited Loading

Codecov Report

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

jreback commented Feb 16, 2018

jbrockmendel commented Feb 16, 2018

jbrockmendel commented Feb 16, 2018

jbrockmendel commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 20, 2018

jreback commented Feb 20, 2018

jbrockmendel commented Feb 20, 2018

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

jorisvandenbossche commented Feb 21, 2018

jorisvandenbossche commented Feb 21, 2018

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 21, 2018

jreback commented Feb 22, 2018

jbrockmendel commented Feb 22, 2018

jreback commented Feb 23, 2018

jbrockmendel commented Feb 23, 2018

jreback commented Feb 23, 2018

codecov bot commented Feb 16, 2018 •

edited

Loading