matplotlib icon indicating copy to clipboard operation
matplotlib copied to clipboard

POC: create `InsetIndicator` artist

Open rcomer opened this issue 1 year ago • 23 comments

PR summary

https://github.com/matplotlib/matplotlib/issues/19768#issuecomment-1854735866 suggested that we should have some kind of container for the output of indicate_inset[_zoom]. https://github.com/matplotlib/matplotlib/issues/23424#issuecomment-1185544814 suggested that it should be drawn as a single path to avoid double-alpha where the box and connectors overlap. This PR is my attempt to implement that.

The new artist has convenience methods to update colour/linestyle/etc for the whole thing, but the user can also get hold of individual patches and update styles on those. The current doctrings of indicate_inset[_zoom] specifically mention updating the visibility of the connectors, but a user might also reasonably want a different colour or linestyle for the connectors than for the box.

The logic from indicate_inset_zoom that decided where the box should be is now method InsetIndicator._bounds_from_inset_ax so we can reuse it. Whenever the connectors attribute is accessed (including at draw) the box bounds and position of connectors (if any) get updated using the current inset axes x- and y-limits. This therefore closes #19768 and we can do things like

import matplotlib.pyplot as plt
import matplotlib.animation as animation
import numpy as np

x = np.arange(100)
y = x + np.sin(x) * 5

fig, ax = plt.subplots()
ax.plot(x, y)

ax_ins = ax.inset_axes([0.6, 0.1, 0.3, 0.3])
ax_ins.plot(x, y)
ax_ins.set_xlim([0, 10])
ax_ins.set_ylim([0, 10])
ind = ax.indicate_inset_zoom(ax_ins)
for conn in ind.connectors:
    # In this example all the connectors are invisible by default - I'm unclear why that is.
    conn.set_visible(True)

def animate(i):
    ax_ins.set_xlim([i, i+10])
    ax_ins.set_ylim([i-5, i+15])
    return ax_ins,

ani = animation.FuncAnimation(fig, animate, interval=100, frames=90)

plt.show()

inset

Styles for the connectors are now inherited from the parent artist when the connectors are created. This therefore closes #23424. Within the draw method, we check which connectors have the same style properties as the rectangle (currently four specific properties - maybe there is a better way). Those that do are drawn with the rectangle as a compound path.

This also inadvertently fixes #23425 since I haven't implemented get_window_extent or get_tight_bbox, so constrained layout will get a null tight bbox. I would like advice on whether I should implement one or both of these and what it should include. For the purposes of layout engines is does not seem useful to include the connectors, but maybe there are other uses for these methods?

PR checklist

rcomer avatar Mar 30 '24 17:03 rcomer

Added a __getitem__ method in an effort to provide a deprecation pathway. Although indicate_inset[_zoom] are still marked as experimental, I suspect the pattern box, connectors = ax.indicate_inset... is pretty common.

rcomer avatar Mar 30 '24 19:03 rcomer

Perhaps instead of inheriting from Rectangle, this should be a container artist with both the rectangle and the connectors as children. Then users who want more control could get hold of the child artists and update colours, etc. on them individually 🤔

rcomer avatar Jul 27 '24 07:07 rcomer

Then users who want more control could get hold of the child artists and update colours, etc. on them individually

That sounds like a fairly common use case to me so I think that'd be awesome. Is there an advantage to the rectangle approach/downside to making it a container?

story645 avatar Jul 28 '24 08:07 story645

Is there an advantage to the rectangle approach/downside to making it a container?

Not that I can think of. I think I just had in mind from https://github.com/matplotlib/matplotlib/issues/23424#issuecomment-1185544814 that the new object should be a Patch and inheriting from Rectangle seemed like the obvious way to do that. Having said that, I'm not sure what defines a Patch and maybe that fact that the connectors are already children that you can configure separately from the whole thing means it doesn't really fit the definition.

rcomer avatar Jul 28 '24 12:07 rcomer

I'm not sure what defines a Patch 

Technically artist w/ face color + edgecolor & for the most part I've always thought of it as roughly the MPL abstraction of an area mark b/c that's basically the space of visual encodings we allow/can apply to most of our patch artists: Six-visual-variables-proposed-by-Bertin-and-their-capacity-in-representing-graphical.png

Where size and shape are controlled by the drawing of the shape, while for example the choice of marker determines shape and marker size controls the size.

So for what it's worth, I agree w/ your intuition that a container Artist is probably a better abstraction for what's actually going on here.

story645 avatar Jul 28 '24 22:07 story645

OK, the InsetIndicator is now a container artist instead of a patch. I couldn't see which module it would obviously fit in, so made a new one.

One side-effect of this is that we no longer get the axes limits updated around the artist, which is done in add_patch here: https://github.com/matplotlib/matplotlib/blob/1e983776e9210fbc7426f28ce6e7eeadea18f9e9/lib/matplotlib/axes/_base.py#L2414

So my test image changed from zoom_inset_connector_styles

to zoom_inset_connector_styles

I guess it would be pretty easy to re-instate that but I'm not sure if it matters. Most of the time there will be a bunch of other artists that this one is indicating.

Still to do:

  • ~Add changenotes~
  • Decide what, if anything, to do about get_window_extent/get_tightbbox

rcomer avatar Aug 11 '24 19:08 rcomer

Added changenotes and directives, though I'm not sure if the behaviour change documentation for indicate_inset[_zoom] is really needed since these methods were marked as experimental...?

rcomer avatar Aug 12 '24 10:08 rcomer

Hi, I've recently run into the following behaviour of indicate_inset_zoom and perhaps addressing it might be part of this PR. Otherwise I can raise it as a separate bug.

I specifically wanted to create the inset area and have it highlighted with a mildly transparent yellow colour. At the same time, I wanted the inset area border and the connectors to be solid black.

However I've discovered that setting colours for the lines and for the background inside the indicate_inset_zoom takes no effect.

The global alpha for indicate_inset_zoom is 0.5 and it seems like the only way to control the alpha of the border and the fill is through alpha parameter. And there's no way of setting various alpha levels for various elements.

Here's an example code and the output:

# IMPORTS
import numpy as np
import matplotlib.pyplot as plt


# DATA
n = 500  # number of samples
x = np.linspace(-1.25, 1.25, n) + np.random.normal(loc=0, scale=0.1, size=n)
y1 = x**3 + np.random.normal(loc=0, scale=0.025, size=n)
y2 = x**2 + np.random.normal(loc=0, scale=0.025, size=n)

# PLOT
fig, ax = plt.subplots(
    figsize=(12, 8),
    layout="constrained",
)

axins = ax.inset_axes(
    bounds=[0.65, 0.1, 0.3, 0.3],
    xlim=(-0.5, 0.5),
    ylim=(-0.25, 0.25),
)

for a in [ax, axins]:
    a.plot(x, y1, ".", color="blue")
    a.plot(x, y2, ".", color="red")

ax.indicate_inset_zoom(
    inset_ax=axins,
    linewidth=2,
    facecolor=("yellow", 0.2),  # alpha here does nothing
    edgecolor=("black", 1),  # alpha here does nothing too
    # alpha=0.5
)

The output:

image

pawjast avatar Aug 12 '24 15:08 pawjast

Hi @pawjast, I think that is not specific to indicate_inset[_zoom] but a more general problem of not being able to set separate face and edge alphas. I thought I had seen another issue about that recently but can’t find it now. Anyway I think it is out of scope for this PR - sorry!

rcomer avatar Aug 12 '24 17:08 rcomer

but a more general problem of not being able to set separate face and edge alphas

I think that should generally work https://matplotlib.org/stable/users/prev_whats_new/whats_new_3.8.0.html#id14 ?

story645 avatar Aug 12 '24 17:08 story645

Fair enough.

I'll have a look at the open issues and try to find it. Otherwise I'll raise a separate bug.

