Skip to content

BUG: None converted to NaN after groupby first and last #33462

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

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

JDkuba
Copy link
Contributor

@JDkuba JDkuba commented Apr 10, 2020

This PR solves #32800. In #32124 not checknull() was introduced to groupfirst() and grouplast(). None is not caught in this conditional and it's being converted to nan because of that.

I'm a newbie and not sure whether I should add whatsnew entry or not. Should I?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks fine, can you add a whatsnew in 1.1, groupby bug fixes. merge master and ping on green.

@@ -893,7 +893,7 @@ def group_last(rank_t[:, :] out,
for j in range(K):
val = values[i, j]

if not checknull(val):
if not checknull(val) or val is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here of why we are doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -986,7 +986,7 @@ def group_nth(rank_t[:, :] out,
for j in range(K):
val = values[i, j]

if not checknull(val):
if not checknull(val) or val is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -94,6 +94,16 @@ def test_nth_with_na_object(index, nulls_fixture):
tm.assert_frame_equal(result, expected)


@pytest.mark.parametrize("method", ["first", "last"])
def test_first_last_with_None(method):
# https://github.com/pandas-dev/pandas/issues/32800
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment, that we wish to preserve None in object dtypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jreback jreback added Bug Dtype Conversions Unexpected or buggy dtype conversions Groupby labels Apr 10, 2020
@JDkuba JDkuba requested a review from jreback April 11, 2020 11:36
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.

lgtm

@WillAyd WillAyd added this to the 1.1 milestone Apr 14, 2020
@jreback jreback merged commit 1834db2 into pandas-dev:master Apr 17, 2020
@jreback
Copy link
Contributor

jreback commented Apr 17, 2020

thanks!

@simonjayhawkins simonjayhawkins mentioned this pull request Apr 17, 2020
@JDkuba JDkuba deleted the bug_groupby_none branch April 18, 2020 08:24
CloseChoice pushed a commit to CloseChoice/pandas that referenced this pull request Apr 20, 2020
…3462)

* BUG: None converted after groupby first and last

* BUG: whatsnew entry
simonjayhawkins pushed a commit to simonjayhawkins/pandas that referenced this pull request May 5, 2020
simonjayhawkins added a commit that referenced this pull request May 5, 2020
rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020
…3462)

* BUG: None converted after groupby first and last

* BUG: whatsnew entry
@simonjayhawkins simonjayhawkins modified the milestones: 1.1, 1.0.4 May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Dtype Conversions Unexpected or buggy dtype conversions Groupby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Groupby converts None to NaN
4 participants