implement arith ops on pd.Categorical
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Then in core.ops we dispatch to pd.Categorical instead of special-casing.

Labels:

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

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

```diff
@@            Coverage Diff             @@
##           master   #21213      +/-   ##
==========================================
- Coverage   91.84%   91.83%   -0.01%     
==========================================
  Files         153      153              
  Lines       49505    49534      +29     
==========================================
+ Hits        45466    45492      +26     
- Misses       4039     4042       +3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <90%> (-0.01%)` | :arrow_down: |
| #single | `41.9% <76.66%> (+0.02%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21213?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/21213/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `96.21% <50%> (-0.15%)` | :arrow_down: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/21213/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.57% <92.85%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21213?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/21213?src=pr&el=footer). Last update [f6abb61...93395fb](https://codecov.io/gh/pandas-dev/pandas/pull/21213?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Why is it actually needed to add those?

At the moment the behavior is _implicitly_ defined in `core.ops`, which is very much not the right place.
> At the moment the behavior is implicitly defined in core.ops, which is very much not the right place.

I am not speaking about the change in ops.py, that is good, but just asking about the reasoning to add those methods to Categorical. Without adding those, python will also raise a TypeError (unless the `right` object knows how to handle it)
>  just asking about the reasoning to add those methods to Categorical. 

a) precedent/demonstration of the Game Plan for e.g. #21160, #20889.
b) explicit instead of implicit

> a) precedent/demonstration of the Game Plan for e.g. #21160, #20889.

Again, that's for the change in ops.py? (which I completely agree with)

> b) explicit instead of implicit

but on the other hand, not defining the method or returning NotImplemented is, although implicit, the standard python idiom to handle this.

This path is already taken when eg the categorical series it the `right` object.

To be clear, there might be good other reasons to do this, eg because it would give changes in behaviour compared to what we currently have.

> Again, that's for the change in ops.py?

No, its for the methods themselves on the EA subclasses.  #21160 already does this, so the "precedent" is more for 20889.

> To be clear, there might be good other reasons to do this, eg because it would give changes in behaviour compared to what we currently have.

The PR will keep the current exception messages unchanged.  Not a big deal.

Really if 21160 goes through it will set the relevant precedent for 20889 and other authors, at which point I don't especially care about this.
yes I think this will be much simpler / better after #21160 

first PR should factor out the ops to a Mixin (I may do that), then integerate.
Sounds good, closing.  If you get a chance to look at #19959 after it goes through we can add tests for IntEA\pm datetime64
@jreback wrote:

> first PR should factor out the ops to a Mixin (I may do that), then integerate.


I was going to move what I was doing with the ops in #20889 into a mixin.  Should I work on this or let you create the mixin as you said you may do above?

Ref @jorisvandenbossche discussion at https://github.com/pandas-dev/pandas/pull/20889#issuecomment-391771306
IMO you can already do this. Some of the changes to ops.py will be duplicated (but you can actually check Jeff's PR how he did it, only a small change), but the mixin being talked about here will not have this "scalar-ops-fallback", which is the point of the Mixin you want to create, I think?