REF: _cython_agg_blocks #31752
REF: _cython_agg_blocks #31752
Conversation
| @@ -1063,58 +1087,32 @@ def _cython_agg_blocks( | |||
| deleted_items.append(locs) | |||
| continue | |||
| else: | |||
| if isinstance(result, Series): | |||
There was a problem hiding this comment.
we have test cases that get here with a Series and without this the cast just below here is inaccurate
There was a problem hiding this comment.
Hmm that's unfortunate. Don't want to go too far down the typing road on this one but maybe should be using assert ... instead of cast
There was a problem hiding this comment.
hmm, reveal_type(result) entering this block is object. probably need to take control of the type of result before this. does adding type annotations to _recast_result help?
There was a problem hiding this comment.
it looks like since aggregate in SelectionMixin does not have type annotations, the return type of s.aggregate(lambda x: alt(x, axis=self.axis)) is Any and hence as result is object no type narrowing occurs. reveal_type(s) is 'pandas.core.groupby.groupby.GroupBy' so it's just aggregate that needs the annotations.
| except ValueError: | ||
| # reshape to be valid for non-Extension Block | ||
| result = result.reshape(1, -1) | ||
| for i, col in enumerate(result.columns): |
There was a problem hiding this comment.
Perhaps misinterpreting but does this not have a huge performance penalty? IIUC tried column iteration back in #28782 but performance took a pretty big hit in doing so
There was a problem hiding this comment.
the actual aggregation isn't being done columnwise, just re-assembling new blocks
| split_items: List[np.ndarray] = [] | ||
| split_frames: List[DataFrame] = [] | ||
|
|
||
| def _recast_result(result, values): |
There was a problem hiding this comment.
can you move this somewhere else, pandas.core.dtypes.cast maybe
|
closing to clear the queue |
jbrockmendel
added
the
Mothballed
jbrockmendel
removed
the
Mothballed
This is effectively an alternative approach to #31616, trying to use a 1-pass approach.
This does not address the uniqueness problem discussed in #31735.
cc @TomAugspurger @WillAyd