REF: avoid catching all exceptions in libreduction #38285
Conversation
The relevant question isn't "will it ever raise" but "will it raise in the cython path but not in the python path". AFAICT IndexError is the only non-contrived way that happens |
|
do you have objections @jorisvandenbossche |
OK, I see, it's only removing the error catching specifically from calling the function on the chunk (which was re-raised as InvalidApply), not general errors from libreduction code |
pandas/core/groupby/ops.py
Outdated
| # worth-retrying in pure-python? | ||
| raise | ||
| except IndexError: | ||
| # test_apply_mutate this is a rare case in which re-running |
There was a problem hiding this comment.
which test is this referring to? (I don't see a test_apply_mutate)
There was a problem hiding this comment.
test_apply_mutate.py, will update to make clearer
There was a problem hiding this comment.
@jbrockmendel is there something you were going to update here?
There was a problem hiding this comment.
thanks for the reminder, just updated
| # the function modified them. | ||
| mgr = self.dummy._mgr | ||
| self.orig_blklocs = mgr.blklocs | ||
| self.orig_blknos = mgr.blknos |
There was a problem hiding this comment.
Is this fixing a specific bug? (but don't see any test added)
There was a problem hiding this comment.
without this we have other tests that fail in cython but not in python, but im not aware of any bugs this causes in master
There was a problem hiding this comment.
You mean that on master the try: piece = f(chunk) ... would fail, and thus be elevated to the python level (where it worked), but with this fix ensures those specific cases don't fail anymore an work on the cython level?
Can you give an example of such a test?
Also, the issues number in the comment seems not directly related (well, it might be it was needed there as well, but since that's an open PR, that's a bit confusing reference)
There was a problem hiding this comment.
Can you give an example of such a test?
FAILED pandas/tests/groupby/test_apply_mutate.py::test_mutate_groups - ValueError: shape mismatch: value array of shape (1,6) could not be broa...
Will attempt to clarify .
|
this looks fine. @jorisvandenbossche any comments? |
Most of the libreduction edits are from #35417