BUG: Groupby.apply wasn't allowing for functions which return lists #31456
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarcoGorelli
force-pushed
the
issue-31441
branch
from
1d1904f to
ed45390
Compare
MarcoGorelli
changed the title
jreback
requested changes
Jan 31, 2020
pandas/_libs/reduction.pyx
Outdated
| @@ -499,7 +499,9 @@ def apply_frame_axis0(object frame, object f, object names, | |||
| # `piece` might not have an index, could be e.g. an int | |||
| pass | |||
|
|
|||
| if not is_scalar(piece): | |||
| if isinstance(piece, list): | |||
There was a problem hiding this comment.
this is not the correct patch,
instead lets change line 507 (in revised code) to be
piece = deepcopy(piece)
There was a problem hiding this comment.
this is not the correct patch,
instead lets change line 507 (in revised code) to be
piece = deepcopy(piece)
This causes some failures, e.g.
$ pytest pandas/tests/groupby/test_groupby.py::test_no_mutate_but_looks_like
which doesn't fail with
if isinstance(piece, list):
piece = deepcopy(piece)
elif not is_scalar(piece):
# Need to copy data to avoid appending references
if hasattr(piece, "copy"):
piece = piece.copy(deep='all')
else:
piece = copy(piece)
There was a problem hiding this comment.
@MarcoGorelli what is the difference that this causes in that test? Would it return one of the results completely as a list instead of exploding to a series?
Returning a list from a groupby op is thorny to begin with so I think should streamline logic as much as possible
There was a problem hiding this comment.
@WillAyd here's the difference:
>>> df = DataFrame({"key": [1, 1, 1, 2, 2, 2, 3, 3, 3], "value": range(9)})
>>> result1 = df.groupby("key", group_keys=True).apply(lambda x: x[:].key)
>>> result2 = df.groupby("key", group_keys=True).apply(lambda x: x.key)
>>> result1
key
1 0 1
1 1
2 1
2 3 2
4 2
5 2
3 6 3
7 3
8 3
>>> result2
key 0 1 2
key
1 1 1 1
2 2 2 2
3 3 3 3
Returning a list from a groupby op is thorny to begin with so I think should streamline logic as much as possible
OK, I'll think of a workaround (but I realise that you'd like to get a v1.0.1 release out very soon so if you think this'll take me too long please do feel free to take over)
There was a problem hiding this comment.
I'm somewhat confused here. If I use this, the test case passes:
pd._testing.assert_series_equal(deepcopy(piece), piece.copy(deep="all"))
piece = piece.copy(deep="all")
but if I use this, it doesn't!
pd._testing.assert_series_equal(deepcopy(piece), piece.copy(deep="all"))
piece = deepcopy(piece)
will look into it
There was a problem hiding this comment.
@dsaxton see here - I share your puzzlement :) Will look into Series.copy tomorrow
jreback
added
Groupby
Regression
MarcoGorelli
force-pushed
the
issue-31441
branch
from
7aa36d9 to
6369f76
Compare
NOTEThe following refers to a commit with line 507 (in revised code) changed to: (so, without the
Sure, here's the output from a run I did: EDITseems like there's some test failures though |
|
Performance results with current commit: |
MarcoGorelli
force-pushed
the
issue-31441
branch
from
6369f76 to
8ede846
Compare
dsaxton
reviewed
Jan 31, 2020
pandas/_libs/reduction.pyx
Outdated
| @@ -499,7 +499,9 @@ def apply_frame_axis0(object frame, object f, object names, | |||
| # `piece` might not have an index, could be e.g. an int | |||
| pass | |||
|
|
|||
| if not is_scalar(piece): | |||
| if isinstance(piece, list): | |||
There was a problem hiding this comment.
Do we want to special case lists specifically here? How about special casing only those times when copy is known to have the deep keyword for piece.copy(deep="all") and letting everything else use a deepcopy? (eg., something like df.groupby("a").apply(lambda x: set(x.index)) would still raise here)
There was a problem hiding this comment.
How about special casing only those times when copy is known to have the deep keyword
How - by using inspect?
There was a problem hiding this comment.
I had been thinking checking specifically for Series, DataFrame, or Index and trying to deepcopy everything else, but there may be a "prettier" way of approaching it. I remember getting the exact same test failure you're seeing now and am still puzzled by it.
MarcoGorelli
force-pushed
the
issue-31441
branch
from
8ede846 to
a198cc8
Compare
dsaxton
reviewed
Jan 31, 2020
pandas/tests/groupby/test_apply.py
Outdated
| def test_apply_function_returns_list(): | ||
| # GH 31441 | ||
| df = pd.DataFrame(["A", "A", "B", "B"], columns=["groups"]) | ||
| result = df.groupby("groups").apply(lambda x: x.index.to_list()) |
There was a problem hiding this comment.
Might be good to add tests for other return types as well (e.g., set or tuple)
There was a problem hiding this comment.
Yes, good suggestion, thanks!
dsaxton
reviewed
Feb 1, 2020
| @pytest.mark.parametrize( | ||
| "function, expected_values", | ||
| [ | ||
| (lambda x: x.index.to_list(), [[0, 1], [2, 3]]), |
There was a problem hiding this comment.
We should be able to skip the conversion to list in the remaining test cases
jreback
approved these changes
Feb 1, 2020
|
thanks @MarcoGorelli |
meeseeksmachine
pushed a commit
to meeseeksmachine/pandas
that referenced
this pull request
Feb 1, 2020
…functions which return lists
jreback
added
Still Needs Manual Backport
and removed
Still Needs Manual Backport
labels
jschendel
pushed a commit
that referenced
this pull request
Feb 1, 2020
…which return lists (#31541) Co-authored-by: Marco Gorelli <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff