PERF: operate on arrays instead of Series in DataFrame/DataFrame ops #33561
Conversation
|
While you're doing this, want to get the other |
|
Yes, will do. (note, I would also be fine with you including this in #33561, if we decide to have a switch between column/blockwise. But it's mainly because you seemed quite hesitant to include something like this, that I thought to do this PR separately) |
pandas/core/ops/__init__.py
Outdated
|
|
||
| else: | ||
| # Remaining cases have less-obvious dispatch rules | ||
| raise NotImplementedError(right) | ||
|
|
||
| new_data = expressions.evaluate(column_op, str_rep, left, right) |
There was a problem hiding this comment.
does this have a perf impact? IIRC a while back I tried removing it because I thought it shouldn't, but found that it did
There was a problem hiding this comment.
As far as I understand, the get_array_op should take care of this?
For example, the arrays_ops.py::na_arithmetic_op also calls expressions.evaluate to perform the actual op.
There was a problem hiding this comment.
That was what i thought too, and I could be remembering incorrectly. But this is worth double-checking.
|
|
||
| def column_op(a, b): | ||
| return {i: func(a.iloc[:, i], b.iloc[i]) for i in range(len(a.columns))} |
There was a problem hiding this comment.
doesnt need to be in this PR, but since you're attention is already focused on this line: this can be incorrect if b is e.g. Categorical, since b.iloc[i] will have the scalar behavior instead of the Categorical (usually raising) behavior. (This is also a place where NaT causes issues, and the idea of only-one-NA causes me nightmares)
The main idea I've had here is to use b.iloc[[i]] instead of b.iloc[i], haven't gotten around to implementing it. Does this change make it easier to address the issue?
There was a problem hiding this comment.
when using something like b.iloc[[i]], you would need to handle broadcasting here as well (since the array ops expect either same length array or scalar). So since that is a pre-existing issue, I would leave that for a dedicated PR.
Is there an issue for this?
There was a problem hiding this comment.
I dont think there's an issue for this. If I had found one, i would have added the Numeric label
|
can you point to some asv's that change with this? (nice cleanup anyhow) |
There is already a Running that benchmark specifically, on master: and with this branch: The comparison ops take the code path I changed here, and those show a consistent speed-up (the arithmetic ones actually take a different code path) |
|
|
||
| else: | ||
| # Remaining cases have less-obvious dispatch rules | ||
| raise NotImplementedError(right) | ||
|
|
||
| new_data = expressions.evaluate(column_op, str_rep, left, right) | ||
| return new_data | ||
| return type(left)._from_arrays( |
There was a problem hiding this comment.
i think you can just return arrays here, since that will be passed to left._construct_result (though the docstring does say DataFrame, i guess thats wrong ATM)
There was a problem hiding this comment.
I want to explicitly use _from_arrays, as that is part of the reason that makes this PR a perf improvement (the _from_arrays constructor specifically handles the case of a list of arrays corresponding to columns). Also, the default DataFrame constructor handles a list of arrays differently.
xref #32779