Fix type annotations in pandas.core.ops
State: closed

Part of #25882
- [X] tests passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

Corrects these errors in `pandas.core.ops`:
```
pandas/core/ops.py:632: error: Unsupported target for indexed assignment
pandas/core/ops.py:633: error: Value of type "object" is not indexable
pandas/core/ops.py:635: error: "object" has no attribute "copy"
pandas/core/ops.py:636: error: Unsupported target for indexed assignment
pandas/core/ops.py:637: error: Unsupported target for indexed assignment
```


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

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

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

```diff
@@            Coverage Diff             @@
##           master   #26377      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.01%     
==========================================
  Files         174      174              
  Lines       50703    50705       +2     
==========================================
- Hits        46488    46485       -3     
- Misses       4215     4220       +5
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.18% <100%> (ø)` | :arrow_up: |
| #single | `41.18% <100%> (-0.17%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26377?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.69% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.01% <0%> (-0.12%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.6% <0%> (-0.11%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.72% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26377?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/26377?src=pr&el=footer). Last update [a6e43a4...d88bdb1](https://codecov.io/gh/pandas-dev/pandas/pull/26377?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/26377?src=pr&el=h1) Report
> Merging [#26377](https://codecov.io/gh/pandas-dev/pandas/pull/26377?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/a6e43a43f2cb1b4b7d46b262be2efb825d033eb8?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #26377      +/-   ##
==========================================
- Coverage   91.68%   91.68%   -0.01%     
==========================================
  Files         174      174              
  Lines       50703    50736      +33     
==========================================
+ Hits        46488    46517      +29     
- Misses       4215     4219       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.19% <100%> (ø)` | :arrow_up: |
| #single | `41.18% <100%> (-0.17%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26377?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `94.68% <100%> (-0.01%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/compat/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9fX2luaXRfXy5weQ==) | `93.54% <0%> (-0.4%)` | :arrow_down: |
| [pandas/plotting/\_style.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19zdHlsZS5weQ==) | `76.92% <0%> (-0.26%)` | :arrow_down: |
| [pandas/plotting/\_misc.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19taXNjLnB5) | `38.23% <0%> (-0.23%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.02% <0%> (-0.12%)` | :arrow_down: |
| [pandas/core/groupby/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9nZW5lcmljLnB5) | `88.93% <0%> (-0.08%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `63.66% <0%> (-0.06%)` | :arrow_down: |
| [pandas/plotting/\_core.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb3JlLnB5) | `83.89% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `97.23% <0%> (-0.01%)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/pandas-dev/pandas/pull/26377/diff?src=pr&el=tree-more) | |

------

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

Ignore previous comment. What if instead of adding all of the casts though we did away with the `reversed` entry altogether? AFAICT it is only used in one place and could be simplified to just be `if _op_name.get('reverse')` anyway which might be more readable than alternatives and simplifies annotations for sure
Looking at it fresh today, we can get by with a single cast like this:
```py
reverse_op = cast(str, _op_descriptions[key]['reverse'])
if reverse_op is not None:
    _op_descriptions[reverse_op] = _op_descriptions[key].copy()
    _op_descriptions[reverse_op]['reversed'] = True
    _op_descriptions[reverse_op]['reverse'] = key
```

Can we live with that? It's a reasonable middle ground between my first solution and making a bunch of substantive changes (see below).

> What if instead of adding all of the casts though we did away with the `reversed` entry altogether? AFAICT it is only used in one place and could be simplified to just be `if _op_name.get('reverse')` anyway which might be more readable than alternatives and simplifies annotations for sure

It's marginally more complicated than that. In addition to being used in the code we're looking at for typing, it's used to build documentation in `_make_flex_doc`:
```py
op_name = op_name.replace('__', '')
op_desc = _op_descriptions[op_name]

if op_desc['reversed']:
    equiv = 'other ' + op_desc['op'] + ' ' + typ
else:
    equiv = typ + ' ' + op_desc['op'] + ' other'
```
This file takes the operators described in `_op_descriptions` and automatically generates "reverse" operations and docs dynamically as appropriate. E.g., multiplication is explicitly described with rmul as its reverse. The operator description for rmul is dynamically generated. When documentation is generated, the order of the operands is determined by the contents of the `'reversed'` dictionary entry.

It's probably possible to simplify it, but how far do you want to go to save a couple casts?
Thanks for digging further. So my point is that the current code looks something like this:

```python
from typing import ... cast

...

    _op_descriptions[key]['reversed'] = False
    reverse_op = cast(str, _op_descriptions[key]['reverse'])
    if reverse_op is not None:
        _op_descriptions[reverse_op] = _op_descriptions[key].copy()
        _op_descriptions[reverse_op]['reversed'] = True
        _op_descriptions[reverse_op]['reverse'] = key

...

# down in _make_flex_doc
if op_desc['reversed']:
    ...
```

AFAICT `reversed` is easily inferred from the `reverse` key, so we can reduce complexity and increase readability by simply changing the last condition in `_make_flex_doc`:

```python
...
    if reverse_op is not None:
        _op_descriptions[reverse_op] = _op_descriptions[key].copy()
        _op_descriptions[reverse_op]['reverse'] = key

...

# down in _make_flex_doc
if op_desc.get('reverse'):
    ...
```

Would definitely prefer the latter from a readability and maintenance perspective but lmk if I am missing anything. 

Not drastic in either case but would like to avoid using cast if a simple refactor of the code makes type inference possible
That approach won't work. `op_desc['reversed']` is a flag to tell the doc generator to flip the order of the operands in the documentation: i.e. for add the docs say "Equivalent to ``series + other``", for radd "Equivalent to ``other + series``". Just using `if op_desc.get('reverse'):` will flip the operands for every operator that has a reverse operator (all but the comparison ops). 

But all of the reverse operator names start with an 'r', so we could use that as the condition:
```py
if op_name.startswith('r'):
        equiv = 'other ' + op_desc['op'] + ' ' + typ
    else: 
…
```

It's unlikely there will be more operators added, so this is unlikely to cause a problem in the future.

That approach is also used in [`def _gen_eval_kwargs(name)`](https://github.com/pandas-dev/pandas/blob/d88bdb13cca56d46fde556519a1c926d96de075e/pandas/core/ops.py#L243).

Ah understood now thanks for clarifying. The alternative you have above seems fine

There's a lot of cleanup / refactor that can get done here but maybe more suitable for follow up PRs
Ha sorry my last comment was referencing your single cast idea...but I think this should do just as well. Should probably remove the large comment block now but otherwise lgtm - over to @jreback 
thanks @gwrome