Skip to content

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

Merged
merged 2 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions pandas/core/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,6 @@ def _aggregate_multiple_funcs(self, arg, _level, _axis):

except (TypeError, DataError):
pass
except SpecificationError:
raise
else:
results.append(new_res)

Expand All @@ -591,8 +589,6 @@ def _aggregate_multiple_funcs(self, arg, _level, _axis):
except ValueError:
# cannot aggregate
continue
except SpecificationError:
raise
else:
results.append(new_res)
keys.append(col)
Expand Down
19 changes: 14 additions & 5 deletions pandas/core/groupby/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ def _transform_fast(self, func, func_nm):
out = self._try_cast(out, self.obj)
return Series(out, index=self.obj.index, name=self.obj.name)

def filter(self, func, dropna=True, *args, **kwargs): # noqa
def filter(self, func, dropna=True, *args, **kwargs):
"""
Return a copy of a Series excluding elements from groups that
do not satisfy the boolean criterion specified by func.
Expand Down Expand Up @@ -1230,7 +1230,7 @@ def first_not_none(values):
return self._concat_objects(keys, values, not_indexed_same=True)

try:
if self.axis == 0:
if self.axis == 0 and isinstance(v, ABCSeries):
# GH6124 if the list of Series have a consistent name,
# then propagate that name to the result.
index = v.index.copy()
Expand Down Expand Up @@ -1266,15 +1266,24 @@ def first_not_none(values):
axis=self.axis,
).unstack()
result.columns = index
else:
elif isinstance(v, ABCSeries):
stacked_values = np.vstack([np.asarray(v) for v in values])
result = DataFrame(
stacked_values.T, index=v.index, columns=key_index
)
else:
# GH#1738: values is list of arrays of unequal lengths
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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

# fall through to the outer else clause
# TODO: sure this is right? we used to do this
# after raising AttributeError above
return Series(
values, index=key_index, name=self._selection_name
)

except (ValueError, AttributeError):
except ValueError:
# TODO: not reached in tests; is this still needed?
# GH1738: values is list of arrays of unequal lengths fall
# through to the outer else caluse
# through to the outer else clause
return Series(values, index=key_index, name=self._selection_name)

# if we have date/time like in the original, then coerce dates
Expand Down
10 changes: 8 additions & 2 deletions pandas/core/groupby/ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you generally know of a reason why we have _has_complex_internals as a Index property? I think only changed for a MultiIndex and only applicable in groupby space.

Might be easier and clearer to just do a isinstance(..., MultiIndex)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of the history there no.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand All @@ -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
Expand Down