Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
BUG: Accept kwargs kind and na_position in Series.sort_index (GH13589)
Add tests for testing na_position and kind kwargs in Series.sort_index Fix minor issues with tests for Series.sort_index Update whatsnew entry; fix tests Update Series doc-string to mention new additions Remove update from Series doc-string Mention versionadded in Series.sort_index() doc-string Revert: Remove version tag from docstring Add test for invalid arguments
  • Loading branch information
sahildua2305 committed Sep 10, 2016
commit 39ad1cb104579181193a052be208f37e917d3b80
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')