-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Accept kwargs kind and na_position in Series.sort_index (GH13589) #13729
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 all commits
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 |
---|---|---|
|
@@ -1773,7 +1773,7 @@ def _try_kind_sort(arr): | |
|
||
@Appender(generic._shared_docs['sort_index'] % _shared_doc_kwargs) | ||
def sort_index(self, axis=0, level=None, ascending=True, inplace=False, | ||
sort_remaining=True): | ||
kind='quicksort', na_position='last', sort_remaining=True): | ||
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. are these in the same order as DataFrame.sort_index? 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. Yes. |
||
|
||
axis = self._get_axis_number(axis) | ||
index = self.index | ||
|
@@ -1782,12 +1782,15 @@ def sort_index(self, axis=0, level=None, ascending=True, inplace=False, | |
sort_remaining=sort_remaining) | ||
elif isinstance(index, MultiIndex): | ||
from pandas.core.groupby import _lexsort_indexer | ||
indexer = _lexsort_indexer(index.labels, orders=ascending) | ||
indexer = _lexsort_indexer(index.labels, orders=ascending, | ||
na_position=na_position) | ||
indexer = _ensure_platform_int(indexer) | ||
new_index = index.take(indexer) | ||
else: | ||
new_index, indexer = index.sort_values(return_indexer=True, | ||
ascending=ascending) | ||
ascending=ascending, | ||
kind=kind, | ||
na_position=na_position) | ||
|
||
new_values = self._values.take(indexer) | ||
result = self._constructor(new_values, index=new_index) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
from pandas import (DataFrame, Series, MultiIndex) | ||
|
||
from pandas.util.testing import (assert_series_equal, assert_almost_equal) | ||
from pandas.util.testing import (assert_series_equal, assert_almost_equal, assertRaisesRegexp) | ||
import pandas.util.testing as tm | ||
|
||
from .common import TestData | ||
|
@@ -144,3 +144,22 @@ def test_sort_index_multiindex(self): | |
# rows share same level='A': sort has no effect without remaining lvls | ||
res = s.sort_index(level='A', sort_remaining=False) | ||
assert_series_equal(s, res) | ||
|
||
def test_sort_index_nan(self): | ||
|
||
# GH13729 | ||
ser = Series(['A', np.nan, 'C', 'D'], [1, 2, 0, np.nan]) | ||
|
||
# na_position='last', kind='quicksort' | ||
result = ser.sort_index(kind='quicksort', na_position='last') | ||
expected = Series(['C', 'A', np.nan, 'D'], [0, 1, 2, np.nan]) | ||
assert_series_equal(result, expected) | ||
|
||
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. pls change the args a I indicated in a previous somment
|
||
# na_position='first' | ||
result = ser.sort_index(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. pls add test error cases for invalid options. 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 update for this comment? 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. @sinhrks Do you mean I should be testing 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. Yes (although I would assume it should raise a ValueError, but didn't check) 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. I just checked, this is the error it returns:
Just to make sure we are in sync, this is what I need to add test for, right? @jorisvandenbossche |
||
expected = Series(['D', 'C', 'A', np.nan], [np.nan, 0, 1, 2]) | ||
assert_series_equal(result, expected) | ||
|
||
# invalid argument | ||
assertRaisesRegexp(TypeError, 'got an unexpected keyword argument', | ||
ser.sort_index, arg='first') |
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.
can you update the Series (only) doc-string to say kind/na_position were added in 0.19.0.
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.
@jreback Should I add this note in doc-string of
Series
orSeries.sort_index
?