Fix GH-29442 DataFrame.groupby doesn't preserve _metadata
State: closed

This bug is a regression in v1.1.0 and was introduced by the fix for GH-34214 in commit [6f065b].

Underlying cause is that the `*Splitter` classes do not use the `._constructor` property and do not call `__finalize__`.

Please note that the method name used for `__finalize__` calls was my best guess since documentation for the value has been hard to find.

[6f065b]: https://github.com/pandas-dev/pandas/commit/6f065b6d423ea211d803e8be93c27f547541c372

- [x] closes #29442
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Groupby
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
Hello @Japanuspus! 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-10-09 12:42:14 UTC
@jbrockmendel This PR is a two-line edit to fix a metadata regression in your recent work on groupby-performance. Would you be available to review? 

I am available to implement this is differently if needed.
cc @TomAugspurger since this is about `__finalize__` being called.  I'm ambivalent, since this is Technically Correct, but also a place where we're really trying to avoid overhead since these methods get called in a loop.

At a glance, this feels like too low of a level to call `__finalize__`. Why can't we call it on the outer level, in whatever function is defining `DataFrameGroupBy.sum()`?
Thank you both for looking into this! I will start with a more concise testcase.

Should I also have a go at moving the `__finalize__` call to an outer layer, or are either of you looking into that in detail?
Regarding performance implications of `__finalize__` in `_chop`

