DEPR: DataFrameGroupBy.apply operating on the group keys
State: closed

- [x] closes #7155 (Replace xxxx with the GitHub issue number)
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Groupby
name: Deprecate
description: Functionality to remove in pandas
name: Apply
description: Apply, Aggregate, Transform, Map

Comments:
I will look in depth in a few hours, I got some obligations. Just initially: 

Is it correct that this will help simplify the code paths for other groupby methods or is this a change that only will affect groupby `apply` itself? If yes, could you explain how the other groupby methods use/are used by `apply`? 
> Is it correct that this will help simplify the code paths for other groupby methods or is this a change that only will affect groupby `apply` itself? 

This change is only for apply itself. Ignoring filtrations (which by design include the grouping columns when applicable), almost all of groupby does not operate on the grouping columns. apply is the major exception, and this deprecation is to correct that.

> If yes, could you explain how the other groupby methods use/are used by `apply`?

Almost no groupby methods use `apply` internally. Those that do:

 - first, last
 - value_counts
 - is_monotonic_increasing, is_monotonic_decreasing
 - dtype
 - diff (for axis=1 only) 

Of the above, all except for diff are only used in the Series case where this deprecation does not apply. For diff, axis=1 has been deprecated.
So this can't affect `SeriesGroupby` at all and no `DataFrameGroupby` method uses `apply`. So that means basically no worries wrt. effect on other method, which is great.

I've looked at the code in `Groupby.apply` and you did in the `if` clause: `self._selected_obj.shape != self._obj_with_exclusions.shape`, which surprised me. So I tried:

```python
>>> df = pd.DataFrame({"A": ["a", "a", None, None, "b", "b", "c"], "B": [1, 1, 2, 2, 3, 3, 1]})
>>> g = df.groupby('A')[df.columns]
>>> g.sum()  # sums over grouping column
    A  B
A
a  aa  2
b  bb  6
c   c  1
```

I never knew this was possible (works also with `apply`), but I think this is a good thing: by default let's exclude grouping columns from `apply`/`agg` etc., but this let's users get them back in in the rare case they need that (there's always an edge  case...).

Do you know if this is documented anywhere? IMO we should officially support that pattern with docs and tests. Probably also show this as an example in the deprecation text for this change?
> Do you know if this is documented anywhere? IMO we should officially support that pattern with docs and tests. 

Added a line in the user guide; I know we test this because that's how I discovered it but I'll have to dig up where that is.

> Probably also show this as an example in the deprecation text for this change?

In the whatsnew, we could add a notable deprecations section to do this, but I'm not sure it's worth it. I'm rather against having an example in the FutureWarning that is raised.
Thanks @rhshadrach 
This is super noisy when you are fine with grouping columns not excluded in 3.0. Is there a way to make this less inconvenient apart from adjusting every groupby apply call?
The warning should be triggered only when both of the following apply:

 - The UDF with the grouping columns does not raise
 - Raising would mean the fallback passes different groups (namely, excluding the grouping columns)

The only thing I could see doing in addition is seeing (perhaps on just the first group) if the result differs whether the grouping columns are included or not. What do you think about this option?

Another option would be changing without deprecation. I'm also planning to put up a PDEP to propose a bunch of changes to agg/apply/transform that get put up behind an option for smoother transition. Perhaps this could go behind that option as well.
If I understand you here, the default case would raise a warning:

```
df = pd.DataFrame({"a": [1, 2, 3], "b": 1.5, "c": True, "d": "d"})


def test(x):
    return x


df.groupby("d").apply(test)
```

which it does right now. This seems pretty noisy, since there is no way to get rid of the warning as a user apart from changing your code everywhere?

I think an option is a good idea, if you want to add something like this anyway for other reasons.
> This seems pretty noisy

I would typically describe "noisy" as warning when behavior won't change, so just want to be clear that this will.

```
# Current
     a    b     c  d
d                   
d 0  1  1.5  True  d
  1  2  1.5  True  d
  2  3  1.5  True  d

# Future
     a    b     c
d                   
d 0  1  1.5  True
  1  2  1.5  True
  2  3  1.5  True
```

To suppress the warning and adopt future behavior, one would do `df.groupby("d")[["a", "b", "c"]].apply(test)`; to keep current behavior it's `df.groupby("d")[["a", "b", "c", "d"]].apply(test)`. Neither of these warn.

But agreed it's noisy in the sense of "someone using apply a lot of may see a lot of warnings" - and experience a lot of behavior changes!

I'll put up a PR to revert.
Yes you are correct, but I guess that most users don't care about grouping columns in apply? Values are constant anyway, so why would you need the information? That's what I mean with noisy. I am assuming that a vast majority of users just doesn't care about the grouping columns
> Values are constant anyway, so why would you need the information? That's what I mean with noisy. I am assuming that a vast majority of users just doesn't care about the grouping columns

It sounds like you're saying you'd be okay changing this behavior without deprecation?

The users may not want them, but they have them now. So if their workflow drops the grouping columns from the result, that workflow will break if we were to change this behavior without deprecation.
> I'm also planning to put up a PDEP to propose a bunch of changes to agg/apply/transform that get put up behind an option for smoother transition

I'd prefer this solution if you end up creating an option anyway. 

But I think I'd also be ok with doing this without a deprecation. Personally, I'd be annoyed that I'd have to change all my apply calls just to silence the warning even though I don't care about the grouping columns
> > I'm also planning to put up a PDEP to propose a bunch of changes to agg/apply/transform that get put up behind an option for smoother transition
> 
> I'd prefer this solution if you end up creating an option anyway.

We've moved away from introducing this option. I'm now looking for ways to introduce this deprecation again.

> But I think I'd also be ok with doing this without a deprecation. Personally, I'd be annoyed that I'd have to change all my apply calls just to silence the warning even though I don't care about the grouping columns

I imagine this is a popular method and we're changing the output in what I fear could be a common case. Doing this without deprecation seems too likely to break a lot of code. I don't fully understand your comment about "changing apply calls just to silence the warning". Wouldn't your code have a good chance of breaking if you didn't change it ahead of time when we change the behavior?


> I imagine this is a popular method and we're changing the output in what I fear could be a common case. Doing this without deprecation seems too likely to break a lot of code. I don't fully understand your comment about "changing apply calls just to silence the warning". Wouldn't your code have a good chance of breaking if you didn't change it ahead of time when we change the behavior?

Not really if you don't care about the group keys being included in your group or not. This was often the case for me when I used apply.

But I agree, not deprecating this in some way is probably a bad idea.
I believe the checks on the warning that were included here completely predict when the output would change. In other words, we have no way of making this less noisy by merely emitting warnings in less cases.

What do you think about an option to adopt the future behavior? Something like `mode.groupings_in_apply`. Defaults to `True` (current behavior). When set to `False`, it would adopt the future behavior. Then we deprecate the option in 3.x. Is that palatable?

If that's a good way forward, I think we still make the changes to the tests as they are in this PR.
Could an idea be to silence the deprecation warning until after 2.1 or maybe even 2.2?

IMO we need a deprecation here, but it may not be necessary to have it effective in 2.1, especially if silencing/unsilencing is low effort?
I'm fine with waiting to deprecate; but I think to @phofl's point, silencing is not low effort (especially when you have many columns you want to include)

And just to make sure, with the option I'm proposing, anyone who has it set to True (the default) will see the deprecation warning.
Sorry, I meant if we in the `pandas/core/groupby/groupby.py` surround the FutureWarning with `warnings.catch_warnings():` and `warnings.filter_warning` code, then remove that code when we want to re-activate the warnings e.g. in v2.2.
@topper-123: I'd personally prefer just to wait until 2.1 is released. When you wrote:

> especially if silencing/unsilencing is low effort?

I thought you meant think from a _user_ perspective. From an implementation perspective, almost all the work is with the tests. But I'm okay with having to redo a little work if the tests have changes from the implementation here by the time 2.1 is release.
Ok, sounds good to me.