BUG: fix `Series.argsort` 
State: closed

- [x] closes #42056
- [x] closes #12694

rendered whatsnew:
![Screen Shot 2021-09-18 at 13 56 52](https://user-images.githubusercontent.com/24256554/133888309-cdfdec3d-7bb4-4758-88b5-258b89f0105c.png)



Labels:
name: Bug
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
Would this handle #12694?
> Would this handle #12694?

Yes this fixes that.

It solves it by:

 - keeping the '-1' for null values (so works for all dtypes), but doing the indexing correctly.
 - sorting the index for the return series so that there is meaning to its presence. 


@jorisvandenbossche since you commented on the underlying issue perhaps you would also like to review?
@attack68 thanks for looking into this topic!

Took a quick look, and some high-level comments:

- I agree that the current behaviour of the index doesn't seem very useful (and is confusing), but we shouldn't regard this as a "bug fix" if we completely change the output's index (that's a breaking change)
  - But further thinking about it, is the new behaviour actually ever useful? When would you want to use this index? If we change the behaviour of this method, we could maybe also consider returning an array instead of a Series (and then we don't have this question about what the index should be)
- For the NaN issue, you are now keeping the -1, but another option to fix this part could be to actually give this the proper index value that would sort the series correctly? (as numpy does)
> * I agree that the current behaviour of the index doesn't seem very useful (and is confusing), but we shouldn't regard this as a "bug fix" if we completely change the output's index (that's a breaking change)

I interpret a "bug-fix" as a breaking change that corrects erroneous behaviour, and as pointed out by others this function is currently unusable because of its errors:
 - one, the index values returned are wrong if interpreted as being the integer indexes over a Series, which contains Nans.
 - two, since values are re-ordered and the index is not re-ordered any index-value relationship  is broken and made completely meaningless - worse that if anyone interprets it as having meaning will lead to bugs in their program.

> But further thinking about it, is the new behaviour actually ever useful? When would you want to use this index? If we change the behaviour of this method, we could maybe also consider returning an array instead of a Series (and then we don't have this question about what the index should be)

My use of numpy argsort has has been to sort an array by the values of another similar shape array, which is just a kind of key-sorting: `array2[array1.argsort()]`, which was useful in a nearest neighbour algorithm as I recall.

Since numpy arrays don't have index objects per se this acts as an index sort. Since pandas data structures have index objects you can perform the same kind of method on objects with shared indexes with: `ser2[ser1.sort_values().index]`.
So argsort is not really necessary.

But, there are often different ways of doing things. And someone coming from numpy used to using `argsort` for this type of thing might instinctively look for same functionality. In this PR the following is now possible:

`ser2.iloc[ser1.argsort()]`  or `ser2[ser.argsort().index]`

I have added "first" and "last" to ``na_position`` so this does all now. 
@jreback @jorisvandenbossche this has been updated. Now has the following user defined action:

```python
s1 = pd.Series([30, 10, np.nan, 20, 25], index=["high", "low", "null", "mid-low", "mid-high"])
s1.argsort(na_position=na_position)
```

`na_position` | ... | ...
--- | --- | --- 
`None` | `"first"` | `"last"`
low 1<br>mid-low 3<br>mid-high 4<br>high 0<br>null -1 | null 2<br>low 1<br>mid-low 3<br>mid-high 4<br>high 0 | low 1<br>mid-low 3<br>mid-high 4<br>high 0<br>null 2

I am not sure we can actually make this change. The problem is these are meant to mirror `np.argsort` which gives the 'correct' results here.

in your last example can you show what `np.argsort` does to the values?
> I am not sure we can actually make this change. The problem is these are meant to mirror `np.argsort` which gives the 'correct' results here.
> 
> in your last example can you show what `np.argsort` does to the values?

The current implementation not only **doesn't mirror** `np.argsort`, but returns incorrect results, confuses the issue further by having an index which is meaningless, and a structure that is useless (having '-1' midway through the results values).

### Current Implementation

```python
>>> s1 = pd.Series([30, 10, np.nan, 20, 25], index=["high", "low", "null", "mid-low", "mid-high"])
>>> s1.argsort()
high        1
low         2
null       -1
mid-low     3
mid-high    0
dtype: int64
>>> np.argsort(s1.values)
array([1, 3, 4, 0, 2])
```

### This PR

My changes here can mirror `np.argsort` (with `na_position="last"`), works with all pandas null values and `dtypes`, has the added benefit of a useable index: (`na_position=None` provides a somewhat recognisable `pandas` format)

```python
>>> s1.argsort(na_position="last")
low         1
mid-low     3
mid-high    4
high        0
null        2
dtype: int64
``` 

@attack68 my point is that even though this is a bug, it is likely relied upon. So in an ideal situation we have `np.argsort(..)` and `.argsort` do the same thing. This change might be too big for a simple change and requires deprecation / hard break.
`.argsort` was changed relatively recently, can you link the issues and summarize what the problem / fix was?
> @attack68 my point is that even though this is a bug, it is likely relied upon. So in an ideal situation we have `np.argsort(..)` and `.argsort` do the same thing. This change might be too big for a simple change and requires deprecation / hard break.

OK, I understand, but by the same token keeping the function as is for the sake of reliability is a valid argument with existing users who have managed to work around it, but invalid with respect to new users of the function for whom it will be unreliable, e.g. the original poster of the underlying issue (not me).

Can I convert the PR to indicate a breaking change?
> > @attack68 my point is that even though this is a bug, it is likely relied upon. So in an ideal situation we have `np.argsort(..)` and `.argsort` do the same thing. This change might be too big for a simple change and requires deprecation / hard break.
> 
> OK, I understand, but by the same token keeping the function as is for the sake of reliability is a valid argument with existing users who have managed to work around it, but invalid with respect to new users of the function for whom it will be unreliable, e.g. the original poster of the underlying issue (not me).
> 
> Can I convert the PR to indicate a breaking change?

can you add a whatsnew note that shows the previous and new behavior; then it will be easier to assess
rendered whatsnew:
![Screen Shot 2021-09-18 at 13 56 52](https://user-images.githubusercontent.com/24256554/133888309-cdfdec3d-7bb4-4758-88b5-258b89f0105c.png)



@jorisvandenbossche perhaps you care to review the whatsnew above?
noting #43840
linking #45475 and #45434 for any relevance if anyone takes this forward
Appears that this PR has sufficiently stalled, so closing for now. Looks almost there is anyone wants to pick it up.
Is it worth reviving this for pandas 2.0?
As shown above the currenct argsort behiour is useless in some circumstances.
i think would either do
- remove .argsort entirely
- have it return a numpy array of indexers

it does the wrong thing now - while your PR does seems to fix it up - its essentially duplicating the sorting methods intent
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.