Repository: pandas-dev/pandas
Pull request: DOC: Fix #24268 by updating description for keep in Series.nlargest
Fix #24268 by updating the description of `keep` in Series.nlargest

- [x] closes #24268 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
DOC: Series.nlargest sorts index with duplicates
I think the description of this method is incorrect. The effect of parameter "keep" says, if we pass "first" to it, means to take the first duplicate value, while we pass "last" to it, means to take the last duplicate value.
However, its real effect is to sort the duplicate values by their indexes.
Hope we put it right soon.

Hints from issue discussion:
I don't think the docstring is wrong, but could be updated to note that the resulting index is sorted

```python
s = pd.Series([3, 3, 3, 2, 1], index=['a', 'b', 'c', 'd', 'e'])

s.nlargest(2, keep='first')
Out[27]: 
a    3
b    3
dtype: int64

s.nlargest(2, keep='last')
Out[28]: 
c    3
b    3
dtype: int64


# unsorted
s = pd.Series([3, 3, 3, 2, 1], index=['e', 'd', 'c', 'b', 'a'])
s.nlargest(2, keep='last')
Out[30]: 
c    3
d    3

```
can I try to fix it?
> can I try to fix it?

did you take the issue?
@chris-b1 I think the description for `keep` could be more clear. Currently it said:
```
            - ``first`` : take the first occurrences based on the index order
            - ``last`` : take the last occurrences based on the index order
```

The index order might not be the same as the order of the index of the Series if the Series is not ordered. 
For instance,
```python
# unsorted
s = pd.Series([3, 3, 3, 2, 1], index=['e', 'd', 'c', 'b', 'a'])
s.nlargest(2, keep='last')
Out[30]: 
c    3
d    3
```
I would naturally assume the index order for `keep='last'` would be `'d'` and `'e'` instead of `'c'` and `'d'` by thinking of ordering `index`. 
@thoo I think the description is correct insofar as it is order of the items in the index array. It's possible that it could be reworded to be a bit more clear like so:
```
            - ``first`` : return the first n occurrences in given index order
            - ``last`` : return the last n occurrences in reverse index order
```
@chris-b1 One thing that I noticed while looking into this is that when you call `s.nlargest(2,keep="last")` it doesn't return the items sorted by index, but in reverse order by index. For example:
```
>>>s = pd.Series([3, 1, 3, 2, 3,], index=['e', 'a', 'c', 'b', 'd'])
>>>s.nlargest(3, keep='last')
d    3
c    3
e    3
```
I feel as if this is a bug in it's own way and that it should instead be returning in the original index order i.e.
```
//should return
e    3
c    3
d    3
```

Commit message: Fix PEP8 issues

Patch to review:
diff --git a/pandas/core/series.py b/pandas/core/series.py
index a5dfe8d43c336..52be1dcddad6e 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -3100,8 +3100,10 @@ def nlargest(self, n=5, keep='first'):
             When there are duplicate values that cannot all fit in a
             Series of `n` elements:
 
-            - ``first`` : take the first occurrences based on the index order
-            - ``last`` : take the last occurrences based on the index order
+            - ``first`` : return the first `n` occurrences in the
+                given index order.
+            - ``last`` : return the last `n` occurrences in the
+                reverse of the given index order.
             - ``all`` : keep all occurrences. This can result in a Series of
                 size larger than `n`.
 
@@ -3196,8 +3198,10 @@ def nsmallest(self, n=5, keep='first'):
             When there are duplicate values that cannot all fit in a
             Series of `n` elements:
 
-            - ``first`` : take the first occurrences based on the index order
-            - ``last`` : take the last occurrences based on the index order
+            - ``first`` : return the first `n` occurrences in the
+                given index order.
+            - ``last`` : return the last `n` occurrences in the
+                reverse of the given index order.
             - ``all`` : keep all occurrences. This can result in a Series of
                 size larger than `n`.
 
@@ -3238,7 +3242,7 @@ def nsmallest(self, n=5, keep='first'):
         Monserat        5200
         dtype: int64
 
-        The `n` largest elements where ``n=5`` by default.
+        The `n` smallest elements where ``n=5`` by default.
 
         >>> s.nsmallest()
         Monserat      5200


Code review comments:
diff_hunk: @@ -3100,8 +3100,10 @@ def nlargest(self, n=5, keep='first'):
             When there are duplicate values that cannot all fit in a
             Series of `n` elements:
 
-            - ``first`` : take the first occurrences based on the index order
-            - ``last`` : take the last occurrences based on the index order
+            - ``first`` : return the first `n` occurrences in the
path: pandas/core/series.py
text: @user1:
Is "given index order" the correct description? I interpreted 
https://github.com/pandas-dev/pandas/issues/24268#issuecomment-446987354 as a modification on the *result*. So below the descriptions of first / last / all, I would note something like

    The `keep` parameter determines which ones to keep when there are duplicates.
    Regardless of `keep`, the result will be sorted by the row label.

@author:
I will include this, but when keep="last", the row labels are sorted in reverse order even in the examples in #24268 
And I feel "given index order" is correct because it is "given" by the user as input, if it is not an implicit RangeIndex

@user2:
I understand what you are writing here but I find the readability rather difficult. Any reason we don't use the same terminology in the `unique` docstring? 

`Uniques are returned in order of appearance`

^ obviously accounting for reversal with the `last` argument

@author:
@user2 What do you suggest me to do? If it's changing the `unique` docstring, I want to leave this as-is, because I still want to somehow emphasize the fact that index order is reversed when `keep="last"`

@user2:
My suggested wording is `Return the first n occurrences in order of appearance` for first and then `Return the last n occurrences in reverse order of appearance`