-
-
Notifications
You must be signed in to change notification settings - Fork 19.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!
WillAyd
left a comment
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
| 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
WillAyd
left a comment
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 |
jreback
left a comment
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 |
jreback
left a comment
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.
datapythonista
left a comment
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
| 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 pandasgit diff upstream/master -u -- "*.py" | flake8 --diff