BUG: regression when applying groupby aggregation on categorical columns
State: closed

- [ ] closes #31256 
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


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

Comments:
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-01-29 19:30:04 UTC
@jbrockmendel did you have thoughts on this approach?
> did you have thoughts on this approach?

ATM just confusion.  Pending the CI, I'll take @charlesdong1991 's word for it that just changing `dtype=dtype` to `dtype=dtype.name` fixes the new test, but it isn't at all clear to me why that would work.
Mmm, this fix doesn't look quite right.

The expected dtype of `SeriesGroupBy[categorical].count()` should be int64, not categorical. IIUC, this change returning a Categorical whose categories are the *counts*, which I don't think we want. It seems to me like we shouldn't be trying to cast back to an EA here. We know that count shouldn't cast back, but for arbitrary user-defined functions we don't know that.

Will think about this a bit.
I'm not really sure what to do here. There's no guarantee that `func(x).dtype == x.dtype` for user-defined functions.

We're relying on the heuristic that if `ExtensionArray._from_sequence(func(x))` works without exception, then `func(x).dtype` should be `x.dtype`. But that's not a valid assumption...
One question (to anyone, including myself): why are we calling `_try_cast` on the result of user-defined functions?  The comment about "perhaps going through object dtype" don't seem to apply to this. If the user-defined function goes through object dtype then that's on them, right?
Removing that try cast shows a handful of failures

```
pandas/tests/groupby/test_function.py::test_median_empty_bins[True] FAILED                                                                                              [  5%]
pandas/tests/groupby/test_function.py::test_ops_general[count-size] FAILED                                                                                              [ 10%]
pandas/tests/groupby/test_groupby.py::test_with_na_groups[float32] FAILED                                                                                               [ 15%]
pandas/tests/groupby/test_groupby.py::test_with_na_groups[int64] FAILED                                                                                                 [ 21%]
pandas/tests/groupby/test_groupby.py::test_with_na_groups[int32] FAILED                                                                                                 [ 26%]
pandas/tests/groupby/test_groupby.py::test_with_na_groups[int16] FAILED                                                                                                 [ 31%]
pandas/tests/groupby/test_groupby.py::test_with_na_groups[int8] FAILED                                                                                                  [ 36%]
pandas/tests/groupby/aggregate/test_aggregate.py::test_aggregate_item_by_item FAILED                                                                                    [ 42%]
pandas/tests/groupby/aggregate/test_cython.py::test_cython_agg_empty_buckets[True-mean-mean] FAILED                                                                     [ 47%]
pandas/tests/groupby/aggregate/test_cython.py::test_cython_agg_empty_buckets[True-median-<lambda>] FAILED                                                               [ 52%]
pandas/tests/groupby/aggregate/test_cython.py::test_cython_agg_empty_buckets[True-var-<lambda>] FAILED                                                                  [ 57%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[NotImplementedError] FAILED                                                                 [ 63%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[RuntimeError] FAILED                                                                        [ 68%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[KeyError] FAILED                                                                            [ 73%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[IndexError] FAILED                                                                          [ 78%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[OSError] FAILED                                                                             [ 84%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[ValueError] FAILED                                                                          [ 89%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[ArithmeticError] FAILED                                                                     [ 94%]
pandas/tests/groupby/aggregate/test_other.py::test_groupby_agg_err_catching[AttributeError] FAILED                                                                      [100%]
```

The first few look like buggy tests I think. For example, why is this a float result (behavior on 0.25.3 / master)?

```
In [15]: pd.DataFrame({"A": [1., 2.]}).groupby([1, 1]).agg(np.size)
Out[15]:
     A
1  2.0
```

Regardless, I'm straying from what can reasonable be changed for the release. What should we do short-term? Do we want to restore 0.25.3 behavior? I'm not even sure how feasible that is. It seems to be taking a very different code path compared to 0.25.3 (based on a quick skim).
> why are we calling _try_cast on the result of user-defined functions?

IIRC from refactoring a few months ago, some of the usages were about converting floats back to ints, which we do elsewhere and is fine.  The rest I think is kludges for datetimelike ops that cast to i8 before calling cython, and then _should_ case back immediately but dont.
> We're relying on the heuristic that if ExtensionArray._from_sequence(func(x)) works without exception

I've been thinking it would be useful to have either a kwarg for _from_sequence or another method that is only goes through if that would be the inferred dtype, e.g. a datetime64[ns] ndarray would work for DatetimeArray._from_sequence(arr, strict=True), but a int64 ndarray wouldnt.
> Removing that try cast shows a handful of failures

