DOC: update the pandas.DataFrame.plot.pie docstring #20133

dim1tri · 2018-03-10T13:45:18Z

Checklist for the pandas documentation sprint (ignore this if you are doing
an unrelated PR):

PR title is "DOC: update the docstring"
The validation script passes: scripts/validate_docstrings.py pandas.DataFrame.plot.pie
The PEP8 style check passes: git diff upstream/master -u -- "*.py" | flake8 --diff
The html version looks good: python doc/make.py --single <your-function-or-method>
It has been proofread on language by another sprint participant

Please include the output of the validation script below between the "```" ticks:

Docstring for "pandas.DataFrame.plot.pie" correct. :)

Renton2017

Well the code is OK. I have a specific doubt on **kwds : optional. I would delete the asterisks and leave like kwds

Renton2017

Well the code is OK. I have a specific doubt on **kwds : optional. I would delete the asterisks and leave like kwds

dim1tri · 2018-03-10T14:10:38Z

@Renton2017, could you please make a reference to a line number?

dukebody · 2018-03-10T14:20:28Z

The keyword arguments must be mentioned in the docstring as **kwargs.

dukebody · 2018-03-10T14:24:24Z

Can you include a screenshot of the generated html?

Renton2017 · 2018-03-10T14:24:30Z

Hi Dimitri,

on line 2844 **kwds : optional
meanwhile on line 2849 kwds : optional

They cannot be true both!

dukebody · 2018-03-10T14:21:07Z

pandas/plotting/_core.py

+        y : str or int, optional
+            Label or position.
+            If not provided, ``subplots=True`` argument must be passed.
+        kwds : optional


Use **kwds : optional, not kwds : optional, even if the validation script complains about it.

dukebody · 2018-03-10T14:22:58Z

pandas/plotting/_core.py

-        `**kwds` : optional
+        y : str or int, optional
+            Label or position.
+            If not provided, ``subplots=True`` argument must be passed.


Really? What happens if we don't pass subplots=True and we don't pass a y argument? Does it raise an error? What does the subplots=True argument change?

Yes. This error is raised:
ValueError: pie requires either y column or 'subplots=True'
with subplots=True, pie() returns an array of plots for all columns

dukebody · 2018-03-10T14:23:29Z

pandas/plotting/_core.py

+
+        See Also
+        --------
+        :meth:`pandas.Series.plot.pie` : Generate a pie plot for a Serie.


pie plot for a Series, in plural

dukebody · 2018-03-10T14:25:34Z

pandas/plotting/_core.py

-            Column to plot.
-        `**kwds` : optional
+        y : str or int, optional
+            Label or position.


I find the Column to plot part important. What about Label of position indicating the column to plot?

dukebody · 2018-03-10T14:28:56Z

pandas/plotting/_core.py

+        Examples
+        --------
+
+        .. plot::


Can you add some text above explaining what is the example about in plain English? Also, perhaps you want to create different plots for different y values to show how the plot changes when changing the y argument.

pep8speaks · 2018-03-10T15:04:00Z

Hello @dim1tri! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 16, 2018 at 21:46 Hours UTC

jreback · 2018-03-10T15:51:32Z

pandas/plotting/_core.py

+
+        A pie plot is a representation of the distribution of numerical data
+        in a DataFrame column. This function wraps the `matplotlib.pyplot.pie`
+        function for specified column. If no column reference is passed and


for the specified column

jreback · 2018-03-10T15:51:49Z

pandas/plotting/_core.py

+
+        A pie plot is a representation of the distribution of numerical data
+        in a DataFrame column. This function wraps the `matplotlib.pyplot.pie`
+        function for specified column. If no column reference is passed and


refer to 'y' here (its the column reference)

jreback · 2018-03-10T15:51:58Z

pandas/plotting/_core.py

+        y : str or int, optional
+            Label or position of the column to plot.
+            If not provided, ``subplots=True`` argument must be passed.
+        **kwds : optional


no ** on kwds

@jreback there is a huge confusion here. @datapythonista said we DO want ** on kwds and they are modifying the validation script to accept it. Now you say we don't want it. Can we make a decision about this?

@jreback we decided to keep them

@jreback I thought the consensus was to have them. We've got a bug in the script that was validating the opposite, which created some confusion, but in the documentation we've got that they should have the ** in the name.

If there is no final decision, I'd accept both. I think it should be quite easy to use validate_docstrings.py to detect all the ones that have, or not have them. And we can quickly change all them later on.

dukebody · 2018-03-10T18:44:49Z

pandas/plotting/_core.py

+        in a DataFrame column. This function wraps the `matplotlib.pyplot.pie`
+        function for specified column. If no column reference is passed and
+        ``subplots`` argument is set to ``True``, an np.array of plots for
+        all DataFrame columns will be returned.


for all the DataFrame columns

jorisvandenbossche

Thanks for the PR! Added some comments

jorisvandenbossche · 2018-03-10T18:43:51Z

pandas/plotting/_core.py

+        A pie plot is a representation of the distribution of numerical data
+        in a DataFrame column. This function wraps the `matplotlib.pyplot.pie`
+        function for specified column. If no column reference is passed and
+        ``subplots`` argument is set to ``True``, an np.array of plots for


you can use single backticks for 'subplots' (because it is a reference to a parameter)

jorisvandenbossche · 2018-03-10T18:44:52Z

pandas/plotting/_core.py

-        y : label or position, optional
-            Column to plot.
-        `**kwds` : optional
+        y : str or int, optional


It's a bit a corner case as well, but in another PR we decide to keep the 'label', as 'str' can actually be too strict on the column name (column names can be other things as strings) ..

jorisvandenbossche · 2018-03-10T18:45:16Z

pandas/plotting/_core.py

            Keyword arguments to pass on to :py:meth:`pandas.DataFrame.plot`.

        Returns
        -------
-        axes : matplotlib.AxesSubplot or np.array of them
+        axes : matplotlib.AxesSubplot or np.array of them.


Can you explain here when an array is returned?

jorisvandenbossche · 2018-03-10T18:46:08Z

pandas/plotting/_core.py

+
+        See Also
+        --------
+        :meth:`pandas.Series.plot.pie` : Generate a pie plot for a Series.


You can leave out the :meth:` part here (the link is automatically made by numpydoc

I would also add the see also to matplotlib.pyplot.pie

jorisvandenbossche · 2018-03-10T18:46:33Z

pandas/plotting/_core.py

+
+            >>> df = pd.DataFrame({'mass': [0.330, 4.87 , 5.97],
+            ...                    'radius': [2439.7, 6051.8, 6378.1]})
+            >>> plot = df.plot.pie(y='mass', labels=['Mercury', 'Venus', 'Earth'])


Maybe also add an example with the subplots argument?

dukebody · 2018-03-10T18:50:20Z

pandas/plotting/_core.py

-        `**kwds` : optional
+        y : str or int, optional
+            Label or position of the column to plot.
+            If not provided, ``subplots=True`` argument must be passed.


Improvement proposal: If not provided, the ``subplots=True`` argument must be passed. In this case, a subplot will be created for every column of the DataFrame.

dukebody · 2018-03-10T18:54:32Z

pandas/plotting/_core.py

+
+        Examples
+        --------
+        In the example below we have a DataFrame with the information about


Perhaps a matter of style, but I'd say:

Given a DataFrame with information about some planets mass and radius, create a pie plot of their masses:

Because the fact that it is an example can be determined from the section it is in, and the fact that we pass the 'mass' column to the pie method can be seen in the code itself.

What about another example without specifying the y argument?

dukebody · 2018-03-11T11:10:41Z

@dim1tri please ping me if you have issues fixing the merge conflicts.

dukebody · 2018-03-13T07:42:46Z

@dim1tri is there something we can do to help you with this PR?

* Aspect Ratio * Use index [ci skip]

codecov · 2018-03-16T21:44:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3561580). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #20133   +/-   ##
=========================================
  Coverage          ?   91.72%           
=========================================
  Files             ?      150           
  Lines             ?    49152           
  Branches          ?        0           
=========================================
  Hits              ?    45086           
  Misses            ?     4066           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.11% <ø> (?)`
#single	`41.84% <ø> (?)`

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`82.23% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3561580...c49fbb3. Read the comment docs.

TomAugspurger · 2018-03-16T21:46:53Z

Updated

Fixed the aspect ratio on the plots, added a subplots=True example, and used the index instead of labels.

jorisvandenbossche · 2018-03-17T09:44:49Z

@dim1tri Thanks!

SimonBaron · 2019-03-12T10:30:36Z

I have wanted to ask for a while, but never sure where.. Is this documentation page a joke? Is it intentionally trolling? It is, right?

Renton2017 reviewed Mar 10, 2018

View reviewed changes

Renton2017 approved these changes Mar 10, 2018

View reviewed changes

dukebody reviewed Mar 10, 2018

View reviewed changes

DOC: update the pandas.DataFrame.plot.pie docstring

ad5fb29

jreback added Docs Visualization plotting labels Mar 10, 2018

jreback requested changes Mar 10, 2018

View reviewed changes

dukebody reviewed Mar 10, 2018

View reviewed changes

jorisvandenbossche reviewed Mar 10, 2018

View reviewed changes

dukebody reviewed Mar 10, 2018

View reviewed changes

Updated [ci skip]

b7c98bb

* Aspect Ratio * Use index [ci skip]

Merge remote-tracking branch 'upstream/master' into dim1tri-master

c49fbb3

TomAugspurger added this to the 0.23.0 milestone Mar 16, 2018

TomAugspurger approved these changes Mar 16, 2018

View reviewed changes

jorisvandenbossche approved these changes Mar 17, 2018

View reviewed changes

jorisvandenbossche merged commit e9a6735 into pandas-dev:master Mar 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: update the pandas.DataFrame.plot.pie docstring #20133

DOC: update the pandas.DataFrame.plot.pie docstring #20133

dim1tri commented Mar 10, 2018

Renton2017 left a comment

Renton2017 left a comment

dim1tri commented Mar 10, 2018

dukebody commented Mar 10, 2018

dukebody commented Mar 10, 2018

Renton2017 commented Mar 10, 2018

dukebody Mar 10, 2018

dukebody Mar 10, 2018

dim1tri Mar 10, 2018

dukebody Mar 10, 2018

dukebody Mar 10, 2018

dukebody Mar 10, 2018

pep8speaks commented Mar 10, 2018 •

edited

Loading

jreback Mar 10, 2018

jreback Mar 10, 2018

jreback Mar 10, 2018

dukebody Mar 10, 2018

jorisvandenbossche Mar 10, 2018

datapythonista Mar 10, 2018

dukebody Mar 10, 2018

jorisvandenbossche left a comment

jorisvandenbossche Mar 10, 2018

jorisvandenbossche Mar 10, 2018

jorisvandenbossche Mar 10, 2018

jorisvandenbossche Mar 10, 2018

jorisvandenbossche Mar 10, 2018

dukebody Mar 10, 2018

dukebody Mar 10, 2018

dukebody commented Mar 11, 2018

dukebody commented Mar 13, 2018

codecov bot commented Mar 16, 2018 •

edited

Loading

TomAugspurger commented Mar 16, 2018

jorisvandenbossche commented Mar 17, 2018

SimonBaron commented Mar 12, 2019

DOC: update the pandas.DataFrame.plot.pie docstring #20133

DOC: update the pandas.DataFrame.plot.pie docstring #20133

Conversation

dim1tri commented Mar 10, 2018

Renton2017 left a comment

Choose a reason for hiding this comment

Renton2017 left a comment

Choose a reason for hiding this comment

dim1tri commented Mar 10, 2018

dukebody commented Mar 10, 2018

dukebody commented Mar 10, 2018

Renton2017 commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Mar 10, 2018 • edited Loading

Comment last updated on March 16, 2018 at 21:46 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukebody commented Mar 11, 2018

dukebody commented Mar 13, 2018

codecov bot commented Mar 16, 2018 • edited Loading

Codecov Report

TomAugspurger commented Mar 16, 2018

jorisvandenbossche commented Mar 17, 2018

SimonBaron commented Mar 12, 2019

pep8speaks commented Mar 10, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading