BUG: Groupby selection context not being properly reset #28541

christopherzimmerman · 2019-09-19T20:38:54Z

closes DataFrameGroupBy.first() affects group without assignment #28523
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

TomAugspurger · 2019-09-19T20:48:08Z

doc/source/whatsnew/v1.0.0.rst

@@ -240,6 +240,7 @@ Groupby/resample/rolling
 - Bug in :meth:`DataFrame.rolling` not allowing for rolling over datetimes when ``axis=1`` (:issue: `28192`)
 - Bug in :meth:`DataFrame.groupby` not offering selection by column name when ``axis=1`` (:issue:`27614`)
 - Bug in :meth:`DataFrameGroupby.agg` not able to use lambda function with named aggregation (:issue:`27519`)
+- Bug in :meth:`DataFrameGroupby` causing unexpected mutations of the groupby object (:issue:`28523`)


I think the ref needs to be :class:`pandas.core.groupby.DataFrameGroupBy`. And maybe briefly explain the user-visible behavior: "Multiple operations on a DataFrameGroupBy object not giving the same results in certain cases".

TomAugspurger · 2019-09-19T20:49:37Z

pandas/tests/groupby/test_groupby.py

+@pytest.mark.parametrize(
+    "func, args",
+    [
+        ("sum", []),


DO we need all these parametrezations? Or does just a few work? sum and nth?

Probably not. It looks like one of [sum, prod, min, max, first, last] and then nth would suffice.

We have a reduction_func fixture I think you should use here; would actually be more functions but also more comprehensive and consistent with some other tests

WillAyd

Looks good just some minor comments. Did you see any other usages that weren't part of the context manager by chance?

WillAyd · 2019-09-19T21:37:09Z

pandas/tests/groupby/test_groupby.py

+@pytest.mark.parametrize(
+    "func, args",
+    [
+        ("sum", []),


We have a reduction_func fixture I think you should use here; would actually be more functions but also more comprehensive and consistent with some other tests

pandas/tests/groupby/test_groupby.py

jbrockmendel · 2019-09-19T23:04:08Z

Will this fix #26864?

…ix funcs

christopherzimmerman · 2019-09-20T01:45:26Z

So there are two tests that will need to be changed regarding this fix:

test_grouper_creation_bug and test_agg_timezone_round_trip

The second change is straightforward, the first might have identified another bug.

Is this behavior expected?

>>> df = DataFrame({"A": [0, 0, 1, 1, 2, 2], "B": [1, 2, 3, 4, 5, 6]})
>>> df.groupby('A').sum()
    B
A
0   3
1   7
2  11
>>> df.groupby('A').apply(lambda x: x.sum())
   A   B
A
0  0   3
1  2   7
2  4  11

The mutating state caused those two to be equal in the test, but now it fails, and I'm not sure if the second is intended behavior.

WillAyd · 2019-09-20T01:58:33Z

I think the second is a bug might have open issue already

…

Sent from my iPhone

On Sep 19, 2019, at 6:45 PM, Chris Zimmerman ***@***.***> wrote: So there are two tests that will need to be changed regarding this fix: test_grouper_creation_bug and test_agg_timezone_round_trip The second change is straightforward, the first might have identified another bug. Is this behavior expected? >>> df = DataFrame({"A": [0, 0, 1, 1, 2, 2], "B": [1, 2, 3, 4, 5, 6]}) >>> df.groupby('A').sum() B A 0 3 1 7 2 11 >>> df.groupby('A').apply(lambda x: x.sum()) A B A 0 0 3 1 2 7 2 4 11 The mutating state caused those two to be equal in the test, but now it fails, and I'm not sure if the second is intended behavior. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

WillAyd · 2019-09-20T02:27:13Z

#22546 #22542 #20420 might all be solved by this

christopherzimmerman · 2019-09-20T02:32:00Z

@WillAyd OK, I'll track down that bug and take care of that before I do anything else with this PR. Also per your previous comment, making the test work with reduction_func removed the last instance of the improper set selection, so I think the context manager is now used everywhere.

WillAyd · 2019-09-20T02:34:33Z

Sounds good - nice work so far!

WillAyd · 2019-10-07T18:47:43Z

#17091 might be closed by this as well

jreback · 2019-10-08T12:47:36Z

@christopherzimmerman can you merge master, been some changed in groupby recently.

jbrockmendel · 2019-10-21T01:27:49Z

@christopherzimmerman can you rebase, this would be really nice to see fixed

… into avoid_mutate_groupby

pep8speaks · 2019-10-21T02:04:08Z

Hello @christopherzimmerman! 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 2019-10-21 13:45:44 UTC

christopherzimmerman · 2019-10-21T13:48:46Z

@jbrockmendel rebased and cleaned up what I was working on. #28549 still needs to be addressed before this is green. I had a PR going for that issue and I'll revisit it.

jbrockmendel · 2019-11-13T02:03:00Z

a lot of work has been going on in these files; can you rebase again? ill try to review in a timely manner so we dont have to go through another round

jreback · 2020-01-01T17:22:16Z

@christopherzimmerman would be a nice change, can you merge master

christopherzimmerman · 2020-01-02T14:37:16Z

I will merge, but #29131 will need to be merged first or else the CI will fail, since this requires some test updates from the other PR.

WillAyd · 2020-03-14T21:48:38Z

Let's come back to this after the other PR is merged then; (looks like more changes are needed there).

Closing to reduce PR queue for the time being - thanks as always!

christopherzimmerman added 3 commits September 19, 2019 14:52

Added failing test for groupby mutation

b5f01b7

Switched implementation to use _group_selection_context

1dbb12d

added whatsnew entry

bcab4a0

christopherzimmerman changed the title ~~BUG: Groupby selection not context not being properly reset~~ BUG: Groupby selection context not being properly reset Sep 19, 2019

TomAugspurger reviewed Sep 19, 2019

View reviewed changes

WillAyd requested changes Sep 19, 2019

View reviewed changes

WillAyd added the Groupby label Sep 19, 2019

WillAyd added this to the 1.0 milestone Sep 19, 2019

updated test, removed last instance of not using context manager to f…

ca26b64

…ix funcs

christopherzimmerman mentioned this pull request Sep 20, 2019

Many groupby tests depend on a bug in DataFrameGroupBy.apply #28549

Closed

christopherzimmerman added 2 commits October 20, 2019 20:55

Merge branch 'master' of https://github.com/christopherzimmerman/pandas…

c4b0a53

… into avoid_mutate_groupby

Rebase from master

7a9e1d1

christopherzimmerman added 3 commits October 20, 2019 21:31

Missed a merge

f24c05c

Fixed two tests

5cc3cb1

Another context needed to be set

3050963

christopherzimmerman mentioned this pull request Oct 21, 2019

[BUG] Fixed behavior of DataFrameGroupBy.apply to respect _group_selection_context #29131

Closed

5 tasks

jreback removed this from the 1.0 milestone Jan 1, 2020

WillAyd closed this Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Groupby selection context not being properly reset #28541

BUG: Groupby selection context not being properly reset #28541

christopherzimmerman commented Sep 19, 2019 •

edited

Loading

TomAugspurger Sep 19, 2019

TomAugspurger Sep 19, 2019

christopherzimmerman Sep 19, 2019

WillAyd Sep 19, 2019

WillAyd left a comment

WillAyd Sep 19, 2019

jbrockmendel commented Sep 19, 2019

christopherzimmerman commented Sep 20, 2019

WillAyd commented Sep 20, 2019 via email

WillAyd commented Sep 20, 2019

christopherzimmerman commented Sep 20, 2019

WillAyd commented Sep 20, 2019

WillAyd commented Oct 7, 2019

jreback commented Oct 8, 2019

jbrockmendel commented Oct 21, 2019

pep8speaks commented Oct 21, 2019 •

edited

Loading

christopherzimmerman commented Oct 21, 2019

jbrockmendel commented Nov 13, 2019

jreback commented Jan 1, 2020

christopherzimmerman commented Jan 2, 2020

WillAyd commented Mar 14, 2020

BUG: Groupby selection context not being properly reset #28541

BUG: Groupby selection context not being properly reset #28541

Conversation

christopherzimmerman commented Sep 19, 2019 • edited Loading

TomAugspurger Sep 19, 2019

Choose a reason for hiding this comment

TomAugspurger Sep 19, 2019

Choose a reason for hiding this comment

christopherzimmerman Sep 19, 2019

Choose a reason for hiding this comment

WillAyd Sep 19, 2019

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd Sep 19, 2019

Choose a reason for hiding this comment

jbrockmendel commented Sep 19, 2019

christopherzimmerman commented Sep 20, 2019

WillAyd commented Sep 20, 2019 via email

WillAyd commented Sep 20, 2019

christopherzimmerman commented Sep 20, 2019

WillAyd commented Sep 20, 2019

WillAyd commented Oct 7, 2019

jreback commented Oct 8, 2019

jbrockmendel commented Oct 21, 2019

pep8speaks commented Oct 21, 2019 • edited Loading

Comment last updated at 2019-10-21 13:45:44 UTC

christopherzimmerman commented Oct 21, 2019

jbrockmendel commented Nov 13, 2019

jreback commented Jan 1, 2020

christopherzimmerman commented Jan 2, 2020

WillAyd commented Mar 14, 2020

christopherzimmerman commented Sep 19, 2019 •

edited

Loading

pep8speaks commented Oct 21, 2019 •

edited

Loading