COMPAT: Return NotImplemented for subclassing #31136

TomAugspurger · 2020-01-19T17:48:06Z

This changes index ops to check the type of the argument in index
ops, rather than just the dtype. This lets index subclasses take control
of binary ops when they know better what the result should be.

Closes #31109

cc @jbrockmendel, I've fixed this as close to the root of the problem as I could, but perhaps this case should be checked elsewhere? As written currently, it only affects add & sub between an Index and a datelike index, but perhaps it should be done more generally?

This changes index ops to check the *type* of the argument in index ops, rather than just the dtype. This lets index subclasses take control of binary ops when they know better what the result should be. Closes pandas-dev#31109

TomAugspurger · 2020-01-19T17:54:05Z

Mmm, this is the wrong place to fix it. We've already unboxed the array in other by the time we get to my changes... Will take another crack.

pandas/core/arrays/datetimelike.py

jbrockmendel · 2020-01-20T02:04:48Z

LGTM, azure error looks unrelated

pandas/core/indexes/extension.py

TomAugspurger · 2020-01-20T16:32:59Z

Just a regression on master.

jreback · 2020-01-20T19:56:33Z

k merge on green

jorisvandenbossche · 2020-01-20T20:00:46Z

pandas/core/indexes/extension.py

@@ -110,6 +114,15 @@ def wrapper(self, other):

 def make_wrapped_arith_op(opname):
    def method(self, other):
+        if (
+            isinstance(other, Index)
+            and is_object_dtype(other.dtype)


Why only object dtype? You could have Index subclasses with a different dtype? (or is that not realistic?)

That's my suspicion. It's at least true for the xarray case.

@dcherian does xarray have any Index subclasses that have a non object dtype?

This is a question for @shoyer

Xarray only has one Index subclass, which is of object dtype. It's definitely a little fragile, though, so we would like to eventually get rid of it.

so we would like to eventually get rid of it.

IIRC CFTime is for the "UT" timezone, which is always within 1 second of UTC, right? Can this be represented as a tzinfo object?

pandas/tests/arithmetic/test_object.py

jreback

looks good

jreback · 2020-01-23T22:42:03Z

pandas/tests/arithmetic/test_object.py

@@ -323,3 +324,51 @@ def test_rsub_object(self):

        with pytest.raises(TypeError):
            np.array([True, pd.Timestamp.now()]) - index
+
+    @pytest.mark.parametrize(


I think you should move this to:

tests/indexes/test_subclass.py

I don't see an indexes/test_subclass.py. Start a new file for this?

TomAugspurger · 2020-01-24T14:36:59Z

Moving the conversation in https://github.com/pandas-dev/pandas/pull/31136/files/35549e681845d8505250dda00a9c22cff915f67f#diff-8ccfea8b354e9d2156ebc3150f69a18b out of inline.

Python has a behavior I hadn't realized before. Typically, for A + B, that calls A.__add__(B), and calls B.__radd__(A) if needed. However, if B is a subclass of A, then B.__radd__(A) is called first.

In [8]: class A:
   ...:     def __add__(self, other):
   ...:         print('a-add')
   ...:         return 0
   ...:     def __radd__(self, other):
   ...:         print('a-radd')
   ...:         return 1
   ...:

In [9]: class B:
   ...:     def __add__(self, other):
   ...:         print('b-add')
   ...:         return 2
   ...:     def __radd__(self, other):
   ...:         print("b-radd")
   ...:         return 3
   ...:

In [10]: A() + B()
a-add
Out[10]: 0

In [11]: class B(A):  # subclass
    ...:     def __add__(self, other):
    ...:         print('b-add')
    ...:         return 2
    ...:     def __radd__(self, other):
    ...:         print("b-radd")
    ...:         return 3
    ...:

In [12]: A() + B()
b-radd
Out[12]: 3

This means that an object dtype Index would have deferred to an Index subclass, just implicitly since Python does that. So numeric is the only behavior change we need to consider.

jbrockmendel · 2020-01-26T17:58:42Z

So numeric is the only behavior change we need to consider.

Numeric shouldn't be relevant for the CFDatetimeIndex case of interest right? I think the fix here should stay narrow and targeted at that for 1.0.

TomAugspurger · 2020-01-27T14:42:38Z

@jorisvandenbossche can you comment on #31136 (comment)? Does it address your concerns in #31136 (comment)?

TomAugspurger · 2020-01-28T14:02:45Z

Merging later today if there aren't any objections.

jorisvandenbossche · 2020-01-28T15:25:34Z

This means that an object dtype Index would have deferred to an Index subclass, just implicitly since Python does that. So numeric is the only behavior change we need to consider.

You mean because if you subclass Index, ops with Index class (i.e. object dtype) already get dispatched fine by python's mechanism. But for ops with numeric indices, the Index subclass is not a subclass of the numeric Index in that case, so therefore we need to handle it specifically there?

TomAugspurger · 2020-01-28T15:48:42Z

But for ops with numeric indices, the Index subclass is not a subclass of the numeric Index in that case[...]

All this is correct. As an example, if you have

class MyIndex(Index):
    pass

Then Int64Index() + MyIndex() will use Int64Index.__add__ since MyIndex isn't a subclass of Int64Index.

[...], so therefore we need to handle it specifically there?

This is where I think we disagree. In general, I don't think that all our Index subclasses can / should be doing these checks.

Ideally in the future we have an ExtensionIndex API and CFTimeIndex is an instance of that. But for now I think we should just restore the 0.25 behavior.

jorisvandenbossche · 2020-01-28T15:57:25Z

[...], so therefore we need to handle it specifically there?

This is where I think we disagree. In general, I don't think that all our Index subclasses can / should be doing these checks.

Ah, sorry, I thought this was what you were doing in this PR. So now you do the check not in general for numeric index, but only for EA-based indexes (to have it for datetime-like indexes) ?

TomAugspurger · 2020-01-28T16:01:46Z

So now you do the check not in general for numeric index, but only for EA-based indexes

Anything using make_wrapped_arith_op, which in practice is just the Datetimelike indexes.

jorisvandenbossche · 2020-01-28T16:06:38Z

OK, I was just going from the fact it being in pandas/core/indexes/extension.py.

Anyway, I am fine with focusing on getting the compatibility with 0.25.
And you are right that longer term the better way is to allow storing general ExtensionArrays in the Index.

TomAugspurger · 2020-01-28T21:14:13Z

@meeseeksdev backport to 1.0.x

…assing

…1405) Co-authored-by: Tom Augspurger <[email protected]>

COMPAT: Return NotImplemented for subclassing

1a03084

This changes index ops to check the *type* of the argument in index ops, rather than just the dtype. This lets index subclasses take control of binary ops when they know better what the result should be. Closes pandas-dev#31109

TomAugspurger added the Numeric Operations Arithmetic, Comparison, and Logical operations label Jan 19, 2020

TomAugspurger added this to the 1.0.0 milestone Jan 19, 2020

fixup

db950c9

jbrockmendel reviewed Jan 19, 2020

View reviewed changes

pandas/core/arrays/datetimelike.py Outdated Show resolved Hide resolved

TomAugspurger added 2 commits January 19, 2020 15:05

fixup

900dacb

fixup

7d057be

jreback requested changes Jan 20, 2020

View reviewed changes

pandas/core/indexes/extension.py Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into 31109-object-ops

35549e6

jreback approved these changes Jan 20, 2020

View reviewed changes

jorisvandenbossche requested changes Jan 20, 2020

View reviewed changes

TomAugspurger added 4 commits January 20, 2020 15:06

Merge remote-tracking branch 'upstream/master' into 31109-object-ops

8b9f5af

added xfails

ec51207

Merge remote-tracking branch 'upstream/master' into 31109-object-ops

b80f8fa

Merge remote-tracking branch 'upstream/master' into 31109-object-ops

d257f65

jreback reviewed Jan 23, 2020

View reviewed changes

object

a3b5c7a

jorisvandenbossche approved these changes Jan 28, 2020

View reviewed changes

TomAugspurger merged commit 3dc59d4 into pandas-dev:master Jan 28, 2020

TomAugspurger deleted the 31109-object-ops branch January 28, 2020 20:57

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Jan 28, 2020

Backport PR pandas-dev#31136: COMPAT: Return NotImplemented for subcl…

1d9a882

…assing

meeseeksmachine mentioned this pull request Jan 28, 2020

Backport PR #31136 on branch 1.0.x (COMPAT: Return NotImplemented for subclassing) #31405

Merged

TomAugspurger added a commit that referenced this pull request Jan 28, 2020

Backport PR #31136: COMPAT: Return NotImplemented for subclassing (#3…

af208b3

…1405) Co-authored-by: Tom Augspurger <[email protected]>

dcherian mentioned this pull request Jan 28, 2020

Test failures with pandas master pydata/xarray#3673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COMPAT: Return NotImplemented for subclassing #31136

COMPAT: Return NotImplemented for subclassing #31136

TomAugspurger commented Jan 19, 2020

TomAugspurger commented Jan 19, 2020

jbrockmendel commented Jan 20, 2020

TomAugspurger commented Jan 20, 2020

jreback commented Jan 20, 2020

jorisvandenbossche Jan 20, 2020

TomAugspurger Jan 20, 2020

dcherian Jan 20, 2020

shoyer Jan 20, 2020

jbrockmendel Jan 26, 2020

jreback left a comment

jreback Jan 23, 2020

TomAugspurger Jan 24, 2020

TomAugspurger commented Jan 24, 2020

jbrockmendel commented Jan 26, 2020

TomAugspurger commented Jan 27, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020

COMPAT: Return NotImplemented for subclassing #31136

COMPAT: Return NotImplemented for subclassing #31136

Conversation

TomAugspurger commented Jan 19, 2020

TomAugspurger commented Jan 19, 2020

jbrockmendel commented Jan 20, 2020

TomAugspurger commented Jan 20, 2020

jreback commented Jan 20, 2020

jorisvandenbossche Jan 20, 2020

Choose a reason for hiding this comment

TomAugspurger Jan 20, 2020

Choose a reason for hiding this comment

dcherian Jan 20, 2020

Choose a reason for hiding this comment

shoyer Jan 20, 2020

Choose a reason for hiding this comment

jbrockmendel Jan 26, 2020

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback Jan 23, 2020

Choose a reason for hiding this comment

TomAugspurger Jan 24, 2020

Choose a reason for hiding this comment

TomAugspurger commented Jan 24, 2020

jbrockmendel commented Jan 26, 2020

TomAugspurger commented Jan 27, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020

jorisvandenbossche commented Jan 28, 2020

TomAugspurger commented Jan 28, 2020