-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: don't catch AttributeError in _wrap_applied_output #29195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,9 @@ def apply(self, f, data, axis=0): | |
f_name not in base.plotting_methods | ||
and hasattr(splitter, "fast_apply") | ||
and axis == 0 | ||
# with MultiIndex, apply_frame_axis0 would raise InvalidApply | ||
# TODO: can we make this check prettier? | ||
and not splitter._get_sorted_data().index._has_complex_internals | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you generally know of a reason why we have Might be easier and clearer to just do a isinstance(..., MultiIndex) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of the history there no. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool; just asking - probably a historical relic. Haven't gone too deep on this one yet but will file comments (most likely) tomorrow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _has_complex_internals predates me :-> |
||
): | ||
try: | ||
result_values, mutated = splitter.fast_apply(f, group_keys) | ||
|
@@ -207,11 +210,14 @@ def apply(self, f, data, axis=0): | |
if len(result_values) == len(group_keys): | ||
return group_keys, result_values, mutated | ||
|
||
except libreduction.InvalidApply: | ||
except libreduction.InvalidApply as err: | ||
# Cannot fast apply on MultiIndex (_has_complex_internals). | ||
# This Exception is also raised if `f` triggers an exception | ||
# but it is preferable to raise the exception in Python. | ||
pass | ||
if "Let this error raise above us" not in str(err): | ||
# TODO: can we infer anything about whether this is | ||
# worth-retrying in pure-python? | ||
raise | ||
except TypeError as err: | ||
if "Cannot convert" in str(err): | ||
# via apply_frame_axis0 if we pass a non-ndarray | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a better way to do this would be to make this if/elif into a function, then just return on the if/elif; that way you can avoid suplicating 1279 and 1287
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally agree, but I think the version on 1287 doesnt need to be there at all (not reached in tests). Just not removing it until im sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, then merge this and followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please. got a couple of local branches about ready to go