BUG/TST: ensure groupby.agg preserves extension dtype
State: closed

Closes https://github.com/pandas-dev/pandas/issues/29141

@jbrockmendel this is adding the cases I found in https://github.com/pandas-dev/pandas/issues/29141. It does a small revert of catching an error to get them passing, but then your PR can fix it properly?

Labels:
name: Groupby
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
Hmm so I think I'm a little hesitant to merge this one. Didn't follow all of the back and forth but do we know a general purpose solution is?

These Exception cleanups are extremely valuable; as evidenced by this issue the amount of catching and re-wiring that goes on is impossible to reason about, so putting this back in might have subtle side effects we don't know about.
> so putting this back in might have subtle side effects we don't know about.

FWIW ive copied the test this introduces into a local branch and am figuring out what "the right" way to do this is.  #29103 is a blocker for that I'm pretty sure
> Hmm so I think I'm a little hesitant to merge this one.  Didn't follow all of the back and forth but do we know a general purpose  solution is?

Well, until three days ago, this *was* the solution; so it can't be that bad (and the "subtle side effects we don't know about" are then the ones we have had for years ..). Yes, those clean-ups are valuable, I don't disagree with that, but all I propose is to figure out the better solution while master is restored to the previous behaviour, and while we have tests in place to guarantee the behaviour (also for other open PRs).

Further, I actually think this can be a good solution. The TypeError is fully expected if a non-ndarray is passed into the fastpath. For sure, we can also check that in advance, and then don't pass it in. But both are valid python idioms.




@jorisvandenbossche i've got a local branch about ready that copy/pastes the tests from this PR and implements a longer-term fix.  does the Decimal test here sufficiently cover the Geopandas breakage you mentioned before?
@jbrockmendel OK, that sounds good. Then will merge this PR and you can remove the catched typeerror again in your upcoming PR.

> does the Decimal test here sufficiently cover the Geopandas breakage you mentioned before?

I *hope*, but it's difficult to be sure :), as it is a quite different EA. In any case, with the change I did that fixed the decimal test, also fixed my geopandas test, so they seem quite related. And I added the test with a custom decimal method to mimic it a bit more.

This wasn't approved by anyone - why merge this instead of helping @jbrockmendel ?
As I mentioned before, to have tests in place to ensure Brock's PRs pass them (there are several open PRs that touch the groupby code, not just a single one). In the discussion above, I didn't hear any objection to the tests itself, so assumed the tests itself were OK.
To come back to this a final time: Will, you are right, I shouldn't have merged before asking again given the discussion. Sorry about that. I was a bit frustrated with the discussion, but I should not have let my frustration ignore the discussion.