CLN: always dispatch-to-series #34286
Conversation
| @@ -624,7 +629,7 @@ def to_series(right): | |||
|
|
|||
|
|
|||
| def _should_reindex_frame_op( | |||
| left: "DataFrame", right, op, axis, default_axis: int, fill_value, level | |||
| left: "DataFrame", right, op, axis, default_axis, fill_value, level | |||
There was a problem hiding this comment.
without this i got mypy complaints that i havent been able to track down
There was a problem hiding this comment.
Can you share the error message?
There was a problem hiding this comment.
pandas/core/ops/__init__.py:706: error: Argument 5 to "_should_reindex_frame_op" has incompatible type "Optional[str]"; expected "int"
There was a problem hiding this comment.
Gotcha. Looks like _get_frame_op_default_axis returns an Optional string and that is used as the default argument in the f function local to _arith_method_FRAME.
So makes sense Mypy would complain about that. Whether the existing annotation is wrong or if its discovering a bug for us I'm not sure
There was a problem hiding this comment.
i think its the annotation. there is some more code that can be ripped out after this, so im planning to do an annotation pass towards the end
pandas/core/ops/__init__.py
Outdated
| assert left.index.equals(right.index) | ||
| assert left.columns.equals(right.columns) | ||
| # TODO: The previous assertion `assert right._indexed_same(left)` | ||
| # fails in cases with shaoe[1] == 0 |
There was a problem hiding this comment.
I don't really understand this comment
There was a problem hiding this comment.
im finding cases where left.columns and right.columns are both empty but different dtypes, which still satisfy left.columns.equals(right.columns), but not right._indexed_same(left). looks like this is an artifact of the intersection done in _should_reindex_frame_op, looking into this now. (also will fix the typo shaoe -> shape)
|
|
||
| elif isinstance(other, ABCSeries): | ||
| # For these values of `axis`, we end up dispatching to Series op, | ||
| # so do not want the masked op. | ||
| # TODO: the above comment is no longer accurate since we now | ||
| # operate blockwise if other._values is an ndarray |
There was a problem hiding this comment.
Can you update the comment instead?
There was a problem hiding this comment.
simpler to do this in the next pass, which will make us always pass the same thing, at which point we can remove the comment entirely
| """ | ||
| if isinstance(op, partial): | ||
| # We get here via dispatch_to_series in DataFrame case | ||
| # TODO: avoid getting here |
There was a problem hiding this comment.
What is an example with which you get this?
There was a problem hiding this comment.
In _arith_method_FRAME we pass na_op (which is one a partial returned by this function here) to _combine_frame, which in turn passes na_op to dispatch_to_series, which then passes na_op back to this function
| @@ -680,11 +685,12 @@ def _frame_arith_method_with_reindex( | |||
|
|
|||
|
|
|||
| def _arith_method_FRAME(cls, op, special): | |||
| # This is the only function where `special` can be either True or False | |||
jbrockmendel
added
the
Numeric Operations
|
ahh need to rebase |
|
kk ping when ready in one |
|
pseudo-green. the remaining CI fail is anoteher ipython directive thing in the docbuild |
|
thanks if u wouldn’t mind pushing a change for the doc thing |
A couple more cleanups follow this.