Skip to content

Support placing legend symbols next to axis labels #16005

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 9 commits into
base: main
Choose a base branch
from

Conversation

alexhartl
Copy link

PR Summary

For twinx() plots it is more space-efficient and intuitive to have legend symbols next to the axis labels instead of in a separate box. This PR adds this functionality, providing plt.xlabel_legend() and plt.ylabel_legend() functions.

Example from docstring:
example

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

@alexhartl alexhartl changed the title Label legends Support placing legend symbols next to axis labels Dec 22, 2019
@jklymak
Copy link
Member

jklymak commented Dec 22, 2019

This seems like an interesting addition, but I'm pretty concerned with bringing legend logic into text.py. I've not followed all the logic, but that seems like a red flag that there are architecture problems with your solution.

It would be good to know how this interacts with tight_layout and constrained_layout.

We already have the idea of legends being outside the axes bounds, so it seems this could just be a modification of those legends. Can you just rotate normal legends 90 degrees? Thats the missing piece here.

@alexhartl
Copy link
Author

It seems like a lot of work to adapt an ordinary legend to replace an axis label. Also, no, at the moment there are problems with rotating those. Actually, on SO the preferred method to achieve this seems to be to emulate the legends symbols with LaTex.

Could you elaborate further which problems you see? The logic could alternatively be placed in axis.py, but considering the layout of individual lines, results look better when adapting _get_layout in text.py.

I tried with tight_layout and constrained_layout and didn't experience any problems.

@jklymak
Copy link
Member

jklymak commented Dec 22, 2019

This PR basically says that every Text artist, which is a pretty low level artist, now has the ability to have a legend symbol attached to it, and gets a different layout when that happens. That seems to be a pretty big architectural change to me. Maybe its a justifiable one, but its a general change to all text objects, and probably deserves its own PR with the API and ramifications fully laid out.

It seems the less intrusive change is to change the axis.label object to have a Text and a handle and not touch Text. That’s still a big change, so perhaps there is an even less intrusive way to go.

@timhoffm
Copy link
Member

Did not look into the details how Legend organizes these things, but from a quick design thought, it may be reasonable to have LegendEntry objects that manage text plus line/marker. Then, reuse one of these to make a custom axis label. LegendEntry might derive from Text (similar how Annotation does).

Anyway, this is a significant API change and needs careful design.

@alexhartl
Copy link
Author

alexhartl commented Dec 22, 2019

It would be possible to implement it in axis.py. The problem here is mainly that if the label stretches over multiple lines, the second line doesn't look properly aligned.

What could be changed easily is not attaching a legend handle to a Text object, but just a DrawingArea, and initialize the DrawingArea somewhere else. This would keep legend out of text.py. Would that alleviate your concerns?

@alexhartl
Copy link
Author

I think it is a good idea to move the functionality to a subclass of Text. However, I think similarities between regular legend entries and axis legends are rather superficial. For regular legends

  • several entries should be properly aligned to each other
  • alignment is always left
  • there is no need for rotation
  • positioning is wrt to the enclosing box and not wrt to the text baseline

So to me forcing legend entries and axis legends into the same object seems to be a lot of additional effort and might even introduce backward compatibility issues, while the joint functionality is quite limited.

@alexhartl
Copy link
Author

I've moved the logic to a subclass of Text (as far as possible without affecting functionality). Let me know what you think. I'll fix conflicts when I get your ok.

@anntzer
Copy link
Contributor

anntzer commented Jan 24, 2020

Rather than moving the functionality to a subclass of Text (given that we just deprecated and removed the somewhat similar TextWithDash #13548 #15950), I would hope that it would be possible to adjust the xylabel machinery to use any (most) Artist instead of just a Text object -- specifically, one could then pass an HPacker (similarly to how legends are built). IOW, use composition (an HPacker which holds the subartists we want, and should be quite more adaptable) rather than inheritance.
I guess this is kind of similar to the DrawingArea proposal just above.

@alexhartl
Copy link
Author

The problem with this approach occurs when allowing multiple lines. Let's consider an example:

ex

a) is my current implementation, b) is what you might get when using a VPacker.

Which option do you prefer? To me, b) looks misaligned. You cannot achieve a) with a VPacker, because you have to position the symbol only in the first line while leaving the other lines unaffected.

I'm not opposed to my DrawingArea proposal. I doubt that the interface would be useful for a lot of other tasks, though.

@anntzer
Copy link
Contributor

anntzer commented Jan 25, 2020

