BUG: make Series.sort_values(ascending=[False]) behave as ascending=False (#15604) #15607
Conversation
pandas/core/series.py
Outdated
| @@ -1722,6 +1723,12 @@ def _try_kind_sort(arr): | |||
|
|
|||
| argsorted = _try_kind_sort(arr[good]) | |||
|
|
|||
| if is_sequence(ascending): | |||
There was a problem hiding this comment.
hmm, how about is_list_like is our general purpose list-detector.
There was a problem hiding this comment.
Fine by me, I was following the code in frame.py. Should I change and rebase?
pandas/core/series.py
Outdated
| raise ValueError('Length of ascending (%d) cannot be larger ' | ||
| 'than 1 for Series' % (len(ascending))) | ||
| ascending = ascending[0] | ||
|
|
||
| if not ascending: |
There was a problem hiding this comment.
then do:
if not is_bool(ascending):
raise ValueError(....)
pandas/core/series.py
Outdated
| @@ -1722,6 +1723,12 @@ def _try_kind_sort(arr): | |||
|
|
|||
| argsorted = _try_kind_sort(arr[good]) | |||
|
|
|||
| if is_sequence(ascending): | |||
| if len(ascending) > 1: | |||
pandas/tests/series/test_sorting.py
Outdated
| assert_almost_equal(expected, ordered.valid().values) | ||
| ordered = ts.sort_values(ascending=[False], na_position='first') | ||
| assert_almost_equal(expected, ordered.valid().values) | ||
|
|
There was a problem hiding this comment.
can you test a couple of invalid ones that they raise (e.g. [], [1, 2, 3], [False, False, False], 'foobar', None
(ascending=None) is technically invalid.
|
bit more testing / validation & please add a whatsnew note. ping when green. |
jreback
added
Bug
Error Reporting
MLopez-Ibanez
force-pushed
the
series-ascending
branch
from
85f41db to
3a28948
Compare
|
All fixed hopefully. There is already a whatsnew line added. Is it the wrong file? |
|
no whatsnew is good (I may have been looking at something else). lgtm. ping on green. |
|
Sorry, it is my first patch ever here. What is green? How do I ping? The contributors document does not say. |
pandas/tests/series/test_sorting.py
Outdated
| @@ -64,6 +64,24 @@ def test_sort_values(self): | |||
| ordered = ts.sort_values(ascending=False, na_position='first') | |||
| assert_almost_equal(expected, ordered.valid().values) | |||
|
|
|||
| # ascending=[False] should behave the same as ascending=False | |||
| ordered = ts.sort_values(ascending=[False]) | |||
| expected = np.sort(ts.valid().values)[::-1] | |||
There was a problem hiding this comment.
sorry didn't notice this. Can you construct the result series and use tm.assert_series_equal for both of these. (literally construct it, don't sort things)
There was a problem hiding this comment.
Could you elaborate? I don't understand what you mean by constructing literally. Do you want me to create an example series for expected and then compare with ordered instead of creating it via np.sort? If you want to avoid NumPy, wouldn't it be better to simply compare the results of ascending=False and ascending=[False] to be _series_equal?
|
@MLopez-Ibanez another comment above. See those 'pending checks', when the CI runs those will eventually turn green (or red if there is an error). pinging is when everything is green, just say hey all green. (or really when you need a review if someone doesn't do it first). |
MLopez-Ibanez
force-pushed
the
series-ascending
branch
from
3a28948 to
6678574
Compare
Codecov Report
@@ Coverage Diff @@
## master #15607 +/- ##
==========================================
- Coverage 91.03% 91% -0.04%
==========================================
Files 137 143 +6
Lines 49309 49301 -8
==========================================
- Hits 44887 44864 -23
- Misses 4422 4437 +15
Continue to review full report at Codecov.
|
|
@jreback Hey all green! |
|
thanks @MLopez-Ibanez keep em coming! |
…alse (pandas-dev#15604) closes pandas-dev#15604 Author: manu <[email protected]> Closes pandas-dev#15607 from MLopez-Ibanez/series-ascending and squashes the following commits: 6678574 [manu] BUG: make Series.sort_values(ascending=[False]) behave as ascending=False (pandas-dev#15604)
git diff upstream/master | flake8 --diff