Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: allow fig.legend outside axes...#19743
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
We should perhaps do a call to hash out the API decisions both for this and for#12679 once and for all... |
Ok but this pr is completely agnostic of how the legend position is specified. If we later decide to do something with compass notation that doesn't change this PR at all. |
c6a9869
to9c577ae
Comparetimhoffm commentedMar 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The cheatsheet way of doing things with bbox_to_anchor is basically what this seeks to avoid. Hopefully we can all agree that getting A, on the left side, by saying Here the proposal is: A: loc="left", outside='upper' Note that if the user specifies I'm not wedded to this API, but actually feel (pretty strongly) that it is the simplest way forward, and doesn't lock us out of a different string-based API... Yes, axes.legend can get outside placement, but lets save that for a separate PR. |
... rebased |
I'll advocate for this one as often-desired, and I'll also advocate for "lets keep it simple" |
Tobychev commentedJun 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jklymak Could the API be simplified by just having separate location specification for outside placement? so you either specify loc, or outside, but not both? Also adding the two specifiers "top" and "bottom" then results in the following API: As the illustration makes clear, in the outside case there is a degeneracy/ambiguity between the two sides of the corners that the current loc position names doesn't have to deal with. To me it seems reasonable that a clear resolution of this ambiguity is solved at the cost of introducing two new (relative) position specifiers. |
efiring commentedDec 1, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I like the idea of separate kwargs for inside versus outside. For the outside naming, maybe specify row or column first, position within row or column second: It's far from perfect, but I think this is a better match to the way I might describe a location around the periphery of the box, without consulting a diagram every time. Another option would be to ditch the separate "left top" and "top left" and just use compass locations and names for everything. Inside or outside, there are just 4 corner locations, "NE", "SE", etc. Simple, concise, and unambiguous. |
1ff75d5
tof5e39a1
Comparelib/matplotlib/figure.py Outdated
# explicitly set the bbox transform if the user hasn't. | ||
l = mlegend.Legend(self, handles, labels, *extra_args, | ||
bbox_transform=transform, **kwargs) | ||
l._outside = outside |
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'm not following the logic here.l._outside
seems to dynamically add an attribute to the Legend. It's better to to pass this via__init__
and document there what the type and expected values are.
I'm wondering whether the whole logic would be better moved in toLegend.__init__
. On the downside, not every legend can cope with "outside", OTOH, if you don't hack this in via a dynamic attribute, you'd have a formaloutside
parameter inLegend.__init__
and you'd have to cope with the outside case anyway. In that situation it seems simpler to makeLegend.__init__(... loc=...)
accept whatever is needed (i.e. also 'ouside upper left') and do all the logic in there.
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.
Except the loc keyword arg does error checking at the legend level, and legend doesn't know if it is being called as a figure legend or an axes legend. I think it's better to keep figure.legend only code in the figure.legend method
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.
But the legend carries the information where it wants to be rendered. That'sloc
and now ammended with_outside
.
By introducing "outside", positioning has become more complex. While we already have the case that the meaning ofloc
changes depending onbbox_to_anchor
, we still got away with a cheap validation because the supported values are the same.
That's now getting a bit more complicated. I still think it's valuable to have all the validation logic in one place. We may need to consider making legend aware on whether it's positioning in a figure or Axes.
Anyway, the outside state should flow through a public API into Legend, otherwise downstream users/libraries will not be able to create their own outsideLegend
s.
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.
Anyway, the outside state should flow through a public API into Legend, otherwise downstream users/libraries will not be able to create their own outside Legends.
Can you clarify what you have in mind here?
This is a pretty over specified problem, where at your and@efiring request we are offering a different idiom for axes than figures for theloc keyword argument. Now you are suggesting that somehow legend itself must parse that, and parse it differently whether it is a figure legend or an axes legend, information the legend does not currently have.
That all seems very confusing to me, and very hard to document. Either we have the grammar forloc proposed here and parse it in the figure method and leave the general object mostly alone. Or the original proposal was to have an optional kwarg "outside" that was not made available to the axes method. That seems cleaner to me, and less likely to lead to confusion, but...
This PR does the mechanical part of adjusting the constrained layout to accept the legend. This is an oft requested feature, but it's not one I particularly plan to use. So if there is going to be a bunch more API discussion I'd like to suggest that one of the two forms above be merged so the functionality is present and then a follow up PR can work out the API and how to document it. Conversely happy to just close the PR and someone else can use it to base their own PR on.
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 looked at the code and the legend indeed knows whether it's an Axes or a Figure legend.
matplotlib/lib/matplotlib/legend.py
Lines 455 to 465 in00ec4ec
ifisinstance(parent,Axes): | |
self.isaxes=True | |
self.axes=parent | |
self.set_figure(parent.figure) | |
elifisinstance(parent,FigureBase): | |
self.isaxes=False | |
self.set_figure(parent) | |
else: | |
raiseTypeError( | |
"Legend needs either Axes or FigureBase as parent" | |
) |
and theinaxes
parameter is already used for conditionalloc
validation:
matplotlib/lib/matplotlib/legend.py
Lines 475 to 478 in00ec4ec
ifnotself.isaxesandloc==0: | |
raiseValueError( | |
"Automatic legend placement (loc='best') not implemented for " | |
"figure legend") |
I propose it's better to passloc
toLegend
unmodified and have all the validation (and maybe transformation) logic inside.
I'm sorry I don't have the time to write a PR for that right now. Overall, your PR solves the problem and does not introduce any undesired public API or side-effects. So we may take it as is for now and cleanup the internal logic later. I believe this is needed for better long-term maintainability.
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.
OK, I moved it in the newest version.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1a80d9e
toc1fe98d
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2913d0b
toc5926ad
Compare@@ -0,0 +1,10 @@ | |||
:orphan: |
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.
:orphan: |
lib/matplotlib/legend.py Outdated
if self.isaxes and self._outside: | ||
# warn if user has done "outside upper right", but don't | ||
# error because at this point we just drop the "outside". | ||
raise UserWarning( |
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.
raiseUserWarning( | |
raiseValueError( |
65f061c
to3d369d4
CompareBTW, this is not particularly hard to re-milestone if it's slowing down the release. OTOH, I think it's substantively done, and oft-requested. |
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.
The nameself._outside
threw me for a bit (I thought it was a bool and I was deeply confused how this worked). Given that this is a private attribute not a huge deal (and likely a me-problem).
Easy enough to change to |
3d369d4
toe722171
CompareChanged |
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.
Almost good to go: Please remove the obsolete comment.
Anybody can merge after that.
lib/matplotlib/legend.py Outdated
if self.isaxes and self._outside_loc: | ||
# warn if user has done "outside upper right", but don't | ||
# error because at this point we just drop the "outside". |
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.
The comment is not valid/needed anymore.
e722171
to54f7236
CompareThanks for the reviews! |
Just dropping in to share some ❤️ for this feature, just used it for the first time and it's so helpful! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Much simpler redo of#13072.Closes#13023.
old
doesn't place the legend outside the axes...
new
Added an
outside
kwarg to fig.legend:If we want it in the upper right, but on top, instead of beside, then
Caveats
There is no magic here - multiple legends will over-run each other. The legends will also overlap with a suptitle/supxlabel/supylabel if they are present. They all get put in the same "margin", but basic functionality is there.
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).