Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ksunden merged 1 commit intomatplotlib:mainfromjklymak:enh-layout-fig-legend
Jan 6, 2023

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedMar 19, 2021
edited
Loading

PR Summary

Much simpler redo of#13072.Closes#13023.

old

importnumpyasnpimportmatplotlib.pyplotaspltfig,axs=plt.subplots(1,3,constrained_layout=True)fori,axinenumerate(axs):ax.plot(range(10),label=f'Boo{i}')ifi==0:ax.set_ylabel('Booo')lg=fig.legend(loc='upper right')fig.suptitle('Top')plt.show()

doesn't place the legend outside the axes...

Old

new

Added anoutside kwarg to fig.legend:

...fig.legend(loc='outside right upper') ...

New

If we want it in the upper right, but on top, instead of beside, then

...fig.legend(loc='outside upper right') ...

New

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

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

story645, IlyaOrson, and jhns-de reacted with thumbs up emojidcherian and tgbrooks reacted with hooray emoji
@jklymakjklymak added topic: geometry managerLayoutEngine, Constrained layout, Tight layout topic: legend labelsMar 19, 2021
@jklymakjklymak added this to thev3.5.0 milestoneMar 19, 2021
@jklymak
Copy link
MemberAuthor

@timhoffm and@anntzer before I go nuts with the docs for this, early feedback on the API would be welcome...

@jklymakjklymak changed the titleENH: allow fig.legend in layoutENH: allow fig.legend outside axes...Mar 19, 2021
@anntzer
Copy link
Contributor

We should perhaps do a call to hash out the API decisions both for this and for#12679 once and for all...

@jklymak
Copy link
MemberAuthor

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.

@jklymakjklymakforce-pushed theenh-layout-fig-legend branch 2 times, most recently fromc6a9869 to9c577aeCompareMarch 21, 2021 17:18
@jklymakjklymak marked this pull request as ready for reviewMarch 21, 2021 17:44
@timhoffm
Copy link
Member

timhoffm commentedMar 24, 2021
edited
Loading

I did not have the time to read through this completely and make my own thoughts.

Some quick observations though:

  • fig.legend(loc='upper right', outside='upper') does not speak to me. This seems like a too clever way to put in additional degrees of freedom.
  • Legend positioning is already a bit of a mess. I always struggle with thebbox_to_anchor option modifying the meaning ofloc. I feel it's getting worse with an addtional parameter.
  • Which positions do we want to address. Are these essentially the A-L ones from the cheatsheet?
    grafik
    Or even more, including the outer edges? The image makes clear that there are more outer positions than inner positions so that a simple additionaloutside bool cannot be enough. MATLAB folds this into the string. e.g. 'westoutside'. I'm wondering if that's a better choice.
  • Semi-OT: Could Axes legend also be made to work "outside" with constrained_layout?
story645 reacted with thumbs up emoji

@jklymak
Copy link
MemberAuthor

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 sayingloc="upper right", bbox_to_anchor=(-.1, .9) is not an acceptable API. Of course this won't break that, but I don't think we should be emulating it.

Here the proposal is:

A: loc="left", outside='upper'
B: loc="left", outside=True
C: loc="lower left", outside=True
D: loc="lower left", outside='lower'
E: loc="lower center", outside=True
F: loc="lower right", outside='lower'
G: loc="lower right", outside=True
H: loc="right", outside=True
I: loc="upper right", outside=True
J: loc="upper right", outside='upper'
K: loc="upper center", outside=True
L: loc="upper left", outside='upper'

Note that if the user specifiesbbox_to_anchor then theoutside kwarg is ignored.

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.

@jklymakjklymakforce-pushed theenh-layout-fig-legend branch from9c577ae toffb8578CompareMay 11, 2021 16:43
@jklymak
Copy link
MemberAuthor

... rebased

@jklymak
Copy link
MemberAuthor

I'll advocate for this one as often-desired, and I'll also advocate for "lets keep it simple"

@Tobychev
Copy link

Tobychev commentedJun 13, 2022
edited
Loading

@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:
A: outside='upper left'
B: outside="left"
C: outside="lower left"
D: outside="bottom left"
E: outside="bottom"
F: outside="bottom right"
G: outside="lower right"
H: outside="right"
I: outside="upper right"
J: outside="top right"
K: outside="top"
L: outside="top left"

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
Copy link
Member

efiring commentedDec 1, 2022
edited
Loading

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:
A: outside='left top'
B: outside="left middle"
C: outside="left bottom"
D: outside="bottom left"
E: outside="bottom middle"
F: outside="bottom right"
G: outside="right bottom"
H: outside="right middle"
I: outside="right top"
J: outside="top right"
K: outside="top middle"
L: outside="top left"

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.
On second thought, I guess those diagonal locations are poor for legends, so the modified compass forms would be "NE top", "NE right", etc.

@jklymak
Copy link
MemberAuthor

# 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
Copy link
Member

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.

Copy link
MemberAuthor

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

Copy link
Member

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 outsideLegends.

Copy link
MemberAuthor

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.

Copy link
Member

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.

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:

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.

Copy link
MemberAuthor

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.

timhoffm reacted with thumbs up emoji
@jklymakjklymak marked this pull request as ready for reviewDecember 13, 2022 21:54
@jklymakjklymak removed the status: needs comment/discussionneeds consensus on next step labelDec 13, 2022
@rcomerrcomer mentioned this pull requestDec 22, 2022
4 tasks
@tacaswelltacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelDec 22, 2022
@@ -0,0 +1,10 @@
:orphan:
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
:orphan:

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(
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
raiseUserWarning(
raiseValueError(

@jklymakjklymakforce-pushed theenh-layout-fig-legend branch 2 times, most recently from65f061c to3d369d4CompareJanuary 4, 2023 23:40
@jklymak
Copy link
MemberAuthor

BTW, 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.

Copy link
Member

@tacaswelltacaswell left a 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).

@jklymak
Copy link
MemberAuthor

Easy enough to change to_outside_loc or something like that. For the main code itis basically a boolean. It only gets used as a string inside_constrained_layout.py.

@jklymak
Copy link
MemberAuthor

Changedself._outside toself._outside_loc

tacaswell reacted with thumbs up emoji

Copy link
Member

@timhoffmtimhoffm left a 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.

Tobychev reacted with hooray emoji

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".
Copy link
Member

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.

jklymak reacted with thumbs up emoji
@ksundenksunden merged commite4c1d70 intomatplotlib:mainJan 6, 2023
@jklymakjklymak deleted the enh-layout-fig-legend branchJanuary 6, 2023 23:50
@jklymak
Copy link
MemberAuthor

Thanks for the reviews!

@dstansby
Copy link
Member

Just dropping in to share some ❤️ for this feature, just used it for the first time and it's so helpful!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
New featureRelease criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: geometry managerLayoutEngine, Constrained layout, Tight layouttopic: legend
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

constrained_layout support for figure.legend
9 participants
@jklymak@anntzer@timhoffm@Tobychev@efiring@dstansby@tacaswell@QuLogic@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp