TST: Add regression test for groupby().apply() external mutation #63126
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
|
All CI checks passed, the pull request is ready for review but i don't see a button to request a reviewer. |
rhshadrach
left a comment
rhshadrach
left a comment
There was a problem hiding this comment.
Thanks for the PR - looking good!
pandas/tests/groupby/test_apply.py
Outdated
| # The expected output in store dict | ||
| expected_out = {0: df[out_mask[0]], 1: df[out_mask[1]]} | ||
|
|
||
| tm.assert_frame_equal(store[0], expected_out[0].drop("B", axis=1)) |
There was a problem hiding this comment.
Expected here can just be df.iloc[[0, 2], 1] I think.
There was a problem hiding this comment.
Apolgoies, I should have written df.iloc[[0, 2], [1]]. What I wrote will give you a Series, but with [1] instead will give you a DataFrame. This is minor, but in general we want to check the entire result in tests without modifications (in this case, a DataFrame).
pandas/tests/groupby/test_apply.py
Outdated
| expected_out = {0: df[out_mask[0]], 1: df[out_mask[1]]} | ||
|
|
||
| tm.assert_frame_equal(store[0], expected_out[0].drop("B", axis=1)) | ||
| tm.assert_frame_equal(store[1], expected_out[1].drop("B", axis=1)) |
pandas/tests/groupby/test_apply.py
Outdated
| } | ||
| ) | ||
|
|
||
| # Empty dict to hold the chunks |
There was a problem hiding this comment.
This comment and the ones below don't add anything that isn't already in the code. Can you remove them.
jadosh123
force-pushed
the
GH/40673-test-apply-mutation
branch
from
6f7a751 to
7e53e6f
Compare
|
@rhshadrach I've removed the comments, I tried the suggestion you made but couldn't get it to pass the test so I kept it as is if that's fine. |
|
I've updated the test test_groupby_apply_store_copy to use iloc slicing as you suggested. |
jadosh123
force-pushed
the
GH/40673-test-apply-mutation
branch
from
d9aea26 to
39eef05
Compare
|
No problem, I've changed it to compare the DataFrames now. |
pandas-devGH-40673: Adds a test case to prevent regression of a bug where the internal object reused by apply() would corrupt externally stored DataFrames created with .copy(). This test verifies that store[0] and store[1] correctly contain independent copies of their respective groups.
jadosh123
force-pushed
the
GH/40673-test-apply-mutation
branch
from
9bb5bc6 to
11eefc4
Compare
rhshadrach
added
Needs Tests
|
Thanks @jadosh123! |
5 participants
GH-40673: Adds a test case to prevent regression of a bug where the internal object reused by apply() would corrupt externally stored DataFrames created with .copy(). This test verifies that store[0] and store[1] correctly contain independent copies of their respective groups.
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.