REF: Pre-empt ValueError in _aggregate_series_fast #29500
Conversation
gfyoung
added
Error Reporting
| @@ -583,7 +583,11 @@ def _transform( | |||
| return result | |||
|
|
|||
| def agg_series(self, obj: Series, func): | |||
| if is_extension_array_dtype(obj.dtype) and obj.dtype.kind != "M": | |||
| if len(obj) == 0: | |||
| # SeriesGrouper would raise if we were to call _aggregate_series_fast | |||
There was a problem hiding this comment.
Hmm should we not just fix _aggregate_series_fast to not raise in that case then? Hoping to avoid special casing like this in any groupby functions
There was a problem hiding this comment.
Hoping to avoid special casing like this in any groupby functions
Agreed on the goal. ATM this is the best way to make progress towards that goal. In particular, making this explicit here is much clearer than catching the ValueError with the particular message on L600.
should we not just fix _aggregate_series_fast to not raise in that case then?
#29499 does something along those lines.
| dummy = obj[:0] | ||
| labels = np.array([-1, -1, -1, 0, 0, 0, 1, 1, 1, 1], dtype=np.int64) | ||
|
|
||
| with pytest.raises(ValueError, match="SeriesGrouper requires non-empty `series`"): |
There was a problem hiding this comment.
Is this visible from an end user perspective?
There was a problem hiding this comment.
this should never be raised to the user, just seemed like the behavior change merited a test.
|
lgtm. can you merge master here and merge on green. |
|
rebased+green; ping. |
|
thanks @jbrockmendel |
cc @WillAyd @jreback orthogonal to other groupby PRs (including #29499) but will have merge conflicts. No preference on what order they should go in.