CLN: Catch more specific exception in groupby.ops #28198
Conversation
| # raise this error to the caller | ||
| pass | ||
| except TypeError as err: | ||
| if "Cannot convert" in str(err): |
There was a problem hiding this comment.
Is the type(s) of exceptions that raise this not well defined?
There was a problem hiding this comment.
cython raises this (with a message matching this pattern) if we pass a non-ndarray to something expected an ndarray. im not aware of any other cases we actually want to let pass here
There was a problem hiding this comment.
I assume you saw other non-Cython TypeErrors show up here then?
There was a problem hiding this comment.
if we dont catch TypeError at all, the only tests that fail are ones where apply_frame_axis0 is raising because it expects an ndarray
There was a problem hiding this comment.
I may have asked this wrong but I don't think we are on the same page. So I was thinking to keep catching TypeError but was questioning if we need the conditional block therein, as it diverges slightly from the pre-existing behavior.
There was a problem hiding this comment.
if we need the conditional block therein, as it diverges slightly from the pre-existing behavior.
Changing the behavior is intentional. ATM we are passing on everything, and I want to pass on a narrow set of TypeErrors.
| except TypeError as err: | ||
| if "Cannot convert" in str(err): | ||
| # via apply_frame_axis0 if we pass a non-ndarray | ||
| pass |
There was a problem hiding this comment.
looks fine, does this change any perf? IOW this looks like this is now taking a path that previously we raised (and then likely did an .apply on)
There was a problem hiding this comment.
Not sure. Between this, #27909, and a not-yet-pushed branch that fixes incorrect exception handling in cython_agg_block, I'm pretty sure we'll end up falling back to python-space less often, but it isn't obvious what the individual changes affect perf-wise.
|
lgtm. can you rebase as I think we merged some other groupby/ops stuff. (just to be sure). |
Lots of these to whittle down.