TST: Clean up tests of DataFrame.sort_{index,values}
State: closed

Before taking a stab at #10806, I looked at the relevant tests and they didn't make much sense to me.

This commit fixes:
- Some expected results were obtained by running code identical to what's being tested. (i.e. test could never fail)
- Removed duplicate assertions testing the same functionality both in `test_sort_index` and in `test_sort_values`.
- Switched the names `test_sort_index` and `test_sort_values` based on the (remaining) assertions being tested in their bodies.

Still confusing, unfixed:
- The `DataFrame.sort_index` docstring doesn't specify what happens when `level` is unspecified and `sort_remaining=False`. However there is a legacy test using this case: it's not clear to me what result or behavior is being tested.

First time pandas contributor, so I'd appreciate a good critical review!
- [X] tests pass
- [X] passes `git diff upstream/master | flake8 --diff`
- [x] ensure default options in docstrings


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Clean

Comments:
## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13496?src=pr) is **84.33%**

> Merging [#13496](https://codecov.io/gh/pydata/pandas/pull/13496?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will increase coverage by **<.01%**

``` diff
@@             master     #13496   diff @@
==========================================
  Files           138        138          
  Lines         51106      51100     -6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          43102      43097     -5   
+ Misses         8004       8003     -1   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [1a12ead...a333162](https://codecov.io/gh/pydata/pandas/compare/1a12eadaf92eb1ee05446d24e5d7a12cde971ce3...a333162cd10080fee441bacc7c3012fc7ab61719?src=pr)

ok, pls factor out `tests/series/tests_sorting.py` while you are at it. (in test_analytics).
Ideally make sure we have test coverage for all of the options for sort_values/index.

I think these tests were this way originally because before 0.17.0 sort did _both_ sort index/values in some cases.

looks good.

> The DataFrame.sort_index docstring doesn't specify what happens when level is unspecified and sort_remaining=False. However there is a legacy test using this case: it's not clear to me what result or behavior is being tested.

I think should do nothing. When you do not specify `level`, I think this means that you want to sort by all levels, and in this way `sort_remaining` (either true or false) has no effect anymore.

I think this is also what the test does? (the same result for both `sort_remaining` is True or False)

yes, the doc-string should have the default for `sort_remaining` and pls check the other params, a stock string should always list the default (even if `None`)

@jorisvandenbossche +1 to your interpretation. I might clarify the code comment in the test to explain.

@jreback Sure, I'll fix up the docstrings and also create `tests/series/tests_sorting.py` from test_analytics.

I could use guidance on a few small details:
1. Numpy Docstring Standard says just to mark optional kwargs as "optional", not to re-state their default value in the docstring. @jreback do you really want the defaults listed?
2. Is a whatsnew entry necessary for this change, which merely refactors tests without touching the core source?
3. The `sort_values` docstring is shared (`core/generic.py`) and therefore includes parameter "by" even for Series where it's not accepted. What's the right way to handle this discrepancy? Is it necessary to "un-share" the docstring?

> Numpy Docstring Standard says just to mark optional kwargs as "optional", not to re-state their default value in the docstring. @jreback do you really want the defaults listed?

yes, we do this for most other things, when you are reading a doc-string, its annoying to refer back to the signature. I am not sure we are very consistent in this though. @jorisvandenbossche 

> Is a whatsnew entry necessary for this change, which merely refactors tests without touching the core source?

In this case no, if its a 'big' test refactor then we usually want to note it

> The sort_values docstring is shared (core/generic.py) and therefore includes parameter "by" even for Series where it's not accepted. What's the right way to handle this discrepancy? Is it necessary to "un-share" the docstring?

Right way is to share, but add in `by` via string replacement specifically for `DataFrame` (to avoid repeating things more than necessary)

> > Numpy Docstring Standard says just to mark optional kwargs as "optional", not to re-state their default value in the docstring. @jreback do you really want the defaults listed?
> 
> yes, we do this for most other things, when you are reading a doc-string, its annoying to refer back to the signature. I am not sure we are very consistent in this though. @jorisvandenbossche 

Well, the question if how you define 'optional'. In principle are almost all keyword arguments optional (in the sense that you do not have to pass them explicitly), but in many cases they have a meaningful default value. And then it is indeed informative to note that as @jreback says.

But I would say, just make a change that you think is sensible, and we can always comment :-)

> add in `by` via string replacement specifically for `DataFrame`

@jreback Do you mean like this? https://github.com/pydata/pandas/pull/13496/commits/a12cea532c8db68641173150e6b984d359395ea3

(However now I'm wondering if "by" will after all become used for Series, `axis=1`, per #10806.)

Besides `by`, also `Series.sort_index` does not accept the kwargs `kind` and `na_position`. Until they get implemented I feel I should do the same sort of string replacement, although this begins to get unweildy.

> Do you mean like this?

Yes, that looks good

> (However now I'm wondering if "by" will after all become used for Series, axis=1, per #10806.)

No, I think that will also only be for dataframes (Series has no axis=1)

> Besides by, also Series.sort_index does not accept the kwargs kind and na_position. Until they get implemented I feel I should do the same sort of string replacement, although this begins to get unweildy.

Indeed, you can change that as well. 
But, it would not be too difficult to add those parameters to Series.sort_index (it's just changing the numpy argsort call with the internal wrapper `_nargsort` which adds those additional keywords. But if you want to do this, maybe keep that for a separate PR.

ping when ready with this

> Series.sort_index does not accept the kwargs kind and na_position

Opened #13589 for these missing features.

> make sure we have test coverage for all of the options for sort_values/index

My report on test coverage:
- `sort_values`'s tests for multiple by's were naive. I've improved this by adding duplicate values to the DataFrame being tested.
- `sort_index` was missing a test for multi-level `Series.sort_index(sort_remaining)`. I've added it.

I think this is about ready for merge, @jreback

very minor comments. pls post the new doc-strings, and ping on green.

@IamJeffG this looks good, except the doc-string on DataFrame.sort_values is looking a bit funny. The by is not indented properly.

@jreback see if my newest commit fixes this. I could verify the indentation problem using string replacement in the REPL, but I was unable to get docs to successfully build, to verify my fix end-to-end.

@IamJeffG Looks good now! (BTW, you can also just look at the docstring in the interactive console (`pd.DataFrame.sort_values?` in ipython) to check that it looks OK)

Thanks for the good tip @jorisvandenbossche!

@jreback We're green.

@IamJeffG Thanks a lot!

Interested in taking on https://github.com/pydata/pandas/issues/13589 as well?

Perhaps. Let me start with #10806, which was my initial goal.