DOC: Fix errors in pandas.Series.argmin #32286
Conversation
|
Thanks @farhanreynaldo can you merge master. failing CI should be fixed. |
|
@datapythonista should we be sharing the bulk of the docstring with Series.argmax, xref #32019 |
Yes, I think that makes sense. @farhanreynaldo the idea here is instead of repeating the same in both docstrings, to reuse from one to the other. We have a @doc(operation='maximum')
def argmax():
"""
This is computing the {operation}.
"""
pass
@doc(argmax, operation='minimum')
def argmin():
passThe |
Sure, let me figure it out first. |
|
@datapythonista should I do another pull request regarding the changes for |
I think a single PR with the changes to reuse the docstring is what makes more sense. Whether you reuse this PR, or you close this an open a new one doesn't make a difference for us. |
There was a problem hiding this comment.
Thanks @farhanreynaldo for the updates.
a couple of ci failues.. you'll need to run black, https://pandas.io/docs/development/contributing.html#python-pep8-black
the also the doc decorator parameters should be strings.
Sure, I will run |
There was a problem hiding this comment.
Thanks @farhanreynaldo , could perhaps reduce the parameter count for clarity.
|
@farhanreynaldo can you merge master to resolve conflicts |
Co-Authored-By: Simon Hawkins <simonjayhawkins@gmail.com>
There was a problem hiding this comment.
Great job @farhanreynaldo
For the examples, we would usually have both the minimum and the maximum in both examples, which may be easier and it makes sense. And even for the see also section we often have both (even if one is self-referencing the current page).
But looks good like this too.
|
@farhanreynaldo can you run black to fix ci. ping on green. |
Yeah, I think it would be nice to put both examples in the documentation, but then we should add another parameter for |
@simonjayhawkins all green now. |
|
The idea is not to add another parameter but to remove the ones you have now for the examples. If you have the max first and the min later you don't need any variable. |
Oh I see, we could reduce the parameter. Is this what you are looking for? and for the examples |
|
Exactly, that's what I meant. |
|
And for see also you could also repeat argmin and argmax and avoid the variables. |
Could we keep the variables in the |
|
I'm ok with that. There is a trade off between keeping things simple in the code, and having each docstring with its own links, examples... In general I think it's better to keep things simpler for us if the docstrings are useful, but not really important in this case, I think any option is good. |
I think I'm just gonna follow your lead. |
There was a problem hiding this comment.
Thanks @farhanreynaldo, looks great. Just couple of minor things that I think can still be improved, but nice work.
pandas/core/base.py
Outdated
| @@ -927,16 +927,19 @@ def max(self, axis=None, skipna=True, *args, **kwargs): | |||
| nv.validate_max(args, kwargs) | |||
| return nanops.nanmax(self._values, skipna=skipna) | |||
|
|
|||
| @doc( | |||
| op="max", oppose="min", value="largest", | |||
There was a problem hiding this comment.
I guess this is from black, but I guess you can leave the decorator in a single line, and black will respect it.
pandas/core/base.py
Outdated
| numpy.ndarray.argmax : Equivalent method for numpy arrays. | ||
| Series.argmin : Similar method, but returning the minimum. | ||
| Series.argmax : Return position of the maximum value. | ||
| Series.argmin : Return position of the minimum value. |
There was a problem hiding this comment.
You're not finally using oppose. I think you can replace those two lines by this, and we don't need to repeat the same page in this case. Or you can simply remove the oppose parameter of doc otherwise.
| Series.argmin : Return position of the minimum value. | |
| Series.arg{oppose} : Return position of the {oppose}imum value. |
pandas/core/base.py
Outdated
| @@ -1019,25 +1026,10 @@ def min(self, axis=None, skipna=True, *args, **kwargs): | |||
| nv.validate_min(args, kwargs) | |||
| return nanops.nanmin(self._values, skipna=skipna) | |||
|
|
|||
| @doc( | |||
| argmax, op="min", oppose="max", value="smallest", | |||
There was a problem hiding this comment.
same as before about being a one-liner.
|
Thanks for the fixes. The error in the CI is caused by a new version of Matplotlib being released, and will be fixed after #32444 is merged. |
Thank you for the input throughout this pull request, @datapythonista and @simonjayhawkins! |
|
If you can merge master, CI should be green now. |
|
@simonjayhawkins do you want to have another look? this should be ready now |
|
I think we should merge this. @farhanreynaldo looks like GitHub deployed some changes last night, were they consider (in the git history) that the person merging the PR is the author, and not you :( See #32457 for the full discussion. |
|
Actually I'm okay with that |
|
Thanks for working on this @farhanreynaldo |
Co-authored-by: Farhan Reynaldo Hutabarat <farhan.hutabarat@dattabot.io> Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffoutput of
python scripts/validate_docstrings.py pandas.Series.argmin: