WIP: decorator for ops boilerplate
State: closed

- [x] closes #23583
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

There are a handful of places where this decorator is applied but commented-out.  This is the "WIP" part of things.

Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Clean

Comments:
Hello @jbrockmendel! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/categorical.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/categorical.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimelike.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/datetimelike.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/integer.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/integer.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/period.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/period.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/sparse.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/sparse.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/timedeltas.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/arrays/timedeltas.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/base.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/indexes/base.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/range.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/indexes/range.py) !

 - There are no PEP8 issues in the file [`pandas/core/ops.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/core/ops.py) !

 - There are no PEP8 issues in the file [`pandas/tests/arithmetic/test_timedelta64.py`](https://github.com/jbrockmendel/pandas/blob/2f929af6611f6615fcc94027dbc25b7199d5627b/pandas/tests/arithmetic/test_timedelta64.py) !


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

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

```diff
@@            Coverage Diff             @@
##           master   #24282      +/-   ##
==========================================
+ Coverage   92.22%   92.23%   +<.01%     
==========================================
  Files         162      162              
  Lines       51828    51785      -43     
==========================================
- Hits        47798    47763      -35     
+ Misses       4030     4022       -8
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.63% <88.67%> (ø)` | :arrow_up: |
| #single | `43.06% <58.49%> (+0.06%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24282?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `96.44% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `98.5% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.23% <ø> (-0.01%)` | :arrow_down: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.31% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/integer.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2ludGVnZXIucHk=) | `95.78% <100%> (+0.24%)` | :arrow_up: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.03% <100%> (-0.06%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.16% <100%> (-0.12%)` | :arrow_down: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `97.29% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `87.65% <80.95%> (+0.49%)` | :arrow_up: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.51% <90.9%> (+0.25%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree-more) | |

------

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

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24282?src=pr&el=h1) Report
> Merging [#24282](https://codecov.io/gh/pandas-dev/pandas/pull/24282?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/7b0fa8e94e22aa4a491fa574bc56f8ca791f7608?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `95.91%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24282      +/-   ##
==========================================
+ Coverage   92.22%   92.23%   +0.01%     
==========================================
  Files         162      162              
  Lines       51824    51774      -50     
==========================================
- Hits        47795    47756      -39     
+ Misses       4029     4018      -11
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.64% <95.91%> (+0.01%)` | :arrow_up: |
| #single | `43.07% <71.42%> (+0.06%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24282?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `96.44% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `98.48% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.23% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `88.41% <100%> (+1.25%)` | :arrow_up: |
| [pandas/core/arrays/integer.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2ludGVnZXIucHk=) | `95.78% <100%> (+0.24%)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.3% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.03% <100%> (-0.06%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.16% <100%> (-0.12%)` | :arrow_down: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `97.29% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.51% <90.9%> (+0.25%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/pandas-dev/pandas/pull/24282/diff?src=pr&el=tree-more) | |

------

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

After a bunch more poking at this, I think we should hold off on applying this decorator for the methods where it would change behavior, in particular for Categorical and PeriodArray comparisons.  Implementing tests for these would be an afterthought, runs the risk of being half-baked.
@jreback thoughts here?  I'm increasingly leaning towards being even stricter, reverting usages that change behavior (basically everything outside of timedeltas) and incrementally re-implementing those in follow-ups with appropriate tests (i.e. things like ops with zero-dim arrays)
> @jreback thoughts here? I'm increasingly leaning towards being even stricter, reverting usages that change behavior (basically everything outside of timedeltas) and incrementally re-implementing those in follow-ups with appropriate tests (i.e. things like ops with zero-dim arrays)

not sure exactly what you mean here. with the decorator?
> not sure exactly what you mean here. with the decorator?

The decorator is in fairly good shape, the question is where we want to actually use the decorator.  AFAICT every place the decorator is used ATM implies some non-zero change to the behavior of the affected function.  For e.g. `TimedeltaArray.__mul__`, div, etc the change is really small, just handles iterators more precisely.  For Categorical comparisons on the other hand, the change is pretty big (with accompanying tests).

But pretty much all the cases in between make not-quite-trivial changes to the methods' behaviors with no accompanying tests.  The decision is where to draw then line on those, and my current thought is to be relatively strict for now.

> But pretty much all the cases in between make not-quite-trivial changes to the methods' behaviors with no accompanying tests. The decision is where to draw then line on those, and my current thought is to be relatively strict for now.

can you highlite this change via an example(s)?
> can you highlite this change via an example(s)?

IntNA
```
arr = np.arange(5)
arr_na = pd.core.arrays.integer_array(arr)

other = (x for x in arr_na)
result = arr_na == other

# PR
>>> result
array([ True,  True,  True,  True,  True])

# master
>>> result
array([False, False, False, False, False])
```

Sparse
```

arr = np.arange(5)
df = pd.DataFrame(arr)
arr_sparse = pd.core.arrays.SparseArray(arr)

# PR
>>> arr_sparse == np.array(4)
[False, False, False, False, True]
Fill: False
IntIndex
Indices: array([1, 2, 3, 4], dtype=int32)

# master
>>> arr_sparse == np.array(4)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pandas/core/arrays/sparse.py", line 1712, in cmp_method
    if len(self) != len(other):
TypeError: len() of unsized object

# PR
>>> arr_sparse + df.T
   0  1  2  3  4
0  0  2  4  6  8

# master
>>> arr_sparse + df.T
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pandas/core/arrays/sparse.py", line 1684, in sparse_arithmetic_method
    self=len(self), other=len(other))))
AssertionError: length mismatch: 5 vs. 1
```

Also, thoughts on collecting categorical and sparse arithmetic/comparison tests in tests.arithmetic?  I need to take a look at how to improve coverage for these without combinatorial explosion.
When comparing with a generator, we want the behavior on master, right? That matches NumPy's behavior anyway.
> Also, thoughts on collecting categorical and sparse arithmetic/comparison tests in tests.arithmetic?

As an alternative, you could add an `arithmetic` pytest marker to the classes those tests are defined on. A dev might want "run all the sparse tests" or "run all the arithmetic tests". I'd favor just leaving them as is for now out of status quo bias.
> As an alternative, you could add an arithmetic pytest marker to the classes those tests are defined on. 

That's not a bad idea.  Regardless, this can wait until the next test-parametrization pass.
> When comparing with a generator, we want the behavior on master, right? That matches NumPy's behavior anyway.

If that's the case, then we probably need to revert that part of the decorator anyway right?

It isn't obvious to me that is the desired behavior.  Is there a reason why would do unroll generators for arithmetic ops but not for comparisons?
I'm not sure either. You could ask on the numpy mailing list to see if that
behavior is deliberate. I know that NumPy 1.16 is deprecating passing
generators to things like `np.concatenate`.

On Wed, Dec 19, 2018 at 5:01 PM jbrockmendel <notifications@github.com>
wrote:

> When comparing with a generator, we want the behavior on master, right?
> That matches NumPy's behavior anyway.
>
> If that's the case, then we probably need to revert that part of the
> decorator anyway right?
>
> It isn't obvious to me that is the desired behavior. Is there a reason why
> would do unroll generators for arithmetic ops but not for comparisons?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24282#issuecomment-448777501>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIs1MQjNWQ4UI55IktcmueKsaFW6bks5u6sVOgaJpZM4ZUHSY>
> .
>

> You could ask on the numpy mailing list to see if that
behavior is deliberate.

OK.  It seems like this PR is going to be in a holding pattern for a while.  I'll make a separate PR to implement the changes+tests for Categorical if you're OK with those.
@TomAugspurger are you on board with the changes to Categorical comparison behavior?  (easiest to look at the new test)
IIUC the change in https://github.com/pandas-dev/pandas/pull/24282/files#diff-c859b3060cdd05f4d0f693b0a4b71fc5R130 is to raise on `cat == df`, making the behavior consistent with how numpy? If so, then +1 
I still think this is worth doing, but I'm not comfortable with introducing lots of small changes in behavior without accompanying tests.

Closing now, will try a new approach after the RC.