Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: as_index=False can return a MultiIndex in groupby.apply #58369

Merged
merged 10 commits into from
May 1, 2024
Merged

BUG: as_index=False can return a MultiIndex in groupby.apply #58369

merged 10 commits into from
May 1, 2024

Conversation

undermyumbrella1
Copy link
Contributor

@undermyumbrella1 undermyumbrella1 commented Apr 22, 2024 •
edited
Loading

@undermyumbrella1 undermyumbrella1 marked this pull request as draft April 22, 2024 14:17
@undermyumbrella1 undermyumbrella1 force-pushed the fix/as_index_false_returns_multiindex branch from 800a2c3 to 52fb291 Compare April 23, 2024 17:34
@mroeschke mroeschke added Groupby MultiIndex Apply Apply, Aggregate, Transform, Map Index Related to the Index class or subclasses and removed Index Related to the Index class or subclasses labels Apr 23, 2024
@undermyumbrella1 undermyumbrella1 marked this pull request as ready for review April 25, 2024 08:23
@undermyumbrella1 undermyumbrella1 force-pushed the fix/as_index_false_returns_multiindex branch from f386f6a to 5a30216 Compare April 25, 2024 12:21
@undermyumbrella1
Copy link
Contributor Author

undermyumbrella1 commented Apr 25, 2024 •
edited
Loading

Removed the addition of multi index, instead of using reset_index(), not sure if there is a use case that would require using reset_index() instead.

rhshadrach
rhshadrach requested changes Apr 27, 2024
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Code change looks good, some requests / questions on the tests. In general, don't leave print statements in the tests - these are captured by pytest and would hurt test suite performance if we left them throughout the tests.

pandas/tests/groupby/methods/test_value_counts.py Outdated
Comment on lines 332 to 333
if "level_0" in result:
expected.insert(loc=0, column="level_0", value=result["level_0"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests, the expected should not depend on the result. This would allow behavior to change but the test to remain the same, and therefore possibly go unnoticed. Can you find a different condition here that doesn't depend on result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved to update expected results based on test parameters

pandas/tests/groupby/test_apply.py Outdated
Comment on lines 1588 to 1590
# GH58291
def test_apply_frame_not_as_index_returns_single_index():
df = DataFrame(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move inside the function to agree with the style of the other tests.

def test_...():
    # GH#58291

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

pandas/tests/groupby/test_apply.py Outdated


# GH58291
def test_apply_column_groupby_frame_not_as_index_returns_single_index(df):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this testing that we don't have test coverage for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved by removing all tests as they are already tested in other tests

pandas/tests/groupby/test_apply.py Outdated


# GH58291
def test_apply_non_column_groupby_frame_not_as_index_returns_single_index(tsframe):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question - what case is this looking to expand our test coverage?

@undermyumbrella1 undermyumbrella1 force-pushed the fix/as_index_false_returns_multiindex branch 2 times, most recently from cd9ae2c to 062031e Compare April 28, 2024 15:31
@undermyumbrella1 undermyumbrella1 force-pushed the fix/as_index_false_returns_multiindex branch from 794ef85 to 8457147 Compare April 28, 2024 15:33
@undermyumbrella1
Copy link
Contributor Author

Thank you for the review, I have updated the pr according to the comments

@rhshadrach rhshadrach added this to the 3.0 milestone May 1, 2024
rhshadrach
rhshadrach approved these changes May 1, 2024
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhshadrach rhshadrach changed the title Fix/as index false returns multiindex BUG: as_index=False can return a MultiIndex in groupby.apply May 1, 2024
@rhshadrach rhshadrach merged commit 564d0d9 into pandas-dev:main May 1, 2024
46 checks passed
@rhshadrach
Copy link
Member

Thanks @undermyumbrella1!

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
@undermyumbrella1 @pmhatre1
BUG: as_index=False can return a MultiIndex in groupby.apply (pandas-…
512153e 
…dev#58369)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhshadrach rhshadrach rhshadrach approved these changes

Assignees
No one assigned
Labels
Apply Apply, Aggregate, Transform, Map Groupby MultiIndex
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

BUG: GroupBy.apply with as_index=False still produces a MultiIndex
3 participants
@undermyumbrella1 @rhshadrach @mroeschke