Fix type annotations in pandas.core.ops

[gwrome]

Part of #25882

• tests passed
• 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

[codecov]

Codecov Report

Merging #26377 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            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%> (ø)	⬆️
#single	41.18% <100%> (-0.17%)	⬇️

Impacted Files	Coverage Δ
pandas/core/ops.py	94.69% <100%> (ø)	⬆️
pandas/io/gbq.py	78.94% <0%> (-10.53%)	⬇️
pandas/core/frame.py	97.01% <0%> (-0.12%)	⬇️
pandas/util/testing.py	90.6% <0%> (-0.11%)	⬇️
pandas/core/indexes/base.py	96.72% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6e43a4...d88bdb1. Read the comment docs.

[codecov]

Codecov Report

Merging #26377 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            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%> (ø)	⬆️
#single	41.18% <100%> (-0.17%)	⬇️

Impacted Files	Coverage Δ
pandas/core/ops.py	94.68% <100%> (-0.01%)	⬇️
pandas/io/gbq.py	78.94% <0%> (-10.53%)	⬇️
pandas/compat/__init__.py	93.54% <0%> (-0.4%)	⬇️
pandas/plotting/_style.py	76.92% <0%> (-0.26%)	⬇️
pandas/plotting/_misc.py	38.23% <0%> (-0.23%)	⬇️
pandas/core/frame.py	97.02% <0%> (-0.12%)	⬇️
pandas/core/groupby/generic.py	88.93% <0%> (-0.08%)	⬇️
pandas/plotting/_converter.py	63.66% <0%> (-0.06%)	⬇️
pandas/plotting/_core.py	83.89% <0%> (-0.02%)	⬇️
pandas/core/groupby/groupby.py	97.23% <0%> (-0.01%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6e43a4...a1f29fd. Read the comment docs.

[WillAyd]

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

[gwrome]

Looking at it fresh today, we can get by with a single cast like this:

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:

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?

[WillAyd]

Thanks for digging further. So my point is that the current code looks something like this:

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:

...
    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

[gwrome]

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:

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).

[WillAyd]

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

[WillAyd]

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

[jreback]

thanks @gwrome