You raise a good point which I didn't think about, but actually now that you show both of them, (b) actually looks correct to me whereas (a) looks misaligned :) Dunno what others think...

@tacaswell
Copy link
Member

This came up on the dev call this week: https://hackmd.io/4zWKhuLXQ16Y_oUfi9hnKA#16005-legend-on-xlabelylabel . If that time is plausible for you @alexharti it may be productive for you to join
the in the near future.


I have not had time to carefully review the code so I am responding to the discussion here and a skimming of the code.


I think that this is an interesting and useful feature, however I am not convinced that it is something we want to put into the Axes and pyplot namespace. One of the biggest issues people have getting started is that Matplotlib's API is too wide at the top level which hurts discoverability and it is my sense that this is a relatively niche use-case.

I think the best path forward here is:

  • extend the API for axis.set_label (and implicitly the Axes.set_{x/y}label methods) to accept any artist (or at least artists that duck-type as Text instances, Axis.label1 is a public attribute whos type we should preserve as much as possible)
  • put the logic for generating the handle + text object in a function in legend.py

The end result would be usage that looks something like (this is a re-phrasing / combining of ideas that have already come up in this thread):

import matplotlib.pyplot as plt
from matplotlib.legend import legend_label  # TODO better name?
fig, ax = plt.subplots()
ln, = ax.plot(...)
ax.set_ylabel(legend_label(ln, 'my text'))

which is only slightly more verbose than current proposal from the end-user point of view.

This would also allow there to be out-of-core exploration of label_legend alternatives (for example, a label that has a sparkline in it which could be useful for exploring very high-dimensional data or variations on alignment / layout / ...) and allow down-stream packages to have versions / wrappers of this that make the right assumptions for their domain.

While I understand our conservativeness may be frustrating from a contributor stand point, hopefully as a user you have some sympathy for it. Once we put something out, it is very hard to take back or change.

@alexhartl
Copy link
Author

This certainly is an interesting interface. However, I think there are issues:

  • In Axis a few parameters of Text are set to defaults like e.g. axes.labelsize. So if legend_label uses Text defaults, appearance would be inconsistent and/or would have to be set manually by the user. But if legend_label uses label defaults, behaviour would be a bit inconsistent given that it should be just a Text with a legend attached, and it wouldn't be possible to use the same object for set_title, as proposed.
  • The legend_label constructor has to accept both legend kwargs and Text kwargs. So one of them can't be passed by **kwargs but by a separate dict. Don't know if that's a pattern that's currently being used in matplotlib.

As counter proposal let me pick up the DrawingArea proposal again. Text would get a function set_symbol() which accepts a DrawingArea. set_symbol() might then be called by Axes.set_ylabel() or Axes.set_title(), resulting in an interface that might look like

import matplotlib.pyplot as plt
from matplotlib.legend import label_legend
fig, ax = plt.subplots()
ln, = ax.plot(...)
ax.set_ylabel('my text', symbol=label_legend(ln))

You could manually create a DrawingArea and put arbitrary artists in it. If you want to invent a label with a sparkline, you would do ax.set_ylabel('my text', symbol=sparkline(...)), where sparkline() subclasses or returns a DrawingArea.

Independent of the interface I would be interested in a few opinions if in the above comparison #16005 (comment) option a) or b) is preferred (or c) make it configurable).

@tacaswell
Copy link
Member

For the first point, the set_foo methods could try to impose their configuration on the object passed in with a flag to suppress that (like we do when creating an object collection from a list of artists).

For the second point, that is more of an oversight in the API that you can not control the text properties other than grabbing the label object and changing it. Having neither set be kwargs, but have a legend_kwargs and a text_kwargs would probably be the right path. However, one of the great things about this proposal is that there can be more than one version so we can have multiple APIs co-exist etc. The other solution to the first point is to make in non-optional that the text style will be set when the Artist is added to the axes and if the user wants to change it they can grab it later and set things (which is what it looks like you have to do now).

If you make the symbol "special" now you have to make a whole bunch of extra decisions, in the core API, that it is not clear about the layout and how to control it (what is the aspect ratio of the symbol? what is the padding between the text and the symbol? do you rotate opposite directions on the left and right spines? in the label on to the left, the right, above, or below the text?)

What does DrawingArea provide that we do not already get from Artist?

If we do go with the idea of extending Text to add a symbol to it, it should be a subclass that is only used when needed. We don't need every piece of text on the figure to be able to grow a symbol (and carry all of the required layout logic etc. It may even be that such a Text sub-class is very helpful in implementing the above helpers

