DOC: Fix errors in pandas.Series.argmin

[farhanreynaldo]

• closes DOC: Fix errors in pandas.Series.argmin pandanistas/pandanistas_sprint_jakarta2020#19
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

output of python scripts/validate_docstrings.py pandas.Series.argmin:

################################################################################
################################## Validation ##################################
################################################################################

[simonjayhawkins]

Thanks @farhanreynaldo can you merge master. failing CI should be fixed.

[simonjayhawkins]

@datapythonista should we be sharing the bulk of the docstring with Series.argmax, xref #32019

[datapythonista]

@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 decorator to help with that. The general idea if I'm not wrong of the exact syntax is:

@doc(operation='maximum')
def argmax():
    """
    This is computing the {operation}.
    """
    pass

@doc(argmax, operation='minimum')
def argmin():
    pass

The @doc decorator was recently introduced in #31060, the diff of that PR can help you understand. If you have any question, or you have any idea for improvement given this use case, feel free to ping @HH-MWB

[farhanreynaldo]

@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 decorator to help with that. The general idea if I'm not wrong of the exact syntax is:

@doc(operation='maximum')
def argmax():
    """
    This is computing the {operation}.
    """
    pass

@doc(argmax, operation='minimum')
def argmin():
    pass

The @doc decorator was recently introduced in #31060, the diff of that PR can help you understand. If you have any question, or you have any idea for improvement given this use case, feel free to ping @HH-MWB

Sure, let me figure it out first.

[farhanreynaldo]

@datapythonista should I do another pull request regarding the changes for pandas.Series.argmax doc?

[datapythonista]

@datapythonista should I do another pull request regarding the changes for pandas.Series.argmax doc?

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.

[simonjayhawkins]

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.

[farhanreynaldo]

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 black pandas

[simonjayhawkins]

Thanks @farhanreynaldo , could perhaps reduce the parameter count for clarity.

[simonjayhawkins]

@farhanreynaldo can you merge master to resolve conflicts

[datapythonista]

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.

[simonjayhawkins]

@farhanreynaldo can you run black to fix ci. ping on green.

[farhanreynaldo]

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.

Yeah, I think it would be nice to put both examples in the documentation, but then we should add another parameter for example values in doc arguments. Is that okay?

[farhanreynaldo]

@farhanreynaldo can you run black to fix ci. ping on green.

@simonjayhawkins all green now.

[datapythonista]

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.

[farhanreynaldo]

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?

See Also
--------
Series.arg{op} : Return position of the {op}imum value.
numpy.ndarray.arg{op} : Equivalent method for numpy arrays.
Series.arg{oppose} : Similar method, but returning the {oppose}imum.
Series.idxmax : Return index label of the maximum values.
Series.idxmin : Return index label of the minimum values.

and for the examples

Examples
--------
Consider dataset containing cereal calories

>>> s = pd.Series({{'Corn Flakes': 100.0, 'Almond Delight': 110.0,
...                'Cinnamon Toast Crunch': 120.0, 'Cocoa Puff': 110.0}})
>>> s
Corn Flakes              100.0
Almond Delight           110.0
Cinnamon Toast Crunch    120.0
Cocoa Puff               110.0
dtype: float64

>>> s.argmax()
2

>>> s.argmin()
0

The maximum cereal calories is the third element and 
the minimum cereal calories is the first element,
since series is zero-indexed. 

[datapythonista]

Exactly, that's what I meant.

[datapythonista]

And for see also you could also repeat argmin and argmax and avoid the variables.

[farhanreynaldo]

And for see also you could also repeat argmin and argmax and avoid the variables.

Could we keep the variables in the See Also section so the order of the reference consistent with its function?
e.g pandas.Series.argmin documentation would have its function appear first in the See also section

[datapythonista]

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.

[farhanreynaldo]

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.

[datapythonista]

Thanks @farhanreynaldo, looks great. Just couple of minor things that I think can still be improved, but nice work.

[datapythonista]

I guess this is from black, but I guess you can leave the decorator in a single line, and black will respect it.

[datapythonista]

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.

Suggested change

	Series.argmin : Return position of the minimum value.
	Series.arg{oppose} : Return position of the {oppose}imum value.

[datapythonista]

same as before about being a one-liner.

[datapythonista]

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.

[farhanreynaldo]

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!

[datapythonista]

If you can merge master, CI should be green now.

[datapythonista]

@simonjayhawkins do you want to have another look? this should be ready now

[simonjayhawkins]

Thanks @farhanreynaldo lgtm

[datapythonista]

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.

[farhanreynaldo]

Actually I'm okay with that

[datapythonista]

Thanks for working on this @farhanreynaldo