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

[Japanuspus]

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.

• closes DataFrame.groupby doesn't preserve _metadata #29442
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @Japanuspus! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2020-10-09 12:42:14 UTC

[Japanuspus]

@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.

[jbrockmendel]

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.

[TomAugspurger]

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()?

[Japanuspus]

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?

[Japanuspus]

Regarding performance implications of __finalize__ in _chop

I tried running the benchmark from GH-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)

[jbrockmendel]

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)

[Japanuspus]

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?

[TomAugspurger]

Perhaps run the groupby with a CategoricalIndex?

[Japanuspus]

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.

[TomAugspurger]

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.

[Japanuspus]

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? 8d3d896

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]

does this need a release note?

[Japanuspus]

does this need a release note?

Added a release note for 1.1.4

[jreback]

@Japanuspus pls limit changes to one thing in a PR, esp as this is a backport.

[Japanuspus]

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?

[jreback]

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

[Japanuspus]

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.

[jreback]

lgtm. @TomAugspurger if any comments.

[jreback]

thanks @Japanuspus