Removing only the EA part of _try_cast trims down those failures to all be on test_groupby_agg_err_catching, and all returning object dtype when we expect decimal dtype.
> The expected dtype of `SeriesGroupBy[categorical].count()` should be int64, not categorical. IIUC, this change returning a Categorical whose categories are the _counts_, which I don't think we want. It seems to me like we shouldn't be trying to cast back to an EA here. We know that count shouldn't cast back, but for arbitrary user-defined functions we don't know that.

I definitely agree on it, this should be int64, not original dtype of `value` column, and in this case, `Categorical`. At the first place, I thought should just not to cast it to EA, but i did that, and lots of tests failed, very similar to what you showed. And for some cases like count or size, the result should not be casted, and I did not find a clean way to avoid `_try_cast`.

Then reading through those failed cases, seems the dtype of agg result are aligned with dtype of that column (by casting)

> The first few look like buggy tests I think. For example, why is this a float result (behavior on 0.25.3 / master)?
> 
> ```
> In [15]: pd.DataFrame({"A": [1., 2.]}).groupby([1, 1]).agg(np.size)
> Out[15]:
>      A
> 1  2.0
> ```

and this explains why the result is a float, because the agg column A is float type, and this is casted as well by `_try_cast`. I also have doubt if this is desired behaviour or not, since ideally, like `count`, this should return an `int`. But seems to me, there is quite difficult to restore 0.25.3 behaviour, and a lot new features might be lost if doing so.  

Also in `test_decimal.py`, i think there were a couple user defined functions, and the result of agg is inferred to decimal dtype . 

> some of the usages were about converting floats back to ints, which we do elsewhere and is fine

Do you recall the details here? I don't think that `pd.DataFrame({"A": [1., 2.]}).groupby([1, 1]).agg(np.size)` returning floats is correct (which we do on 0.25.3 and master).

> have either a kwarg for _from_sequence

I think that's the original intent of `_from_sequence`, to only accept a sequence of instances of the scalar.

---

I think solving this properly will need some kind of indication from the user... There are fundamentally ambiguous cases.
I may have a potential halfway fix, that may be appropriate for 1.0.0. Will push it here in a bit. Basic idea is to only try casting back when the result is an instance of the dtype's type. Dunno how that'll affect CategoricalDtype, which doesn't really have a `.type`.

```diff
diff --cc pandas/core/groupby/groupby.py
index d93681506b,c028066e57..0000000000
--- a/pandas/core/groupby/groupby.py
+++ b/pandas/core/groupby/groupby.py
@@@ -813,13 -813,10 +813,10 @@@ b  2"""
                  # datetime64tz is handled correctly in agg_series,
                  #  so is excluded here.
  
--                # return the same type (Series) as our caller
-                 cls = dtype.construct_array_type()
-                 result = try_cast_to_ea(cls, result, dtype=dtype.name)
+                 if len(result) and isinstance(result[0], dtype.type):
+                     cls = dtype.construct_array_type()
+                     result = try_cast_to_ea(cls, result, dtype=dtype)
 +
-                 # still preserve the order for categorical
-                 if hasattr(result, "ordered"):
-                     result = result.set_ordered(dtype.ordered)
              elif numeric_only and is_numeric_dtype(dtype) or not numeric_only:
                  result = maybe_downcast_to_dtype(result, dtype)
 
```
I pushed that change here. The basic rule is we cast back to an EA when the scalars are instances of the ExtensionDtype's type.

This required changing a few tests. I think the updated tests are a bit more correct. Can people (cc @jreback @jorisvandenbossche @jbrockmendel @WillAyd) take a look and let me know if we want to proceed with this route for 1.0.0?
I’m not a huge fan of the current fix - do we know where the regression was introduced?

Sent from my iPhone

> On Jan 29, 2020, at 6:41 AM, Tom Augspurger <notifications@github.com> wrote:
> 
> ﻿
> I pushed that change here. The basic rule is we cast back to an EA when the scalars are instances of the ExtensionDtype's type.
> 
> This required changing a few tests. I think the updated tests are a bit more correct. Can people (cc @jreback @jorisvandenbossche @jbrockmendel @WillAyd) take a look and let me know if we want to proceed with this route for 1.0.0?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

I recently opened an issue that is related to this topic of "trying to cast back" and some problem with that: https://github.com/pandas-dev/pandas/issues/31108

> >  have either a kwarg for _from_sequence
>
> I think that's the original intent of _from_sequence, to only accept a sequence of instances of the scalar.