I tried running the benchmark from [GH-34214 ](https://github.com/pandas-dev/pandas/pull/34214), but this seems to hit `.fast_apply` and not `._chop`.

Can either of you come up with a benchmark that hits `._chop` in a hot loop where calling `__finalize__` on the groups would really be wasted?

(`fast_apply` benchmark for reference)
```
import numpy as np
from pandas import DataFrame

N = 10 ** 4
labels = np.random.randint(0, 2000, size=N)
labels2 = np.random.randint(0, 3, size=N)
df = DataFrame(
    {
        "key": labels,
        "key2": labels2,
        "value1": np.random.randn(N),
        "value2": ["foo", "bar", "baz", "qux"] * (N // 4),
    }
)

%timeit df.groupby(["key", "key2"]).apply(lambda x: 1)
%timeit df.groupby("key").apply(lambda x: 1)
```


> Can either of you come up with a benchmark that hits ._chop in a hot loop where calling __finalize__ on the groups would really be wasted?

Try setting `df.index = pd.CategoricalIndex(df.index)`
Just running `df.index = pd.CategoricalIndex(df.index)` for the `df` produced in the `fast_apply` benchmark does not hit `_.chop`. But I have a feeling that was maybe not what you meant? 
Perhaps run the groupby with a CategoricalIndex?
Thanks for helping out! Managed to get a benchmark with some degradation:

With `df` as above
```
df.index = pd.CategoricalIndex(df.key)
%timeit df.groupby(level='key').apply(lambda x: 1)
```

Result was a 4% performance hit:
- This patch: 85.3 ms ± 570 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
- 1.1.x from source: 81.9 ms ± 1.48 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

I will not have time to learn enough of the inner workings of pandas to improve this on my own, but am willing to help out if you have some concrete pointers.  
Alternatively you could accept as-is, and then have a look at performance once tests are in place? Or just drop this (I can copy-paste the testcase to the issue thread) -- for my own use case I have implemented a fallback to handle initialization without `__finalize__`, so all is good for me.
I guess one way to get a sense for the performance impact is to count how many times `__finalize__` is called. I think ideally it's called exactly once, rather than once per group.
Good point @TomAugspurger.

With respect to my first commit, adding `__finalize__` in `DataSplitter._chop`, it seems this could be improved for the code paths tested here by moving `__finalize__` out to `_GroupBy.__iter__` and `_GroupBy.apply`. Should I make an attempt to do so, or would you rather take another approach? 

With respect to the `sum`-test, I could not see how to move the call  further out than `_agg_general` without having to duplicate it across all the aggregation methods? https://github.com/pandas-dev/pandas/pull/35688/commits/8d3d896bcb20f7bcb0d3ac2b71098dfb05fff11e

Also seems I have a bunch of failing tests, so will take a look at that. Please let me know if you would rather take this in another direction.

@simonjayhawkins Yes, I should probably add a release note -- but there was a bit of scope creep (my original PR addressed my own problem, not the issue listed), and that work broke something. Also, I have come around to agreeing that we need a more performant version of my initial fix.

I will be short on time for a week or so, but then I will have a go at this.
Just a note (mostly to self) on the failing tests since it took me an embarrassing amount of time to identify them.

The relevant tests are in `tests/generic/test_finalize.py` and are marked [`xfail`](https://docs.pytest.org/en/reorganize-docs/new-docs/user/xfail.html), which means that when running tests in strict mode, it will be reported as an error if they pass.
Since they include a test for aggregation by sum, they now pass which caused the test to be red in strict mode.

I will remove the `xfail` flag from the relevant cases, and then proceed to look at the performance as discussed above.


> The relevant tests are in tests/generic/test_finalize.py and are marked xfail

Tracking down xfailed tests that can be fixed is really helpful, thanks.
This got sort of messy, and I think a summary is in place:

My initial PR did not address the linked issue and would cause performance regressions. When this was keenly pointed, I added a test and fix for the original issue.

After an unfortunate pause I continued to identify and disable x-failing tests caused by the fix, and also added a test for performance, counting the number of `__finalize__` calls in a `groupby().apply()` invocation, which shares relevant code-path with `.sum()`.

Finally, I have reverted the calls to `__finalize__` from my original PR since this is really an unrelated issue. I have left the test (with an `xfail`-tag), but will file a separate issue for this.
> 
> this looks fine; i doubt performance is actually an issue here, can you show a benchmark otherwise?

For the PR as it stands, I don't think performance can be improved.

For the, unrelated and now removed, change in `DataSplitter`  (7cc4d5375e57277f316c5a8bb17a58ab8003c5f1) there is a benchmark in the comments above where we saw a 4% penalty. The alternative solution in 0af015ba77d8ff05f0d38a3d4b0518bbc737d393 only performs the `__finalize__` after aggregation and has no performance issues, but is of course less solid in the sense that there could be code-paths with multiple calls to `__finalize__`. This is the part I propose to move to another PR.

The applied benchmark for reference: 
```
import numpy as np
import pandas as pd

N = 10 ** 4
labels = np.random.randint(0, 2000, size=N)
labels2 = np.random.randint(0, 3, size=N)
df = pd.DataFrame(
    {
        "key": labels,
        "key2": labels2,
        "value1": np.random.randn(N),
        "value2": ["foo", "bar", "baz", "qux"] * (N // 4),
    }
)
df.index = pd.CategoricalIndex(df.key)
%timeit df.groupby(level='key').apply(lambda x: 1)

```

> 
> 
> does this need a release note?

Added a release note for 1.1.4
@Japanuspus pls limit changes to one thing in a PR, esp as this is a backport.
I am slightly confused about @jreback mentioning this being a backport PR -- my intention was to target master. Or does this just mean that the patch will both apply to master and release branches?
> I am slightly confused about @jreback mentioning this being a backport PR -- my intention was to target master. Or does this just mean that the patch will both apply to master and release branches?

this si a regresion, so we target master, then its backported. so pls limit the change to the regression. if you want to do another change that targets master only that's ok, but separate PR
> > I am slightly confused about @jreback mentioning this being a backport PR -- my intention was to target master. Or does this just mean that the patch will both apply to master and release branches?
> 
> this si a regresion, so we target master, then its backported. so pls limit the change to the regression. if you want to do another change that targets master only that's ok, but separate PR

Thank you for the explanation.
I believe the current PR is cut as tight as possible -- will go ahead with a separate PR for the `iter(...groupby())` issue.
lgtm. @TomAugspurger if any comments.
thanks @Japanuspus 
Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

```
$ git checkout 1.1.x
$ git pull
```

2. Cherry pick the first parent branch of the this PR on top of the older branch:
```
$ git cherry-pick -m1 d7a5b838d8d6234f6bec5a30bfa33b24bd4afbd9
```

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

```
$ git commit -am "Backport PR #35688: Fix GH-29442 DataFrame.groupby doesn't preserve _metadata"
```

4. Push to a named branch :

```
git push YOURFORK 1.1.x:auto-backport-of-pr-35688-on-1.1.x
```

5. Create a PR against branch 1.1.x, I would have named this PR:

> "Backport PR #35688 on branch 1.1.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to [suggest an improvement](https://github.com/MeeseeksBox/MeeseeksDev).