Skip to content

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.19.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ Other enhancements
- ``Timestamp``, ``Period``, ``DatetimeIndex``, ``PeriodIndex`` and ``.dt`` accessor have gained a ``.is_leap_year`` property to check whether the date belongs to a leap year. (:issue:`13727`)
- ``astype()`` will now accept a dict of column name to data types mapping as the ``dtype`` argument. (:issue:`12086`)
- The ``pd.read_json`` and ``DataFrame.to_json`` has gained support for reading and writing json lines with ``lines`` option see :ref:`Line delimited json <io.jsonl>` (:issue:`9180`)
- `Series.sort_index`` will now accept kwargs ``kind`` and ``na_position`` (:issue:`13589`)
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 update the Series (only) doc-string to say kind/na_position were added in 0.19.0.

Copy link
Contributor Author

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 or Series.sort_index ?


.. _whatsnew_0190.api:

Expand Down
6 changes: 3 additions & 3 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -2022,16 +2022,16 @@ def sort_values(self, by, axis=0, ascending=True, inplace=False,
Sort ascending vs. descending
inplace : bool, default False
if True, perform operation in-place
sort_remaining : bool, default True
if true and sorting by level and index is multilevel, sort by other
levels too (in order) after sorting by specified level
kind : {'quicksort', 'mergesort', 'heapsort'}, default 'quicksort'
Choice of sorting algorithm. See also ndarray.np.sort for more
information. `mergesort` is the only stable algorithm. For
DataFrames, this option is only applied when sorting on a single
column or label.
na_position : {'first', 'last'}, default 'last'
`first` puts NaNs at the beginning, `last` puts NaNs at the end
sort_remaining : bool, default True
if true and sorting by level and index is multilevel, sort by other
levels too (in order) after sorting by specified level

Returns
-------
Expand Down
9 changes: 6 additions & 3 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

are these in the same order as DataFrame.sort_index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.


axis = self._get_axis_number(axis)
index = self.index
Expand All @@ -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)
Expand Down
21 changes: 20 additions & 1 deletion pandas/tests/series/test_sorting.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Copy link
Contributor

Choose a reason for hiding this comment

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

pls change the args a I indicated in a previous somment

result = ser.sort_index(.....)
expected = .....
assert_series_equal(result, expected)

# na_position='first'
result = ser.sort_index(na_position='first')
Copy link
Member

@sinhrks sinhrks Aug 2, 2016

Choose a reason for hiding this comment

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

pls add test error cases for invalid options.

Copy link
Member

Choose a reason for hiding this comment

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

Can you update for this comment?

Copy link
Contributor Author

@sahildua2305 sahildua2305 Aug 16, 2016

Choose a reason for hiding this comment

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

@sinhrks Do you mean I should be testing TypeError here (for invalid arguments)?

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked, this is the error it returns:

TypeError: sort_index() got an unexpected keyword argument 'random_argument'

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')