-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Introduce compass-notation for legend #12679
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
base: main
Are you sure you want to change the base?
ENH: Introduce compass-notation for legend #12679
Conversation
eb23290
to
826b7b1
Compare
I'm not really happy with the two-letter compass notation. It's not readable. For me, the only reason to introduce this is that we already have that in other parts of the library. For information, MATLAB uses written-out compass notation: |
The main advantage is that it is completely unambiguous and hence straight forward to remember. It seems there are a lot of people who need to google for the correct names/codes each time they do a plot, so this is guaranteed to be helpful for those in need of it (others don't have to care). Concerning readability, note that I am explicitely not proposing to change the examples to use this compass notation. |
826b7b1
to
2e0c8c5
Compare
I'm fine with this proposed notation, particularly as we use it elsewhere. "left centre" is really bad. I'd be fine with "west" as well as "W". |
I think I have a mild preference for the MATLAB-style(!!) spelled-out strings "northeast" etc (as long as we use it everywhere). Let's say +0.5 for "northeast" and +0 for "NE" (but only one of them, and has to be consistent with eg. inset locator). |
I'm afraid we cannot easily get around the short "NE". It's already been used e.g. for anchors https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.set_anchor.html. Even if we decide to use "northeast" everywhere, we'd have to keep "NE" around for the anchor (at least for a deprecation period, but I tend to not deprecating that). |
Should I allow for "northeast" in addition to "NE" then? (Note to self: This still needs a what's new entry.) |
I'm +/-0 on this. It's a more readable convention. OTOH, it's not good to have too many ways to specify things. |
lib/matplotlib/legend.py
Outdated
'upper left' 'NW' 2 | ||
'lower left' 'SW' 3 | ||
'lower right' 'SE' 4 | ||
'right' 5 |
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 is right
vs center right
, and why is there no matching left
?
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.
right
is outside of the Axes. This is what MATLAB calls "outsideeast". Unfortunately, our terminology is not good here, and we simply did not implement any other outside positions.
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.
"right"
and "center right"
are identical. They both result in the legend to be placed at the "east" position.
matplotlib/lib/matplotlib/legend.py
Lines 1086 to 1088 in 9b01a03
R: "E", | |
CL: "W", | |
CR: "E", |
I suppose one of them only exists for backwards compatibility reasons.
If you want to place the legend outside the axes you need to supply a tuple of xy coordinates (loc=(1.05, 0.7)
) or directly use the bbox_to_anchor
argument.
I don't think it makes sense to add more than one way to do it for legends. Pick an "ideal" name to use for legends, and then update anchors to support that new ideal name, optionally deprecating the old one. |
That's the conflict here. IMO "southeast" is to be preferred to "SE". But "SE" is already used for anchors (https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.set_anchor.html). Different terminologies for legends and anchors make no sense. That would mean "SE" would need to be depreacated for anchors. But OTOH is it worth it? |
I wouldn't overly worry about set_anchor, which is likely much more rarely used than legend() (and already not consistent). If we switch to spelled-out compass directions, I'd just do the deprecation dance on set_anchor. |
👍 in general, I am -0 on supporting both the abbreviations and the full names, and +1 on no spaces if we do full names. I am not sure which way to go on full names vs abbreviations though. |
xref #13072 |
I am still 👍 , but on the phone call @efiring convinced me that we should do both and with spaces is going to be easier to read. |
Strong preference for spaceless longforms. |
Following up, on the call again,
|
dbe28aa
to
75670d0
Compare
75670d0
to
9940eca
Compare
@@ -584,10 +552,6 @@ def __init__(self, parent, handles, labels, | |||
else: | |||
self.get_frame().set_alpha(framealpha) | |||
|
|||
tmp = self._loc_used_default | |||
self._set_loc(loc) | |||
self._loc_used_default = tmp # ignore changes done by _set_loc |
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.
Those lines somehow got lost during rebase. I have no idea what they are used for. Anyone else maybe?
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.
ping @timhoffm about this code...
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 must stay in (and maybe get a more descriptive comment).
It's a hack to remember if we're using loc='best'
as default or if it was explicitly set by the user. It's used to determine whether to issue a warning with loc='best'
and many plot elements. The warning should only be issued with for the default, not when the user explicitly sets 'best'.
See #12455 (review)
and the link therein: timhoffm@917b520
9940eca
to
2b8181a
Compare
Updated. I introduced getters and setters for |
2b8181a
to
db8c7d9
Compare
flake8:
|
Labelled "work in progress" due to the unclear state of backward incompatibility. |
The problem with backwards incompatibility is the following: This PR attempts to simplify the positionning code, such that internally, the short compass notation is used throughout. Previously we would convert from strings to numbers ("lower left" --> 3), then from numbers to compass (3 --> "SW"). With this PR the intermediate step is removed. This works nicely for The cleanest solution to this would be to
Alternatives are of course:
|
One solution for strict backcompat (frankly, I think the change would affect vanishingly few people) could be to make the compass locations IntEnums (instead of strs), which subclass int so they behave like ints for all purposes (see the similar thing I did for MouseButton). Alternatively, control whether AnchoredOffsetbox.loc is a number or a string using a global rcparam (ugh), emitting a deprecation warning when loc is accessed if the rcparam value is "legacy" (or however we want to call it). |
I don't think I understand the IntEnum proposal. Would that mean to have some (a) result in the same problem of making a decision on the "standard" kind of location, i.e. between |
Users would remain able to pass in |
Pushing to 3.3, but have not read through the full discussion. If people think this has converged and can be merged in the next ~3 days please do so for 3.2. |
This is correctly labelled "Work-in-progress" and cannot be merged as is. It will need new input from my side followed by a new round of dicussion. |
Here's a suggestion including outside axes with compass notation, essentially treating each cell as a coordinate point from the center. Two steps in any direction is outside the axes. (Admittedly,
|
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
PR Summary
Legend locations (
loc
) can currently be given as strings (like "upper left") or numbers (like2
). Both have the drawback that they are hard to remember. Some arguments are exchanged about this in #11174Also, there is a recent stackoverflow question "Legend location, similar to telephone", asking why not order the numbers like on a phone.
A possible solution was proposed by @efiring to use a compass notation. Also, the new inset locator uses compass notation.
This PR introduces this compass notation, (e.g.
loc="NW"
)in addition to the existing location strings and codes for
legend
.PR Checklist