def legend_label(art, label, txt_kwargs, legend_kwargs, layout_kwargs):
    handle = extract_legend(art, **legend_kwargs)
    return SymbolAwareText(label, handle, **txt_kwargs, **layout_kwargs)

If there is no collisions between them, you could also imaging having some sorting logic inside so that the outer API stays flat.

@jklymak jklymak marked this pull request as draft September 24, 2020 15:24
@jklymak jklymak added the stale label Sep 24, 2020
@jklymak
Copy link
Member

jklymak commented Sep 24, 2020

This seems stalled out. @alexhartl we very much appreciate the contribution, and there seemed substantial support for this, but there would need to be a fair bit of discussion about architecting. You can probably do that here, but I've moved this to "Draft" and marked stale. If there is no further action in a month or so, we may just close this for now. Don't be shy about pinging us. Old PRs can get buried.

@alexhartl
Copy link
Author

Sorry, at some point I got too busy and lost sight of this.

I would very much like to see this functionality in matplotlib and I can do the coding if you want. But I think it's better if you guys first set out a plan of how we proceed here. Besides the interface (which after all isn't too important to me), I see at least the following points:

  • I moved some stuff from Legend to a superclass called BasicLegend (I'm open for suggestions for better names), which is an architectural change which also needs agreement.
  • The question I've raised in Support placing legend symbols next to axis labels #16005 (comment). @anntzer already stated his opinion, I'd prefer variant a) but maybe it's just me who has a crazy eye.
  • Whether this should be split to multiple PRs, as someone suggested

@alexhartl
Copy link
Author

Hi again, I'm currently working on a paper where I would again need this functionality. As I said, I don't care too much about the architectural details, so if we could agree on some interface, so that I can develop a PR that would find its way into matplotlib, that would be great.

@jklymak
Copy link
Member

jklymak commented Mar 25, 2022

@alexhartl Thanks for your patience with this. We discussed extensively on the weekly developer call. Without getting into exactly what you have already done, we think the architecture should be something like:

  1. expose a helper to make an hpacker that has the text and symbol just like legend does and returns a LegendText subclass of Text (or whatever name). Ideally legend would also get a mild refactor to use this object.
  2. allow xlabel/ylabel/title etc accept a Text object as well as a string for directly putting in the label slot.

I think this is relatively close to your current design, but doesn't make all labels have the extra marker by default, and allows future flexibility for other Text subclasses.

@alexhartl
Copy link
Author

alexhartl commented Mar 29, 2022

Sounds good. In addition to the actual interface, I introduced a superclass of Legend called BasicLegend (alternatively LegendBase, LegendConfig, etc.) containing functionality for holding parameters/config (handlelength, labelspacing, etc.) that is shared between Legend and LegendText. Opinions on this?

@jklymak
Copy link
Member

jklymak commented Mar 29, 2022

can you explain what that superclass does? Does your new LegendText need to inherit from it? Or can those parameter just be sent directly to LegendText, and then Legend just use LegendText instances?

@alexhartl
Copy link
Author

It holds several parameters (numpoints, markerscale, scatterpoints, scatteryoffsets, borderpad, handlelength, handleheight, handletextpad) and the logic for handler maps. Yes, the parameters could simply be passed on to the LegendTexts, but for the handler maps this becomes tricky to not break backward compatibility.

Additionally, when placing multiple LegendTexts you probably usually want them to have a similar appearance, so it might make sense to configure them in a common object (which ideally can be based on a real boxed legend).

As alternative to a superclass, how about allowing/requiring to instantiate a dummy Legend in such cases, i.e. something like

line = plt.plot([1,2,3])
legend = Legend(None,[],[], handlelength=0.7)
plt.xlabel(create_legendtext('My xlabel', line, legend=legend))
plt.ylabel(create_legendtext('My ylabel', line, legend=legend))

This would be less intrusive and constitute a comparable pattern to how you have to instantiate a dummy Line2D if you want a custom legend entry.

@jklymak
Copy link
Member

jklymak commented Apr 8, 2022

Without checking the details, I thought it would make more sense the other way: Legend.init takes those parameters, and passes them to LegendText.__init__ for each text/label pair, and then the LegendText objects are assembled in the Legend in the (nested) vpackers. Then when you also allow users to make a LegendText object and pass as text to xlabel etc...

@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 Jul 31, 2023
@melissawm
Copy link
Member

Hi @alexhartl - would you like help to pick this back up? Cheers!

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Aug 9, 2023
@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 Oct 11, 2023
@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

7 participants