BUG: Accept kwargs kind and na_position in Series.sort_index (GH13589) #13729
Conversation
|
you always needs tests |
|
@jreback I'm not sure what to add in tests for this one. Can you please help me here? |
|
you are adding keywords so need tests - look at what Dataframe tests for snd mirror those |
|
I tried looking into |
|
rebase on master you have a really old master |
Thanks for the hint, @jreback. Please check the tests I have added if they are sufficient or not. |
| @@ -1756,7 +1756,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.
are these in the same order as DataFrame.sort_index?
jreback
added
API Design
Algos
| @@ -731,3 +731,5 @@ Bug Fixes | |||
| - Bug where ``pd.read_gbq()`` could throw ``ImportError: No module named discovery`` as a result of a naming conflict with another python package called apiclient (:issue:`13454`) | |||
| - Bug in ``Index.union`` returns an incorrect result with a named empty index (:issue:`13432`) | |||
| - Bugs in ``Index.difference`` and ``DataFrame.join`` raise in Python3 when using mixed-integer indexes (:issue:`13432`, :issue:`12814`) | |||
|
|
|||
| - Bug in ``Series.sort_index`` didn't accept kwargs ``kind`` and ``na_position`` (:issue:`13589`) | |||
There was a problem hiding this comment.
its not a bug, rather an API change, where Series.sort_index gained these kwargs (they are somewhat similar, but in this case we are highliting the change).
There was a problem hiding this comment.
I would rather put it in the 'other enhancements' section, as it is adding keywords, not changing behaviour of existing ones
There was a problem hiding this comment.
I added in API changes. Should I update again?
There was a problem hiding this comment.
It's OK (we do some clean-up of those files anyway before releasing)
|
@jreback Is this fine? |
| @@ -111,6 +111,8 @@ class Series(base.IndexOpsMixin, strings.StringAccessorMixin, | |||
| associated index values-- they need not be the same length. The result | |||
| index will be the sorted union of the two indexes. | |||
|
|
|||
| `kind` and `na_position` were added in 0.19.0 to Series.sort_index() | |||
There was a problem hiding this comment.
Can u change sort_index docstring rather than the class itself (u have to modify sort_index template appropriately). Also, pls use versionadded tag for each options.
|
@sahildua2305 Only a few small comments, for the rest this is already looking fine! |
|
@jorisvandenbossche I've updated the PR. Please check if it's good now. |
|
can you rebase / update? |
|
Yes! I'll do so. |
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
sahildua2305
force-pushed
the
series-sort-index-bug
branch
from
91ef9be to
39ad1cb
Compare
|
closing in favor of #14445 |
/cc @jreback