-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix Bug with NA value in Grouping for Groupby.nth #26152
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
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
e62a14f
Added failing test
WillAyd e113b55
Merge remote-tracking branch 'upstream/master' into groupby-nth-nan
WillAyd 823225b
Added fix in code
WillAyd 068936f
Expanded test coverage scope
WillAyd 8716764
Whatsnew note
WillAyd 3000d2b
Fixed implementation
WillAyd 84ddd6d
Typing and doc fixups
WillAyd 84d343a
Fixed dropna annotation
WillAyd 6f40fbe
Replaced Collection reference with List in annotation
WillAyd e2d006d
Fixed cast expression
WillAyd 69014a7
Merge remote-tracking branch 'upstream/master' into groupby-nth-nan
WillAyd 0b7eb6c
Moved comment position
WillAyd 48d90ee
Removed re-assignment of nth_values for typing
WillAyd 7eea5e3
typing fixup
WillAyd 4dec450
Merge remote-tracking branch 'upstream/master' into groupby-nth-nan
WillAyd ce0abcd
Merge remote-tracking branch 'upstream/master' into groupby-nth-nan
WillAyd c2a0b8e
dedented and added comment
WillAyd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ class providing the base-class of operations. | |
import datetime | ||
from functools import partial, wraps | ||
import types | ||
from typing import FrozenSet, Optional, Tuple, Type | ||
from typing import FrozenSet, List, Optional, Tuple, Type, Union | ||
import warnings | ||
|
||
import numpy as np | ||
|
@@ -1546,15 +1546,16 @@ def backfill(self, limit=None): | |
|
||
@Substitution(name='groupby') | ||
@Substitution(see_also=_common_see_also) | ||
def nth(self, n, dropna=None): | ||
def nth(self, | ||
n: Union[int, List[int]], | ||
dropna: Optional[str] = None) -> DataFrame: | ||
""" | ||
Take the nth row from each group if n is an int, or a subset of rows | ||
if n is a list of ints. | ||
|
||
If dropna, will take the nth non-null row, dropna is either | ||
Truthy (if a Series) or 'all', 'any' (if a DataFrame); | ||
this is equivalent to calling dropna(how=dropna) before the | ||
groupby. | ||
'all' or 'any'; this is equivalent to calling dropna(how=dropna) | ||
before the groupby. | ||
|
||
Parameters | ||
---------- | ||
|
@@ -1617,34 +1618,43 @@ def nth(self, n, dropna=None): | |
4 2 5.0 | ||
""" | ||
|
||
if isinstance(n, int): | ||
nth_values = [n] | ||
elif isinstance(n, (set, list, tuple)): | ||
nth_values = list(set(n)) | ||
if dropna is not None: | ||
raise ValueError( | ||
"dropna option with a list of nth values is not supported") | ||
else: | ||
valid_containers = (set, list, tuple) | ||
if not isinstance(n, (valid_containers, int)): | ||
raise TypeError("n needs to be an int or a list/set/tuple of ints") | ||
|
||
nth_values = np.array(nth_values, dtype=np.intp) | ||
self._set_group_selection() | ||
|
||
if not dropna: | ||
mask_left = np.in1d(self._cumcount_array(), nth_values) | ||
|
||
if isinstance(n, int): | ||
nth_values = [n] | ||
elif isinstance(n, valid_containers): | ||
nth_values = list(set(n)) | ||
|
||
nth_array = np.array(nth_values, dtype=np.intp) | ||
self._set_group_selection() | ||
|
||
mask_left = np.in1d(self._cumcount_array(), nth_array) | ||
mask_right = np.in1d(self._cumcount_array(ascending=False) + 1, | ||
-nth_values) | ||
-nth_array) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note I had to change this because |
||
mask = mask_left | mask_right | ||
|
||
ids, _, _ = self.grouper.group_info | ||
|
||
# Drop NA values in grouping | ||
mask = mask & (ids != -1) | ||
|
||
out = self._selected_obj[mask] | ||
if not self.as_index: | ||
return out | ||
|
||
ids, _, _ = self.grouper.group_info | ||
out.index = self.grouper.result_index[ids[mask]] | ||
|
||
return out.sort_index() if self.sort else out | ||
|
||
# dropna is truthy | ||
if isinstance(n, valid_containers): | ||
raise ValueError( | ||
"dropna option with a list of nth values is not supported") | ||
|
||
if dropna not in ['any', 'all']: | ||
if isinstance(self._selected_obj, Series) and dropna is True: | ||
warnings.warn("the dropna={dropna} keyword is deprecated," | ||
|
@@ -1679,15 +1689,16 @@ def nth(self, n, dropna=None): | |
|
||
else: | ||
|
||
# create a grouper with the original parameters, but on the dropped | ||
# create a grouper with the original parameters, but on dropped | ||
# object | ||
from pandas.core.groupby.grouper import _get_grouper | ||
grouper, _, _ = _get_grouper(dropped, key=self.keys, | ||
axis=self.axis, level=self.level, | ||
sort=self.sort, | ||
mutated=self.mutated) | ||
|
||
grb = dropped.groupby(grouper, as_index=self.as_index, sort=self.sort) | ||
grb = dropped.groupby( | ||
grouper, as_index=self.as_index, sort=self.sort) | ||
sizes, result = grb.size(), grb.nth(n) | ||
mask = (sizes < max_len).values | ||
|
||
|
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of explicitly checking
if dropna is not None
this condition was refactored to sit in a branch that follows the implicit condition ofif dropna
.There is however a slight behavior difference between this PR and master, where now these are equivalent:
Whereas on master these would not yield the same thing:
I think the new behavior is more consistent and preferred. Might be worth a whatsnew