Skip to content
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

BUG: pd.Series.argsort misaligned index #42056

Open
2 of 3 tasks
cericson opened this issue Jun 16, 2021 · 4 comments
Open
2 of 3 tasks

BUG: pd.Series.argsort misaligned index #42056

cericson opened this issue Jun 16, 2021 · 4 comments
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug

Comments

@cericson
Copy link

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • (optional) I have confirmed this bug exists on the master branch of pandas.


Code:

series = pd.Series([30, 10, 20], index=['c', 'a', 'b'])
order = series.argsort()
order

Output:

c    1
a    2
b    0
dtype: int64

Problem description

It appears that the argsort output str index order (c, a, b) remains the same as the input data, while the numeric indices (1, 2, 0) are referring to the input data in a different order. This means the str index no longer corresponds with the values (the numeric indices) in the resulting Series. This behavior is confusing, since it sort of looks like c would be the 1st element (0-indexed), a the 2nd, and b the 0th, which is incorrect.

There's actually another example of this here: https://www.geeksforgeeks.org/python-pandas-series-argsort/. The str indices in the result of the argsort don't give any useful information - only the order of the numeric indices is relevant.

Expected Output

I'm not sure if it would make more sense if str indices were reordered to match the numeric indices, like this:

a    1
b    2
c    0
dtype: int64

This might be nice, for example, to get the indices of the largest N elements: order.index[-N:]. The common workflow series[series.argsort()] still seems to work, but I'm not sure of all the other indexing implications.

Output of pd.show_versions()

INSTALLED VERSIONS

commit : 2cb9652
python : 3.7.5.final.0
python-bits : 64
OS : Linux
OS-release : 3.10.0-1160.25.1.el7.x86_64
Version : #1 SMP Tue Apr 13 18:55:45 EDT 2021
machine : x86_64
processor : x86_64
byteorder : little
LC_ALL : None
LANG : C.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.2.4
numpy : 1.18.5
pytz : 2021.1
dateutil : 2.8.1
pip : 21.1
setuptools : 56.0.0
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : 2.11.3
IPython : 7.22.0
pandas_datareader: None
bs4 : None
bottleneck : None
fsspec : None
fastparquet : None
gcsfs : None
matplotlib : 3.4.1
numexpr : None
odfpy : None
openpyxl : 3.0.7
pandas_gbq : None
pyarrow : None
pyxlsb : None
s3fs : None
scipy : 1.6.3
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
xlwt : None
numba : 0.53.1

@cericson cericson added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 16, 2021
@attack68
Copy link
Contributor

With a Series you are, mathematically, associating an index label with a value in a pair, such as in:

series = pd.Series([30, 10, np.nan, 20], index=['high', 'low', 'none', 'mid'])
high  30
low   10
none  nan
mid   20

If you were to sort this series naturally you use the value key and maintain the index as part of a pair:

low  10
mid  20
high 30
none  nan

When you perform argsort on a series you return a different object, a list of integer indexes that would sort those associations by their value. I.e. you input a list of pairs and returned a list of single integer values, so:

series.argsort()  # should really return just [1,3,0,2] where 'nans' are placed at end
high  1
low  2
none  -1
mid   0

The fact that it doesn't do this raises several issues for me:

  1. The obvious problem here is that the answer is incorrect, index 2 of series is actually nan, and index 3 (with value 20) is not even included. (this is the result of the method using a mask to split nans and real values an therefore loses index positions over the whole)

  2. The index label is maintained which breaks the pair associations and is meaningless, and as you highlight, confusing.

  3. Since the index is maintained and nans are set to -1 this means you get a -1 in the middle of the series which breaks the function of argsort.

I would propose that the Series.argsort method is re-written to:

values = self.to_numpy()
res = np.argsort(values, kind=kind)
res_ser = Series(res, index=self.index[res], dtype="int64", name=self.name)
mask = isna(values)
res_ser[self.index[mask]] = -1  # set na tp -1
return res_ser.__finalize__(self, method="argsort")

With that the following the results are obtained:

series.argsort()
low     1
mid     3
high    0
none   -1

where the index has been sorted according to the argsort, so this series has a new meaning:
(index-label, original integer-index label) and the ordering of those pairs is based on the value from (index-label, value).

@attack68 attack68 added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 17, 2021
@attack68 attack68 mentioned this issue Jun 17, 2021
2 tasks
@jbrockmendel
Copy link
Member

Might make sense to return an ndarray instead of a Series?

@attack68
Copy link
Contributor

well jeff suggested just remove the method.
If its between returning an ndarray or removing, I'd just remove it.
Dont know how ndarray will work with non ndarray items like .

pd Argsort is useless when handling missing data. The above PR fixed all problems and had a better index, but it didnt gain any traction. Only a few people over the years seem to have noticed how bad this method is, and those that did just stopped using it.

@jbrockmendel
Copy link
Member

I'd be fine with deprecating and telling users to use ser.array.argsort instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants