DOC: Fix Series nsmallest and nlargest docstring/doctests #22731
Conversation
|
Hello @Moisan! Thanks for updating the PR.
Comment last updated on September 18, 2018 at 01:46 Hours UTC |
| - ``first`` : take the first occurrences based on the index order | ||
| - ``last`` : take the last occurrences based on the index order | ||
| - ``all`` : keep all occurrences. This can result in a Series of | ||
| size larger than `n`. |
There was a problem hiding this comment.
Is the period here on the last bullet required to pass the docstring validation as-is? Shouldn't be necessary but if that's the intent here just something we should address separately @datapythonista
There was a problem hiding this comment.
I confirm that the validation fails if the last period is not present.
| Brunei 434000 | ||
| dtype: int64 | ||
| >>> s.nlargest(3) |
There was a problem hiding this comment.
I think it would be nice to have just a quick one-liner to highlight the difference between this and the subsequent example
There was a problem hiding this comment.
Do you mean a comment at the end of the line?
There was a problem hiding this comment.
No just some text in between the examples to call out what the user should be looking at
pandas/core/series.py
Outdated
| Brunei 434000 | ||
| dtype: int64 | ||
| The n largest elements where n=3 with all duplicates kept. Note that the |
There was a problem hiding this comment.
Looks like this line is too long: https://travis-ci.org/pandas-dev/pandas/jobs/429733867#L3699
pandas/core/series.py
Outdated
| Monserat 5200 | ||
| dtype: int64 | ||
| The n largest elements where n=5 by default. |
There was a problem hiding this comment.
single backticks around n. Double backticks around n=5.
pandas/core/series.py
Outdated
| Brunei 434000 | ||
| dtype: int64 | ||
| The n largest elements where n=3. Default keep value is 'first' so |
There was a problem hiding this comment.
Same. Single backticks around keep.
pandas/core/series.py
Outdated
| The n largest elements where n=3. Default keep value is 'first' so | ||
| Malta will | ||
| be kept. |
There was a problem hiding this comment.
Can be on the same line as "Malta will"
pandas/core/series.py
Outdated
| Malta 434000 | ||
| dtype: int64 | ||
| The n largest elements where n=3 and keeping the last duplicates. |
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Think this is a nice update so lgtm
datapythonista
left a comment
datapythonista
left a comment
There was a problem hiding this comment.
Great changes, suggested couple of small things, but other than that looks really good.
pandas/core/series.py
Outdated
| Parameters | ||
| ---------- | ||
| n : int |
There was a problem hiding this comment.
can you add the default here?
pandas/core/series.py
Outdated
| - ``last`` : take the last occurrence. | ||
| n : int, default 5 | ||
| Return this many descending sorted values. | ||
| keep : str, default 'first' |
There was a problem hiding this comment.
I think it's better to keep {'first', 'last', 'all'}, as I don't think there is any other value allowed. That applies to both docstrings.
| See Also | ||
| -------- | ||
| Series.nsmallest | ||
| Series.nsmallest: Get the `n` smallest elements. |
There was a problem hiding this comment.
I'd add sort_values and head here, as they're mentioned in the notes. In both docstrings.
datapythonista
left a comment
datapythonista
left a comment
There was a problem hiding this comment.
lgtm. Excellent work @Moisan, thank you!
|
Passed on travis. Merging. |
5 participants
git diff upstream/master -u -- "*.py" | flake8 --diffBased on #22459. Fix the docstring for Series.nsmallest and Series.nlargest. I did both together since the same example could be used. I removed both from the skipped doctests in
ci/doctests.sh.I also found out about the "all" option for the
keepparameter which was surprisingly not documented.