-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
ENH: Implement xlabel and ylabel options in Series.plot and DataFrame.plot #34223
Conversation
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 |
pandas/plotting/_matplotlib/core.py
Outdated
xlabel: Optional[str] = None, | ||
ylabel: Optional[str] = None, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
There was a problem hiding this 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
pandas/tests/plotting/test_frame.py
Outdated
@pytest.mark.parametrize( | ||
"index_name, old_xlabel, new_xlabel, old_ylabel, new_ylabel", | ||
[ | ||
(None, "", "new_x", "", ""), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
pandas/plotting/_matplotlib/core.py
Outdated
@@ -153,8 +158,8 @@ def __init__( | |||
|
|||
self.grid = grid | |||
self.legend = legend | |||
self.legend_handles = [] | |||
self.legend_labels = [] | |||
self.legend_handles: List = [] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm @jreback
some further reviews are very appreciated! @jreback |
There was a problem hiding this 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).
thanks for the review, @jreback , done! updated in |
There was a problem hiding this 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.
There was a problem hiding this 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.
pandas/plotting/_matplotlib/core.py
Outdated
# 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
pandas/plotting/_core.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, thats fair! changed
pandas/plotting/_core.py
Outdated
|
||
.. versionadded:: 1.1.0 | ||
|
||
ylabel : label, default None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed!
done. any further feedbacks are welcome! |
@datapythonista pls merge if good |
Thanks @charlesdong1991, nice PR. |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff