BUG: incorrect EA casting in groubpy.agg
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

The existing code is implicitly assuming that _from_sequence is strict, so that maybe_cast_to_extension_array will only return an EA when correct.  Since that assumption is untrue, the current code will return incorrect results, as in the test this adds.

If we just removed L729-L741 in groupby.ops, we would have 12ish test failures.  A few of those would be for float64 result failing to cast back to Float64, and the rest would be for `ndarray[Decimal objects]` failing to cast back to DecimalArray.  Until `_from_sequence` is reliable, I would rather remove these few lines and return correct-but-suboptimally-casted results than have these kludges.

cc @jorisvandenbossche 

Labels:
name: Groupby
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
updated to remove the incorrect casting without special-casing pandas-internal EAs
@jbrockmendel Can you show an example of what currently gives a wrong output? 
(looking at the diff it seems something with Period? )
ok code / tests look ok, can you add a whatsnew note in the EA section (or groupby)
> Can you show an example of what currently gives a wrong output?
(looking at the diff it seems something with Period? )

```
dti = pd.date_range("2012-01-01", periods=4)
pi = dti.to_period("D")

df = DataFrame({"a": [0, 0, 1, 1], "b": pi})
gb = df.groupby("a")

>>> gb["b"].agg(lambda x: x.iloc[0].year)
a
0    2012-01-01
1    2012-01-01
Name: b, dtype: period[D]
```
I would personally focus on fixing the actual cause (stricter way to cast back), instead of doing this "fix" (because it's fixing period, but at a cost of causing regressions for other dtypes)
> (because it's fixing period, but at a cost of causing regressions for other dtypes)

how much of your opinion is based on IntegerArray/FloatingArray/BooleanArray?  We could special-case those similar to what we do in _ea_wrap_cython_operation.

For general EAs (and DecimalArray) I think "sometimes giving completely incorrect results" is a much bigger problem than "sometimes giving correct results in a sub-optimal container"
> how much of your opinion is based on IntegerArray/FloatingArray/BooleanArray? 

Yes, quite a bit ;). Also based on geopandas, but our `_from_sequence` is already pretty strict there anyway, so we don't really have this problem.

Now, I don't think this is a critical bug fix for 1.2, so assuming it's for 1.3 (I think the rc will be cut any time now), that gives us more time to fix this, in which case I would prefer to focus on getting https://github.com/pandas-dev/pandas/pull/38315 done.  
(if we do that, will move your period test there to ensure that is fixed)

We could also still merge this, and revert most of the test changes in https://github.com/pandas-dev/pandas/pull/38315, but not sure what buys us that (apart from a bigger diff in https://github.com/pandas-dev/pandas/pull/38315)
> Yes, quite a bit ;).

I'd be OK with re-implementing part of an earlier commit that special-cases those dtypes.  That would fix the Period-like bug and only change the Decimal-like cases, which are relatively straightforward conceptually.  Would you be OK with that?
> That would fix the Period-like bug and only change the Decimal-like cases, which are relatively straightforward conceptually. Would you be OK with that?

I still don't fully understand what would buy us that, since #38315 is also fixing the Period-like bug, but without regressing on the decimal case. So I would just need to revert most of this PR in #38315 (except the period test case, which I can add there), making the diff there larger. 
Your input on the different questions I laid out in #38315 is very welcome, to ensure we can move it forward.


> since #38315 is also fixing the Period-like bug, but without regressing on the decimal case

#38315 is very ambitious and potentially addressing multiple intertwined issues.  I'm not ready to consider it a "solved problem".


>  I'm not ready to consider it a "solved problem".

But this PR is also not "solving the problem" .. 
It's IMO rather complicating the situation by incorrecting part of the tests. I am fine if the PR would just focus on fixing the Period case though, then it already fixes this specific case and adds a test, making #38315 actually easier instead of more complex.
I think updating `maybe_cast_result` to special case periods as well is actually a nice short term solution to get this fix in (we already do that for other dtypes as well, so while it is not the nicest thing to do (but one of the goals of #38315 will be to clean that up), it's at least consistent ..).