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.

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

TST: Groupby first/last/nth nan column test #33627

Merged
merged 5 commits into from
Apr 23, 2020

Conversation

jamescobonkerr
Copy link
Contributor

@jamescobonkerr jamescobonkerr commented Apr 18, 2020

The first two asserts (first()/last()) fail on 1.0.3, as expected.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks - just a minor nit to make things more readable otherwise lgtm

pandas/tests/groupby/test_nth.py Outdated Show resolved Hide resolved
@WillAyd WillAyd added the Testing pandas testing functions or related to the test suite label Apr 18, 2020
@WillAyd WillAyd added this to the 1.1 milestone Apr 18, 2020
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @jamescobonkerr for the PR. does this test follow the same code path as the code sample in the issue OP?, see #33591 (comment)

@simonjayhawkins
Copy link
Member

The first two asserts (first()/last()) fail on 1.0.3, as expected

this was changed in #32949

be86b65 is the first new commit
commit be86b65
Author: jbrockmendel jbrockmendel@gmail.com
Date: Fri Apr 10 09:18:06 2020 -0700

CLN: remove unnecessary Series._convert calls (#32949)

@jamescobonkerr
Copy link
Contributor Author

Thanks @jamescobonkerr for the PR. does this test follow the same code path as the code sample in the issue OP?, see #33591 (comment)

Ah, you're right — pardon the oversight.

The first two asserts (first()/last()) fail on 1.0.3, as expected

this was changed in #32949

be86b65 is the first new commit
commit be86b65
Author: jbrockmendel jbrockmendel@gmail.com
Date: Fri Apr 10 09:18:06 2020 -0700

CLN: remove unnecessary Series._convert calls (#32949)

Removing the _convert call from core.groupby.generic.DataFrameGroupBy._wrap_aggregated_output seems to fix the reported issue, however, breaks a range of other tests. I don't have a great understanding of how GroupBy works. I'll close this PR.

@simonjayhawkins
Copy link
Member

@jamescobonkerr Thanks for working on this.

The test you have added here does have value since it is testing a change in behaviour against regression.

As you pointed out this test fails on 1.0.3 and is fixed on master. AFAICT this is working since #32949

I'll reopen this PR, but once merged will not close #33591 (rather than open a new issue at this point, i'll reference you code sample in #33591)

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @jamescobonkerr lgtm pending green ci.

pandas/tests/groupby/test_nth.py Outdated Show resolved Hide resolved
@jamescobonkerr jamescobonkerr marked this pull request as draft April 23, 2020 14:27
@jamescobonkerr jamescobonkerr marked this pull request as ready for review April 23, 2020 15:42
@jreback jreback merged commit 3db460c into pandas-dev:master Apr 23, 2020
@jreback
Copy link
Contributor

jreback commented Apr 23, 2020

thanks @jamescobonkerr always happy to take these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants