-
- Notifications
You must be signed in to change notification settings - Fork 19.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): | ||
| Contributor 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? Contributor Author 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) | ||
| | ||
| Contributor 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') | ||
| Member 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. Member 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? Contributor Author 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 Member 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) Contributor Author 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
SeriesorSeries.sort_index?