Hi @pawjast, I think that is not specific to indicate_inset[_zoom] but a more general problem of not being able to set separate face and edge alphas. I thought I had seen another issue about that recently but can’t find it now. Anyway I think it is out of scope for this PR - sorry!

pawjast avatar Aug 12 '24 17:08 pawjast

Huh. Turns out it works if you actively set alpha=None, but I had to study the code to figure that out....

rcomer avatar Aug 12 '24 19:08 rcomer

Huh. Turns out it works if you actively set alpha=None, but I had to study the code to figure that out....

Oh, that's cause global alpha is supposed to take precedence, but I think we didn't factor in all the places where we set a default alpha value 😦

ETA: what I mean is f(facecolor=(c1, a1), edgecolor=(c2, a2), alpha=a3), the a3 takes precedence over the alphas in each color.

story645 avatar Aug 12 '24 19:08 story645

Huh. Turns out it works if you actively set alpha=None, but I had to study the code to figure that out....

So it sort of works but needs a better documentation?

pawjast avatar Aug 12 '24 20:08 pawjast

I hid the alpha discussion because that was addressed by #28710 and is not needed for the reviewers of this PR.

rcomer avatar Aug 14 '24 06:08 rcomer

Decide what, if anything, to do about get_window_extent/get_tightbbox

It is also used by fig.savefig(..., bbox_inches='tight') so if we do not implement it then there is a high risk that people using the inline backend may be able to drive them selves to a state where we clip something we should not have, however as both the host and inset axes do implement those methods, I'm struggling to come up with an arangement of the two axes that would result in the connector getting clipped...

When you said "container" I was worried you were following what we do with e.g. error bar where we return a tuple subclass, but I think what you implemented is the right way to go.

tacaswell avatar Aug 15 '24 14:08 tacaswell

So should I have the get_window_extent just return the rectangle’s window extent? It’s entirely possible that the rectangle could be placed halfway out of the main axes. I’m not sure why anyone would want that but that doesn’t mean that nobody will.

rcomer avatar Aug 15 '24 19:08 rcomer

I’m not sure why anyone would want that but that doesn’t mean that nobody will.

What happens when the box gets dragged/can it get dragged by the user out of bounds?

story645 avatar Aug 15 '24 19:08 story645

I’m not sure why anyone would want that but that doesn’t mean that nobody will.

What happens when the box gets dragged/can it get dragged by the user out of bounds?

I don’t know about dragging but if you pan/zoom it can end up outside the axes.

image

rcomer avatar Aug 15 '24 19:08 rcomer

why did the lower left and upper right segments disappear?

story645 avatar Aug 15 '24 20:08 story645

I don’t know about dragging but if you pan/zoom it can end up outside the axes.

yeah, that is all a bit tricky. The rectangle would ideally clip to the parent axes; probably easy. The connectors would ideally clip the parent axes as well, but only on the side that links to the rectangle. I'm not aware that we have a way to do that and I'm not sure it's worth the hassle to make it. Either you are doing this for data exploration, in which case a little mess is OK, or you are doing it for a final plot, in which case you can clean up the positions then.

jklymak avatar Aug 16 '24 00:08 jklymak

why did the lower left and upper right segments disappear?

You mean the connectors? Only 2 of those are visible by default. https://matplotlib.org/stable/gallery/subplots_axes_and_figures/zoom_inset_axes.html

rcomer avatar Aug 16 '24 07:08 rcomer

why did the lower left and upper right segments disappear?

You mean the connectors? Only 2 of those are visible by default. https://matplotlib.org/stable/gallery/subplots_axes_and_figures/zoom_inset_axes.html

Thanks! Sorry for not checking the docs 🤦‍♀️ Am clearly confused by that API choice but is out of scope here.

story645 avatar Aug 16 '24 14:08 story645

Is this no longer PoC (as in title)?

QuLogic avatar Sep 12 '24 19:09 QuLogic

I just noticed I forgot to update the deprecation warning when the patch became an artist. Now fixed.

rcomer avatar Sep 17 '24 18:09 rcomer