BUG: allow missing values in Index when calling Index.sort_values
State: closed

- [X] closes #35584, xref #35922 
- [X] tests added 1 / passed 1
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [X] whatsnew entry

### Problem

`Index.sort_values` breaks when `Index` includes `None`. The OP points out that a `Series` doesn't break in a similar case. In my opinion, there is a number of ways missing values can creep into an Index, and `sort_values` shoudn't be breaking.

### Details

When we do `Series.sort_values` we shunt the missing values to the end or the beginning of the Series depending on `na_position`, and then `argsort` the rest. When we currently do `sort_values` on an `Index`, we try to `argsort` the whole thing, and it expectedly breaks.

### Solution and alternatives

I propose we do the same stuff for `Index` that `Series` does, that is send the missing values to the end or the beginning. Added the `na_position` kwarg that works similar to how it works in `Series`.

Another possible solution is to explicitly forbid Indices with missing values and raise a clear error. I don't think it's advisable, since there isn't much of a practical difference between non-unique indices, which we allow, and this case.


Labels:
name: Bug
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Index
description: Related to the Index class or subclasses

Comments:
Couldn't find a better home for the test than `test_common.py` in indixes. Maybe there is a better place for it?
Exempted MultiIndex from this fix, as it's completely unclear where you should put missing values in this case.
Hm. There are a bunch of tests that expect NaNs to be at the beginning. Will have to add `na_position` to the function signature.
I think this is ready for review. Since this PR changes the API, I put it in 1.2.0 whatsnew.

Hello @AlexKirko! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:











There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-09-06 04:24:45 UTC
For some weird reason, we expect different behavior from `Index.sort_values` and `Series.sort_values` when `ascending=False` and the object contains duplicates. I'll give more details once I figure out a candidate solution that passes the tests without altering the expectation of this behavior.
Guess I'll limit this PR to implementing missing values support for `Index.sort_values` using `nargsort`.

Switching the Series.sort_values to it, I think, belongs in another issue and PR, along with picking a convention for sorting duplicates and enforcing it across our test suite.
> Guess I'll limit this PR to implementing missing values support for `Index.sort_values` using `nargsort`.
> 
> Switching the Series.sort_values to it, I think, belongs in another issue and PR, along with picking a convention for sorting duplicates and enforcing it across our test suite.

yes absolutely. after this PR is merged we *for sure* want to combine implementations (there even might be some open issues about this); this impacted adding the key arg as well.
Great, I'll make the changes tomorrow.

Made the changes to the tests, all green. Please take a look.

I wonder what's up with the test fails on Windows. Looks unrelated, will look into it next morning.
@jreback 
Added a fixture, reworked the tests. All green, please take a look whether this is what you had in mind.
Numpy dev fails now, but I don't see Index.sort_values being called in the failing tests. Will investigate why this is happening.
So far unable to reproduce the np-dev test failures on my local machine (including using a numpy-dev environment on Windows AND Ubuntu).

Will probably have to revert everything and reintroduce incrementally to track down what makes the np-dev pipeline fail.

Sorry for the delay.
This is super-weird, but I figured out why the np-dev pipeline is failing. For some reason `test_union_different_types` in `test_setops.py' pulls my new fixture instead of `index` and `index_fixture2`. This, as far as I can tell, is a bug that's specific to this pipeline (can't reproduce it locally on Ubuntu 18.04 and numpy-dev environment).

Now that I know what's going on, I should be able to circumvent it.

UPDATE: deep-copying the inde fixes it. Still weird.
Waiting for the mypy fix (#36085). The `py37_np16` appears to be unrelated too, will look into it.
@jreback 
Finally all green, please take another look.
Apologies for the delay: ran into an elusive bug with `Index.copy` on one of the pipelines, which I wasn't able to reproduce. Took a while to track down and circumvent.
@jreback 
Changes made, all green.
thanks @AlexKirko very nice! happy to take more to consolidate the Index/Series code for this :->
Thanks!
A bit sick atm, but I'll get back to this in a few days with a new PR.