Yes, that was indeed the intent. Our own implementation of `_from_sequence` are IMO buggy because we are using them for a lot of other things as well (eg we started using it for parsing in `pd.array` as well, which is/can be less strict). They should only accept instances of its own scalar type.

> I think solving this properly will need some kind of indication from the user... There are fundamentally ambiguous cases.

Yes, that sounds as a good idea. I think long term, we should add a way to specify the expected dtype of the result (and also for the plain DataFrame apply). I think in dask something like that is already done?

> The basic rule is we cast back to an EA when the scalars are instances of the ExtensionDtype's type.

A potential problem with this is that it's not very robust / deterministic if you're only checking the first element (eg the first element could be a missing value, which is almost never an instance of the dtype's type)
> I’m not a huge fan of the current fix - do we know where the regression was introduced?

I'm not sure, but I think it's some combination of https://github.com/pandas-dev/pandas/pull/24488/files#diff-6d2a4be351f3d7ec35b9c6696a615242L771, and calling `_try_cast_result` in more places.

>  A potential problem with this is that it's not very robust / deterministic if you're only checking the first element (eg the first element could be a missing value, which is almost never an instance of the dtype's type)

Yep, good point. On balance though, I this is the best we can do for 1.0. It's slightly more complicated, but perhaps we check the first non-NA result?

I'll add an xfailing case.
There's a couple more test failures around resampling.

```
    def test_resample_integerarray():
        # GH 25580, resample on IntegerArray
        ts = pd.Series(
            range(9), index=pd.date_range("1/1/2000", periods=9, freq="T"), dtype="Int64"
        )
        result = ts.resample("3T").sum()
        expected = Series(
            [3, 12, 21],
            index=pd.date_range("1/1/2000", periods=3, freq="3T"),
            dtype="Int64",
        )
>       tm.assert_series_equal(result, expected)
E       AssertionError: Attributes of Series are different
E       
E       Attribute "dtype" are different
E       [left]:  float64
E       [right]: Int64
```

I think that `IntegerArray.resample().sum()` should be Int64 dtype, since IntegerArray._reduce('sum') is known to be an int. Fixing that is ugly, but doable.

The test also asserts that `IntegerArray.resample().mean()` is also Int64Dtype. I think that's incorrect, as `IntegerArray._reduce("mean")` is a float.
>> A potential problem with this is that it's not very robust / deterministic if you're only checking the first element (eg the first element could be a missing value, which is almost never an instance of the dtype's type)

> Yep, good point. On balance though, I this is the best we can do for 1.0. It's slightly more complicated, but perhaps we check the first non-NA result?

I wasn't able to do this for Int64 dtype, even checking just non-NA values. The presence of an NA in `result` forced object dtype, so we had a mix of NA and Python ints. Python ints aren't instances of Int64Dtype.type (numpy.int64).

I'll push an xfailing test, but for now I'd recommend accepting this limitation in the hope that most reductions don't produce NA, and we're hoping to have a more robust fix later on.
Pushed a release note. Can you take another look @jorisvandenbossche @jreback?

@WillAyd I agree that this fix isn't optimal. Do you think it's an improvement over the behavior on master though? Do you think it's OK for 1.0.0?
I’ll defer to others on this - don’t have a ton of time this week so don’t want to be a blocker.

Thanks for pushing the release

Sent from my iPhone

> On Jan 29, 2020, at 10:26 AM, Tom Augspurger <notifications@github.com> wrote:
> 
> ﻿
> Pushed a release note. Can you take another look @jorisvandenbossche @jreback?
> 
> @WillAyd I agree that this fix isn't optimal. Do you think it's an improvement over the behavior on master though? Do you think it's OK for 1.0.0?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

@TomAugspurger for the whatsnew note you are now focussing on resample. Can you explain a bit how it comes that there is such a change for resample and not for groupby? I don't see much resample specific change in the diff?
Will double check, but the behavior change from 0.25.x is just for resample. The change from master affects both. Make sense?
> he behavior change from 0.25.x is just for resample. The change from master affects both. Make sense?

Yes, that's how I understood it. I just find it strange from looking at the diff ;)
Thanks for fixing and pushing the release! @TomAugspurger 👍 

Maybe I should not disrupt you, I just saw the changes you've made and maybe it's nicer to also add `*pandas 1.0.0*` above line 670 like you did on line 651 ^^
> nicer to also add *pandas 1.0.0* above line 670 like you did on line 651 ^^

Fixed, thanks!
Good to merge when CI passes? I think this is the final thing before 1.0.
> Good to merge when CI passes?

Will take another look now
No objections to merging, agreed that some follow-up discussion is necessary on _from_sequence