-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: df.sort_values() not respecting na_position with categoricals #22556 #22640
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
Changes from 7 commits
d119a00
f36346b
1e26989
dc4b4c4
a4ee75f
2b6f0b0
229fff5
fbc8a4e
41b29fb
1999d58
76da923
8f81649
8943280
1709c91
8983dd9
7f6c68a
9f0343b
9e7afb1
8d4aedc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,7 +241,19 @@ def nargsort(items, kind='quicksort', ascending=True, na_position='last'): | |
|
||
# specially handle Categorical | ||
if is_categorical_dtype(items): | ||
return items.argsort(ascending=ascending, kind=kind) | ||
if na_position not in ['first', 'last']: | ||
staftermath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
raise ValueError('invalid na_position: {!r}'.format(na_position)) | ||
mask = isna(items) | ||
staftermath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cnt_null = mask.sum() | ||
sorted_idx = items.argsort(ascending=ascending, kind=kind) | ||
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. do we not have a .sort_values on Categorical that accepts na_position already? if we don’t all of this code should live there anyhow 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. the way I see it, |
||
if ascending: | ||
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. Can you simplify the condition here to just negate 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. good point. not sure how to do condition check and negate |
||
# NaN is coded as -1 and is listed in front after sorting | ||
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. this duplicates some code in 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. great suggestion. updated. |
||
return sorted_idx if na_position == 'first' \ | ||
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. We typically use implicit line continuation where possible. If still needed after refactor of above condition would prefer that style |
||
else np.roll(sorted_idx, -cnt_null) | ||
else: | ||
# NaN is coded as -1 and is listed in the end after sorting | ||
return sorted_idx if na_position == 'last' else \ | ||
np.roll(sorted_idx, cnt_null) | ||
|
||
items = np.asanyarray(items) | ||
idx = np.arange(len(items)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
from pandas.compat import lrange | ||
from pandas.api.types import CategoricalDtype | ||
from pandas import (DataFrame, Series, MultiIndex, Timestamp, | ||
date_range, NaT, IntervalIndex) | ||
date_range, NaT, IntervalIndex, Categorical) | ||
|
||
from pandas.util.testing import assert_series_equal, assert_frame_equal | ||
|
||
|
@@ -598,3 +598,55 @@ def test_sort_index_intervalindex(self): | |
closed='right') | ||
result = result.columns.levels[1].categories | ||
tm.assert_index_equal(result, expected) | ||
|
||
def test_sort_index_na_position_with_categories(self): | ||
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. Can you parametrize this test? Believe you can use 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. @WillAyd parametrized a bit. I am pretty new at this. Thanks for all the suggestions! 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. No worries - appreciate the help and we hope you learn as you go through this process. FWIW this unfortunately not the parametrization we are looking for. There should be a decorator for the function which performs that actual task. What you want is the |
||
# GH 22556 | ||
# Positioning missing value properly when column is Categorical. | ||
df_category_with_nan = pd.DataFrame({ | ||
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. Can you parametrize these various cases? |
||
'c': pd.Categorical(['A', np.nan, 'B', np.nan, 'C'], | ||
categories=['A', 'B', 'C'], | ||
ordered=True)}) | ||
result = df_category_with_nan.sort_values(by='c', | ||
ascending=True, | ||
na_position='first') | ||
expected = DataFrame({ | ||
'c': Categorical([np.nan, np.nan, 'A', 'B', 'C'], | ||
categories=['A', 'B', 'C'], | ||
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. can you see if you can parametrize this a bit |
||
ordered=True)}, index=[1, 3, 0, 2, 4]) | ||
|
||
assert_frame_equal(result, expected) | ||
|
||
result = df_category_with_nan.sort_values(by='c', | ||
ascending=True, | ||
na_position='last') | ||
expected = DataFrame({ | ||
'c': Categorical(['A', 'B', 'C', np.nan, np.nan], | ||
categories=['A', 'B', 'C'], | ||
ordered=True)}, index=[0, 2, 4, 1, 3]) | ||
|
||
assert_frame_equal(result, expected) | ||
|
||
result = df_category_with_nan.sort_values(by='c', | ||
ascending=False, | ||
na_position='first') | ||
expected = DataFrame({ | ||
'c': Categorical([np.nan, np.nan, 'C', 'B', 'A'], | ||
categories=['A', 'B', 'C'], | ||
ordered=True)}, index=[3, 1, 4, 2, 0]) | ||
|
||
assert_frame_equal(result, expected) | ||
|
||
result = df_category_with_nan.sort_values(by='c', | ||
ascending=False, | ||
na_position='last') | ||
expected = DataFrame({ | ||
'c': Categorical(['C', 'B', 'A', np.nan, np.nan], | ||
categories=['A', 'B', 'C'], | ||
ordered=True)}, index=[4, 2, 0, 3, 1]) | ||
|
||
assert_frame_equal(result, expected) | ||
|
||
with pytest.raises(ValueError): | ||
staftermath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
df_category_with_nan.sort_values(by='c', | ||
ascending=False, | ||
na_position='bad_position') |
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.
Extra backtick before
na_position