-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
REF, TYP: factor out _mark_right from _add_legend_handle #40078
REF, TYP: factor out _mark_right from _add_legend_handle #40078
Conversation
6ddff42
to
bfacfd8
Compare
codecov/patch shows the lines from |
pandas/plotting/_matplotlib/core.py
Outdated
def _add_legend_handle(self, handle, label, index=None): | ||
def _mark_right_label( | ||
self, label: Optional[str], index: Optional[int] | ||
) -> Optional[str]: |
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.
can you add a doc-string
@fangchenli I think the coverage check here is failing because the slow plotting tests' coverage isn't currently uploaded to codecov, any suggestions for how to deal with that? Should |
Yes, enable coverage for the build that actually run those tests and upload the report. Codecov'll automatically merge multiple reports. xref #39758 |
codecov check passes now @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.
a minor comment on naming, could also keep as is
thanks @MarcoGorelli I think it's fine to merge! |
…40078) * wip * add docstring, simplify * remove redundant null check in _add_legend_handles * shorten dosctrings (these are only internal) * 🚚 _add_legend_handle -> _append_legend_handles_labels
…40078) * wip * add docstring, simplify * remove redundant null check in _add_legend_handles * shorten dosctrings (these are only internal) * 🚚 _add_legend_handle -> _append_legend_handles_labels
This PR factors out adding
(right)
to labels which are meant to go on the right y-axis. This does not change anything user-facing, and is simply a precursor to fixing #40044 and #39522 .The change is that on
master
, onlyleg.get_texts()
showsb (right)
whileax.right_ax.get_legend_handles_labels()
showsb
. The former is used for constructing the user-facing legend. The reason I think it needs doing is that I believe that to address #40044 and #39522, we need to use.get_legend_handles_labels()
for both handles and labels, while currently.get_legend_handles_labels
is used for handles andleg.get_texts()
is used for labels.Putting a breakpoint after
pandas/pandas/plotting/_matplotlib/core.py
Line 591 in ab687ae
results in , on this branch:
and on master: