Skip to content

ENH: Implement xlabel and ylabel options in Series.plot and DataFrame.plot #34223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 32 commits into from
Jun 26, 2020

Conversation

charlesdong1991
Copy link
Member

@charlesdong1991 charlesdong1991 commented May 17, 2020

@pep8speaks
Copy link

pep8speaks commented May 17, 2020

Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-25 15:57:49 UTC

@charlesdong1991 charlesdong1991 changed the title ENH: Add xlabel, ylabel options in plot ENH: Implement xlabel and ylabel options in Series.plot and DataFrame.plot May 17, 2020
@charlesdong1991 charlesdong1991 requested review from jreback and WillAyd and removed request for jreback May 20, 2020 17:29
Comment on lines 98 to 99
xlabel: Optional[str] = None,
ylabel: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to be string? Should work with e.g. int / float

Anyway, I tried running this and it looks good! Nice tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think it should be string? this is not calling any column of dataframe, but explicitly defined by users who want to give labels for x/y axis. I am not very sure about if users would name it using int/float, quite rare case though

I will later check to see if matplotlib allows to do so or not, if so, maybe we could support too! thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried running it passing an int, and it worked fine - but yeah, typical case would be to label with a str, so it's fine as is, sorry for the noise :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change to Label then and add a test case, so that we also support numbers!

Thanks for the feedback!

@charlesdong1991
Copy link
Member Author

maybe @WillAyd @jreback for a look?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good. Possible improvement on test, maybe unnecessary change for annotation but otherwise lgtm

@pytest.mark.parametrize(
"index_name, old_xlabel, new_xlabel, old_ylabel, new_ylabel",
[
(None, "", "new_x", "", ""),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge issue but instead of parametrizing xlabel / ylabel as individual arguments can you not just pass one set of labels and maybe have another parametrization on transpose to test the other axis? Would probably help readability

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, actually we can also remove the need of transpose, and simply use old_label and new_label instead. As commented in the PR, for default, ylabel should always be empty string, while xlabel be the index name. And if assigned, they will be overriden by assigned value.

Pls let me know your thoughts on the simplified tests! thanks! @WillAyd

@@ -153,8 +158,8 @@ def __init__(

self.grid = grid
self.legend = legend
self.legend_handles = []
self.legend_labels = []
self.legend_handles: List = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error does this solve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this solves mypy complaint, exactly the same as in #28373

@charlesdong1991
Copy link
Member Author

@WillAyd thanks for reviews, and changes are made accordingly

cc @jreback

@charlesdong1991 charlesdong1991 requested a review from WillAyd June 14, 2020 16:13
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm @jreback

@charlesdong1991 charlesdong1991 requested review from jreback and removed request for jreback June 20, 2020 09:00
@charlesdong1991
Copy link
Member Author

some further reviews are very appreciated! @jreback

@jreback jreback added this to the 1.1 milestone Jun 20, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code and tests look good, can you update the visualization.rst to add this in (and potentially update one or more existing examples).

@charlesdong1991
Copy link
Member Author

charlesdong1991 commented Jun 21, 2020

thanks for the review, @jreback , done! updated in visualization.rst and with an existing df used there as example.

@charlesdong1991 charlesdong1991 requested a review from jreback June 24, 2020 06:10
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. @datapythonista @TomAugspurger if any comments.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR @charlesdong1991, lgtm. Just couple of small things, but looks good.

# GH9093, currently Pandas does not show ylabel, so if users provide
# ylabel will set it as ylabel in the plot.
if self.ylabel is not None:
ax.set_ylabel(self.ylabel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this consistent with xlabel, which seems to pprint the value?

May be I'm wrong, but feels like df.plot(xlabel=a_complex_structure) will make a_complex_structure (may be a dict of lists) look "nice". While df.plot(ylabel=a_complex_structure) may fail, or just convert to string with str instead of pprint_thing.

Would be probably good to add a test in the parametrization to make sure this works as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call! i think matplotlib's ax.set_ylabel already could make the complex structure case look nice by converting to string automatically without using pprint_thing, e.g. inputs like [1, 2, 3] will be shown as '[1, 2, 3]' same in matplotlib plot.

But i think it might be indeed nicer to be consistent with xlabel so I added pprint_thing for ylabel as well, and add the test case in parametrization! thanks!

@@ -653,6 +653,16 @@ class PlotAccessor(PandasObject):
Set the x limits of the current axes.
ylim : 2-tuple/list
Set the y limits of the current axes.
xlabel : label, default None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
xlabel : label, default None
xlabel : label, optional

We usually use default None when the None is being used (imagine you have something like fill_with=None). When None means that the value is simply not provided, and the parameter won't be use, we use optional. A bit complex, but we've been somehow consistent with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, thats fair! changed


.. versionadded:: 1.1.0

ylabel : label, default None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ylabel : label, default None
ylabel : label, optional

.. versionadded:: 1.1.0

You may set the ``xlabel`` and ``ylabel`` arguments to give the plot custom labels
for x and y axis. By default, Pandas will pick up index name as xlabel, while leaving
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for x and y axis. By default, Pandas will pick up index name as xlabel, while leaving
for x and y axis. By default, pandas will pick up index name as xlabel, while leaving

We use pandas in lowercase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed!

@charlesdong1991
Copy link
Member Author

done. any further feedbacks are welcome!

cc @jreback @WillAyd @datapythonista

@jreback
Copy link
Contributor

jreback commented Jun 25, 2020

@datapythonista pls merge if good

@datapythonista datapythonista merged commit 998e2ab into pandas-dev:master Jun 26, 2020
@datapythonista
Copy link
Member

Thanks @charlesdong1991, nice PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add concise xlabel=, ylabel=, option in plot() method
6 participants