DOC: Fix errors in pandas.Series.argmin #32286

farhanreynaldo · 2020-02-27T07:53:20Z

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 · 2020-02-27T17:58:17Z

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

simonjayhawkins · 2020-02-27T17:59:41Z

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

datapythonista · 2020-02-27T18:28:28Z

@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

…gmin-doc

farhanreynaldo · 2020-02-28T04:12:37Z

@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.

…gmin-doc

farhanreynaldo · 2020-02-29T11:14:12Z

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

datapythonista · 2020-02-29T13:22:29Z

@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.

…gmin-doc

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.

pandas/core/base.py

…gmin-doc

farhanreynaldo · 2020-03-02T13:30:04Z

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.

pandas/core/base.py

simonjayhawkins · 2020-03-03T12:11:15Z

@farhanreynaldo can you merge master to resolve conflicts

Co-Authored-By: Simon Hawkins <[email protected]>

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 · 2020-03-03T17:58:13Z

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

farhanreynaldo · 2020-03-04T01:00:47Z

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 · 2020-03-04T01:28:43Z

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

@simonjayhawkins all green now.

datapythonista · 2020-03-04T01:34:32Z

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.

…gmin-doc

farhanreynaldo · 2020-03-04T04:08:24Z

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 · 2020-03-04T07:44:40Z

Exactly, that's what I meant.

datapythonista · 2020-03-04T07:47:38Z

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

farhanreynaldo · 2020-03-04T08:07:12Z

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 · 2020-03-04T08:19:33Z

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 · 2020-03-04T08:23:57Z

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 · 2020-03-04T23:14:04Z

pandas/core/base.py

@@ -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",


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

datapythonista · 2020-03-04T23:15:58Z

pandas/core/base.py

-        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.


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 · 2020-03-04T23:16:16Z

pandas/core/base.py

@@ -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",


same as before about being a one-liner.

…gmin-doc

datapythonista · 2020-03-05T08:41:50Z

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 · 2020-03-05T09:24:26Z

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 · 2020-03-05T10:53:00Z

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

…gmin-doc

datapythonista · 2020-03-05T14:54:45Z

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

simonjayhawkins

Thanks @farhanreynaldo lgtm

datapythonista · 2020-03-05T16:40:53Z

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 · 2020-03-06T05:25:40Z

Actually I'm okay with that

datapythonista · 2020-03-06T12:31:08Z

Thanks for working on this @farhanreynaldo

Co-authored-by: Farhan Reynaldo Hutabarat <[email protected]> Co-authored-by: Simon Hawkins <[email protected]>

DOC: Fix errors in pandas.Series.argmin

b75fddc

simonjayhawkins added the Docs label Feb 27, 2020

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

827db07

…gmin-doc

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

ef13499

…gmin-doc

Farhan Reynaldo Hutabarat added 2 commits March 2, 2020 11:35

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

ae050f7

…gmin-doc

refactor argmax and argmin doc using @doc

353ce65

simonjayhawkins reviewed Mar 2, 2020

View reviewed changes

pandas/core/base.py Outdated Show resolved Hide resolved

pandas/core/base.py Outdated Show resolved Hide resolved

Farhan Reynaldo Hutabarat added 2 commits March 2, 2020 20:29

formatting using black pandas

3acc9dd

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

a516ee6

…gmin-doc

simonjayhawkins reviewed Mar 2, 2020

View reviewed changes

Farhan Reynaldo Hutabarat added 2 commits March 3, 2020 12:58

reduce parameter count from args

2bdd088

reduce parameter count from args

4de38a7

simonjayhawkins reviewed Mar 3, 2020

View reviewed changes

pandas/core/base.py Outdated Show resolved Hide resolved

simonjayhawkins added this to the 1.1 milestone Mar 3, 2020

farhanreynaldo and others added 2 commits March 3, 2020 22:39

Update pandas/core/base.py

1714e12

Co-Authored-By: Simon Hawkins <[email protected]>

Merge branch 'master' into argmin-doc

a2696a9

datapythonista approved these changes Mar 3, 2020

View reviewed changes

formatting by black pandas

7f055ba

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

a75a04a

…gmin-doc

combine examples and self-referencing

c89126a

remove argmax and argmin variables in See Also

04c197a

datapythonista approved these changes Mar 4, 2020

View reviewed changes

Farhan Reynaldo Hutabarat added 2 commits March 5, 2020 13:57

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

ed7a161

…gmin-doc

make doc decorator one-liner

183833a

Merge branch 'master' of https://github.com/pandas-dev/pandas into ar…

1699089

…gmin-doc

simonjayhawkins approved these changes Mar 5, 2020

View reviewed changes

datapythonista merged commit f6b3e82 into pandas-dev:master Mar 6, 2020

SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020

DOC: Fix errors in pandas.Series.argmin (pandas-dev#32286)

cdfcdf4

Co-authored-by: Farhan Reynaldo Hutabarat <[email protected]> Co-authored-by: Simon Hawkins <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Fix errors in pandas.Series.argmin #32286

DOC: Fix errors in pandas.Series.argmin #32286

farhanreynaldo commented Feb 27, 2020 •

edited

Loading

simonjayhawkins commented Feb 27, 2020

simonjayhawkins commented Feb 27, 2020

datapythonista commented Feb 27, 2020

farhanreynaldo commented Feb 28, 2020

farhanreynaldo commented Feb 29, 2020

datapythonista commented Feb 29, 2020

simonjayhawkins left a comment

farhanreynaldo commented Mar 2, 2020

simonjayhawkins left a comment

simonjayhawkins commented Mar 3, 2020

datapythonista left a comment

simonjayhawkins commented Mar 3, 2020

farhanreynaldo commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista left a comment

datapythonista Mar 4, 2020

datapythonista Mar 4, 2020

datapythonista Mar 4, 2020

datapythonista commented Mar 5, 2020

farhanreynaldo commented Mar 5, 2020

datapythonista commented Mar 5, 2020

datapythonista commented Mar 5, 2020

simonjayhawkins left a comment

datapythonista commented Mar 5, 2020

farhanreynaldo commented Mar 6, 2020

datapythonista commented Mar 6, 2020

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

DOC: Fix errors in pandas.Series.argmin #32286

DOC: Fix errors in pandas.Series.argmin #32286

Conversation

farhanreynaldo commented Feb 27, 2020 • edited Loading

simonjayhawkins commented Feb 27, 2020

simonjayhawkins commented Feb 27, 2020

datapythonista commented Feb 27, 2020

farhanreynaldo commented Feb 28, 2020

farhanreynaldo commented Feb 29, 2020

datapythonista commented Feb 29, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

farhanreynaldo commented Mar 2, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Mar 3, 2020

datapythonista left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Mar 3, 2020

farhanreynaldo commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista commented Mar 4, 2020

farhanreynaldo commented Mar 4, 2020

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista Mar 4, 2020

Choose a reason for hiding this comment

datapythonista Mar 4, 2020

Choose a reason for hiding this comment

datapythonista Mar 4, 2020

Choose a reason for hiding this comment

datapythonista commented Mar 5, 2020

farhanreynaldo commented Mar 5, 2020

datapythonista commented Mar 5, 2020

datapythonista commented Mar 5, 2020

simonjayhawkins left a comment

Choose a reason for hiding this comment

datapythonista commented Mar 5, 2020

farhanreynaldo commented Mar 6, 2020

datapythonista commented Mar 6, 2020

farhanreynaldo commented Feb 27, 2020 •

edited

Loading