Skip to content

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ImportanceOfBeingErnest
Copy link
Member

PR Summary

Legend locations (loc) can currently be given as strings (like "upper left") or numbers (like 2). Both have the drawback that they are hard to remember. Some arguments are exchanged about this in #11174

Is it “top” or “upper”, “bottom” or “lower”, “Center”, “centre”, or “middle”? I’d have to look it up.

Additionally, the numbers don't even have a reasonable order:

    2  9 1
    6 10 7
    3  8 4

Also, 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")

NW   N   NE
W    C    E
SW   S   SE

in addition to the existing location strings and codes for legend.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.1 milestone Oct 30, 2018
@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the legend-loc-compass-notation branch 2 times, most recently from eb23290 to 826b7b1 Compare October 30, 2018 21:19
@timhoffm
Copy link
Member

I'm not really happy with the two-letter compass notation. It's not readable. loc='SE' is obscure while loc='lower right' is self-explanatory.

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:
https://de.mathworks.com/help/matlab/ref/legend.html#bt6ef_q-1-lcn
In itself, that's not too bad. However, I doubt we should not introduce yet another format. Also MATLAB has "*outside" locations, which would be really nice, but that's another topic.

@ImportanceOfBeingErnest
Copy link
Member Author

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.

@jklymak
Copy link
Member

jklymak commented Oct 31, 2018

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

@anntzer
Copy link
Contributor

anntzer commented Oct 31, 2018

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

@timhoffm
Copy link
Member

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

@ImportanceOfBeingErnest
Copy link
Member Author

Should I allow for "northeast" in addition to "NE" then?

(Note to self: This still needs a what's new entry.)

@timhoffm
Copy link
Member

timhoffm commented Nov 1, 2018

Should I allow for "northeast" in addition to "NE" then?

I'm +/-0 on this. It's a more readable convention. OTOH, it's not good to have too many ways to specify things.

'upper left' 'NW' 2
'lower left' 'SW' 3
'lower right' 'SE' 4
'right' 5
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Member Author

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.

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.

@eric-wieser
Copy link
Contributor

Should I allow for "northeast" in addition to "NE" then?

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.

@timhoffm
Copy link
Member

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?

@anntzer
Copy link
Contributor

anntzer commented Jan 2, 2019

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.

@tacaswell
Copy link
Member

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

@tacaswell
Copy link
Member

xref #13072

@tacaswell
Copy link
Member

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.

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.

@anntzer
Copy link
Contributor

anntzer commented Feb 18, 2019

Strong preference for spaceless longforms.

@tacaswell
Copy link
Member

Following up, on the call again,

  • allow short notation (no spaces) all uppercase
  • allow long notation (no spaces) all lowercase

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

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?

Copy link
Member

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

Copy link
Member

@timhoffm timhoffm Mar 2, 2019

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

@ImportanceOfBeingErnest
Copy link
Member Author

ImportanceOfBeingErnest commented Feb 25, 2019

Updated.
This now introduces "NW" as well as "northwest" as locations for the legend, as well as for other anchored artists and inset_axes.
For this I externalized the mapping from the possible input values into cbook, such that it can be reused everywhere. Internally, the one- or two character strings are now used to specify the location. (Previously strings got mapped to numbers, than back to those compass location codes.)
This should preserve all deprecations, although there is an attribute self._loc_used_default which got lost when rebasing - and I have absolutely no idea about its purpose and I still need get this pass the tests.

I introduced getters and setters for loc as well. This is a new feature, which allows to set the location a posteriori, and it also ensures that if people used the private attribute self._loc in their code to achieve the same, their code would not fail through this change.

@timhoffm
Copy link
Member

timhoffm commented Mar 7, 2019

flake8:

./lib/matplotlib/cbook/__init__.py:2170:1: W293 blank line contains whitespace
./lib/matplotlib/cbook/__init__.py:2192:1: W293 blank line contains whitespace
./lib/matplotlib/cbook/__init__.py:2198:1: W293 blank line contains whitespace

@timhoffm
Copy link
Member

timhoffm commented Mar 7, 2019

Labelled "work in progress" due to the unclear state of backward incompatibility.

#12679 (comment)

@ImportanceOfBeingErnest
Copy link
Member Author

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 legend, because there is no public method for getting or setting the location.
Now to the problem: AnchoredOffsetbox does have a public loc attribute. It's easy to route any assignment to this though the new converter. However, reading out this attribute would yield a different value, i.e. "SW" instead of 3. This is an API change.

The cleanest solution to this would be to

  • start a deprecation, like "Warning, this attribute will return the location in compass notation in the future"; but then the question is how to allow people to adapt their code already now? Usually I would go on with "To prevent this warning and use the compass notation already now, use the get_loc method instead." However, some concerns were raised about adding such get_loc method. But without such new method, no useful deprecation can be performed.

Alternatives are of course:

  • Ignore the fact that this is an API change; presumably the number of cases where someone want to read out AnchoredOffsetbox.loc is quite small.
  • Reintroduce the conversion from string to number, i.e. let loc work with the numbers, only convert to short compass notation at the very end. This would render some of the attempted simplifications of this PR useless.

@anntzer
Copy link
Contributor

anntzer commented Mar 11, 2019

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

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Mar 18, 2019
@ImportanceOfBeingErnest
Copy link
Member Author

I don't think I understand the IntEnum proposal. Would that mean to have some LocClass.NW object people need to use instead of "NW" or "northwest". Wouldn't that

(a) result in the same problem of making a decision on the "standard" kind of location, i.e. between LocClass.NW and LocClass.northwest?
(b) Introduce a hysteresis in arguments you pass (i.e. a string "northwest") versus arguments you get out (i.e. a number/enum LocClass.NW)? Or should users then pass LocClass.NW themselves?

@anntzer
Copy link
Contributor

anntzer commented Apr 1, 2019

Users would remain able to pass in "NW" or "northwest" (or Location.NW, or 2), but when reading out the loc attribute they would get Location.NW. Because Location.NW would be an IntEnum, it would compare equal (for == or as a dict key or whatnot) to its current int value (2).
Essentially, Location.NW is the number 2, just with a nicer repr.

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 25, 2019
@tacaswell
Copy link
Member

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.

@ImportanceOfBeingErnest
Copy link
Member Author

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.

@QuLogic QuLogic marked this pull request as draft April 15, 2020 02:46
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 Apr 30, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@fourpoints
Copy link
Contributor

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, northnorth might sound awkward.)

   │NNW│ NN│NNE│
───╆━━━┿━━━┿━━━╅───
WNW┃NW │ N │ NE┃ENE
───╂───┼───┼───╂───
 WW┃W  │ C │  E┃EE
───╂───┼───┼───╂───
WSW┃SW │ S │ SE┃ESE
───╄━━━┿━━━┿━━━╃───
   │SSW│ SS│SSE│

@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 21, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants