ENH: Support ExtensionArray operators via a mixin
State: closed

- [x] closes #19577
- [x] tests added / passed
    - tests/extension/decimal/test_decimal.py:TestOperator
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry
    - in v0.24.0

Based on a discussion in #20889, this provides a mixin (via a mixin factory) that provides a default implementation of operators for `ExtensionArray` using the operators defined on the underlying `ExtensionDtype` .  Tested using the `DecimalArray` implementation.

NOTE: This requires #21183 ~~and #21260~~ to be accepted into master, and so the changes from those pull requests are included here.

Comments from @jorisvandenbossche @TomAugspurger @jreback @jbrockmendel are welcome.



Labels:
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=h1) Report
> Merging [#21261](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0801b8c9090752c1342962aecd03e9a0468adbd4?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `94.28%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21261/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21261      +/-   ##
==========================================
+ Coverage    91.9%    91.9%   +<.01%     
==========================================
  Files         154      154              
  Lines       49557    49625      +68     
==========================================
+ Hits        45544    45608      +64     
- Misses       4013     4017       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.27% <94.28%> (ø)` | :arrow_up: |
| #single | `42.06% <65.71%> (+0.03%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/21261/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL19faW5pdF9fLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/21261/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `96.47% <100%> (+0.05%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/21261/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `85.22% <100%> (+0.03%)` | :arrow_up: |
| [pandas/api/extensions/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/21261/diff?src=pr&el=tree#diff-cGFuZGFzL2FwaS9leHRlbnNpb25zL19faW5pdF9fLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/21261/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `87.59% <92.85%> (+3.64%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=footer). Last update [0801b8c...8fc93e4](https://codecov.io/gh/pandas-dev/pandas/pull/21261?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jreback The idea is to use one piece of code for all of the operators, and to use the implementation of the operator on the underlying `ExtensionDtype`, **_if_** that implementation exists.

Also, this is following the pattern used in `ops.py`, where the operators are generated dynamically, using the same code to call each operator.
@jbrockmendel @jreback @jorisvandenbossche My latest commit also does something with the JSON example to see that we trap different kinds of exceptions correctly.  I'm not sure if I should put that there or not.  It also illustrates that you can define just single methods using the mixin, i.e., if your underlying dtype only supports addition, but nothing else, you can then define an operator for addition and not define the other ones.
This needs some tests, seee: https://github.com/pandas-dev/pandas/pull/21160/files#diff-f12c8797eb9d98e36fd196d5e0821f75  for a generic way to do this.

FInally I would suggest that the implementation of Categorical use this (can be a followup PR) to validate this approach. 

Too much code has been added w/o adequate testing.
Hello @Dr-Irv! Thanks for updating the PR.



























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

##### Comment last updated on June 29, 2018 at 00:18 Hours UTC
@jreback @jorisvandenbossche @jbrockmendel I have just pushed a new commit that addresses a number of issues raised above, including:
  - Updated whatsnew to provide more of a description
  - Renamed the mixin classes
  - Let errors generated by missing implementations of the underlying classes get reported as is
  - Implement tests based on test framework written by @jreback elsewhere.

@jorisvandenbossche @TomAugspurger Latest commit reorganizes the tests (based on your suggestions), adds docs to extending.rst, and most importantly, fixes a bug in how the reverse operators were called.
@jreback Can you take a look at https://github.com/pandas-dev/pandas/pull/21183 (then the diff here will be smaller), both me and Tom approved it, but since you actually commented on that code but in this PR, please take a look there.
@jorisvandenbossche You wrote:

> As I said in my previous comment, I am not really sure about the added value of this more complex implementation, or at least for now.
> 
> My take: let's focus in this PR on what you actually wanted: a mixin that you can inherit if you want default scalar-ops-fallback behaviour, and leave more generic ways to add ops for another PR, as that needs more discussion.

I'm in agreement with you.  I was responding to what I think was on the mind of @jreback.  I'm not sure how to reconcile your position with his.

Having said that, the hooks (`dispatch_to_extension_op()` that I have put in pandas/core/ops.py will work independent of how a developer implements the ops on the ExtensionArray subclass. 

IMHO, I view the original implementation in the PR as a stepping stone towards having more generic ops support for EA's.

I'm going to wait until @jreback chimes in, and then you guys need to let me know which way I should go, once you reconcile your differences of opinion.
well I have state my point many times. I would simply drop the scalar implementation entirely. I agree its getting too complex. If an extension class wants to implement ops, then let them. But providing scalar support is a pretty specific usecase IMHO (and to be honest not very general).
@Dr-Irv your implemenation is way too complex here.

Just make 2 classes. Define the actual operators

```
class _ExtensionArithmeticOpsMixin(base):
+        __add__ = base._create_method(operator.add)
+        __radd__ = base._create_method(ops.radd)
+        __sub__ = base._create_method(operator.sub)
+        __rsub__ = base._create_method(ops.rsub)
+        __mul__ = base._create_method(operator.mul)
+        __rmul__ = base._create_method(ops.rmul)
+        __pow__ = base._create_method(operator.pow)
+        __rpow__ = base._create_method(ops.rpow)
+        __mod__ = base._create_method(operator.mod)
+        __rmod__ = base._create_method(ops.rmod)
+        __floordiv__ = base._create_method(operator.floordiv)
+        __rfloordiv__ = base._create_method(ops.rfloordiv)
+        __truediv__ = base._create_method(operator.truediv)
+        __rtruediv__ = base._create_method(ops.rtruediv)
+        if not PY3:
+            __div__ = base._create_method(operator.div)
+            __rdiv__ = base._create_method(ops.rdiv)
+
+        __divmod__ = base._create_method(divmod)
+        __rdivmod__ = base._create_method(ops.rdivmod)
```
and that's it.

If you want to extend and do scalar type artihmetic in our own extension class then by all means.
Not sure any reason for anything beyond basic ops definitions should live here.

@jreback You wrote:

> If an extension class wants to implement ops, then let them. But providing scalar support is a pretty specific usecase IMHO (and to be honest not very general).

I respectfully disagree. The use case is exhibited with `DecimalArray`. I also have my own use case, where the underlying scalars all have the operators defined and are objects, so the mixin implementation takes care of that.

In addition, with what I previously implemented, we end up testing the hooks for operators on `ExtensionArray`, independent of whether someone implements each operator.



@jreback You wrote:

> @Dr-Irv your implemenation is way too complex here.
> 
> Just make 2 classes. Define the actual operators
> 

> ```
> class _ExtensionArithmeticOpsMixin(base):
> +        __add__ = base._create_method(operator.add)
> +        __radd__ = base._create_method(ops.radd)
> +        __sub__ = base._create_method(operator.sub)
> ## Omitting rest
> ```

The problem here is that `_create_method()` needs to be a class method of the parent class.  So you need to know the parent class.  I don't see how your idea will work.

I'm going to revert back to the prior implementation, where there is a base `ExtensionScalarOpsMixin` and `ExtensionScalarArithmeticMixin` and `ExtensionScalarComparisonMixin` inherit from that.

> well I have state my point many times. I would simply drop the scalar implementation entirely. 

The scalar implementation is what this PR was proposing from the beginning, and AFAIK you agreed on providing, for those users that want it, an *optional* mixin that provides scalar-ops fallback behaviour. Yes, you made clear comments about not liking the *implementation*, but not about the starting point of this PR. 
This PR was not about adding a generic ops implementation (as you have a PR for that in https://github.com/pandas-dev/pandas/pull/21191), but purely the scalar ops mixin. Let's just keep it separated that way, and discuss the generic ops in a separate issue/PR.

> The problem here is that _create_method() needs to be a class method of the parent class. So you need to know the parent class. I don't see how your idea will work.

Can't this be `self`?
@jreback more or less in agreement with @jorisvandenbossche here, though my main thought is that the scalar-dispatch is effectively what will be needed for object-dtyped arrays.
I wrote:
> The problem here is that _create_method() needs to be a class method of the parent class. So you need to know the parent class. I don't see how your idea will work.

@jorisvandenbossche 
>  Can't this be self?

No, because we want `__add__`, `__sub__`, etc. to be defined on the subclass of the `ExtensionArray` _class_, not on each instance of the `ExtensionArray` subclass.

For example, using `DecimalArray`, we want `__add__` to be defined on `DecimalArray`, and since `_create_method()` contains the code to create the  method, `_create_method()` is a class method.

as i did in my impl

this is so much simpler to just make class methods

then you opt in by 

MyExtensionArray._add_numeric_ops()

and just override the _create_method
(or even could just pass it here directly)


@jreback YOu wrote:
> as i did in my impl
> 
> this is so much simpler to just make class methods
> 
> then you opt in by
> 
> MyExtensionArray._add_numeric_ops()
> 
> and just override the _create_method
> (or even could just pass it here directly)

OK, but that's not a true mixin.  I'll do as you suggest.

@jreback so what you propose (from a user perspective) is to have a user do

```
class MyExtensionArray(ExtensionArray, ExtensionScalarOpsMixin)
    pass

MyExtensionArray._add_arithmetic_ops()
MyExtensionArray._add_comparison_ops()
```

(where the Mixin implements the `_make_..` methods) 
instead of the current

```
class MyExtensionArray(ExtensionArray, 
                       ExtensionScalarArithmeticOpsMixin,
                       ExtensionScalarComparisonOpsMixin)
    pass

```

to be clear on the API you propose?
@jbrockmendel wrote:

> @jreback more or less in agreement with @jorisvandenbossche here, though my main thought is that the scalar-dispatch is effectively what will be needed for object-dtyped arrays.

Yes, that is exactly my use case!

For @jreback and @jorisvandenbossche , latest commit does it the way that @jreback suggests

@jreback following up on the conversation at the pandas dev meeting, I have removed the code that is in #21260 (since this PR no longer needs it), so I think this one here is good to go.
@Dr-Irv ok rebased. ping on green.
thanks!
@jreback there were still some unresolved comments. @Dr-Irv can you do a follow-up PR for the case where an EA is the right hand side of operation?
2018-06-29 15:51 GMT+02:00 Dr. Irv <notifications@github.com>:

> *@Dr-Irv* commented on this pull request.
> ------------------------------
>
> In pandas/core/arrays/base.py
> <https://github.com/pandas-dev/pandas/pull/21261#discussion_r199165171>:
>
> > +                    ovalues = param
> +                else:  # Assume its an object
> +                    ovalues = [param] * len(self)
> +                return ovalues
> +            lvalues = self
> +            rvalues = convert_values(other)
> +
> +            # If the operator is not defined for the underlying objects,
> +            # a TypeError should be raised
> +            res = [op(a, b) for (a, b) in zip(lvalues, rvalues)]
> +
> +            if coerce_to_dtype:
> +                try:
> +                    res = self._from_sequence(res)
> +                except TypeError:
> +                    pass
>
> @jorisvandenbossche <https://github.com/jorisvandenbossche> But why
> repeat that logic? If we leave it as a list, then the Series constructor
> will do the inference on the dtype.
>
>
> Because IMO the ExtensionArray itself should also do the sensible thing
(eg sometimes you want to act on the values, eg if you don't want
alignment). And returning a list is then not consistent and not useful.
Note that it's not fully repeating the logic, it's only reusing the
underlying functionality (in principle it should be a single helper
function, but would need to look that up), and no inference needs to happen
in the Series constructor.