-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: Fix Series.sort_values descending & mergesort #28698
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
Conversation
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.
it’s possible some existing test are off because this path is not exactly the same as nanargsort (but we do want to unify)
pandas/tests/series/test_sorting.py
Outdated
@@ -86,6 +86,12 @@ def test_sort_values(self): | |||
with pytest.raises(ValueError, match=msg): | |||
s.sort_values(inplace=True) | |||
|
|||
# mergesort and ascending=False |
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.
make a new test
doc/source/whatsnew/v0.25.2.rst
Outdated
@@ -102,6 +102,9 @@ Other | |||
- Compatibility with Python 3.8 in :meth:`DataFrame.query` (:issue:`27261`) | |||
- Fix to ensure that tab-completion in an IPython console does not raise | |||
warnings for deprecated attributes (:issue:`27900`). | |||
- Bug in :meth:`Series.sort_values` when ``ascending`` is set to ``False`` and |
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.
move to 1.0 this is a non trivial change
186c814
to
62af0bf
Compare
Yes that is the case. Now for quicksort, the ascending and descending sort are not guaranteed to be reversed versions of each other. n = 100000
np.random.seed(123)
values = np.random.randint(0, 4, n)
idx = np.arange(n)
s = pd.Series(values, index=idx)
asc = s.sort_values(ascending=True)
desc = s.sort_values(ascending=False)
asc.index.equals(desc.index[::-1]) # False ( without nanargsort it is True) Many tests for |
@has2k1 can you rebase |
The solution uses the `nargsort` function, which was created to handle in one place the intricacies of sorting. Not adapting Series.sort_values to use may have been an oversight. closes pandas-dev#28697
48fe4cb
to
ce1d2bf
Compare
Closing as this looks stale, but certainly @has2k1 ping if you'd like to pick back up |
The solution uses the
nargsort
function, which was created to handlein one place the intricacies of sorting. Not adapting Series.sort_values
to use may have been an oversight.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff