MNT: Don't require renderer for window_extent and tightbbox

[jklymak]

PR Summary

This PR is a proof of concept of making renderers globally optional. We don't need to use this in tight_layout because it has a funky _draw_disabled context. However, constrained_layout can have all the renderer tracking pulled out and still works in the tests and interactively so far as I can tell.

See #22744 for more discussion, but I think deciding on the renderer at the Figure level is the right thing to do, and stop trying to sort it out at a higher level.

See also:

• Add a Figure._get_cachedRenderer() method #21319
• FIX: Make sure a renderer gets attached to figure after draw #21758

PR Checklist

Tests and Styling

• Has pytest style unit tests (and pytest passes).
• Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

• New features are documented, with examples if plot related.
• New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
• API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
• Documentation is sphinx and numpydoc compliant (the docs should build without error).

[jklymak]

This is the main new (private) method. Users should generally not need to fetch the renderer going forward...

[greglucas]

I find the naming a bit confusing. I'd suggest just _get_renderer(self), the caching should be an implementation detail. The if self.cachedRenderer is not None seems like it should be implemented in the self.canvas.get_renderer() method (and indeed, there is some logic to re-use the Agg renderer for this)

matplotlib/lib/matplotlib/backends/backend_agg.py

Lines 415 to 425 in 7b2d828

	def get_renderer(self, cleared=False):
	w, h = self.figure.bbox.size
	key = w, h, self.figure.dpi
	reuse_renderer = (hasattr(self, "renderer")
	and getattr(self, "_lastKey", None) == key)
	if not reuse_renderer:
	self.renderer = RendererAgg(w, h, self.figure.dpi)
	self._lastKey = key
	elif cleared:
	self.renderer.clear()
	return self.renderer

[jklymak]

Hmm, for some reason .Artist.get_tightlayout doesn't resolve. .Artist.get_window_extent does no problem.

[tacaswell]

I think that for internal calls, if we have the renderer we should pass it through rather than relying on the cache to implicitly pass it to something deep in the call stack if it is needed.

[jklymak]

I think that for internal calls, if we have the renderer we should pass it through rather than relying on the cache to implicitly pass it to something deep in the call stack if it is needed.

Maybe? However, a lot of the time we are only fetching the renderer to pass it through.

[greglucas]

Would it be possible to use fig.canvas.get_renderer() everywhere? It seems like the get_renderer() call should handle the caching if it can.

[jklymak]

@greglucas I think that's what the new fig._get_cached_renderer does (if it needs to)?

The point here is that all the artists know what their figure is, so if they need the renderer, they can just ask their figure for it. However, it seems a considerable simplification not to ask users to get the renderer.

[greglucas]

Overall I think this is a nice simplification for users to have the renderer sorted out "under the hood" for them rather than having to explicitly pass in a renderer. Just a few comments wondering about where the lines are drawn for when the renderer gets requested (get_tightbbox, window_extent, somewhere else...) and which object/method's job it is to keep track of the caching.

[greglucas]

Can you remove this entirely? It looks like this is only used within LayoutEngine now, so could be replaced there.

[jklymak]

Yep

[greglucas]

Is there a reason to put this inside get_tightbbox() versus get_window_extent()? It looks like further down you put it inside the get_window_extent() call, so I wasn't following where this is needed versus where None could be passed through...

[jklymak]

The locator needs the renderer, however, its debatable whether we should remove that as well.

[greglucas]

I find the naming a bit confusing. I'd suggest just _get_renderer(self), the caching should be an implementation detail. The if self.cachedRenderer is not None seems like it should be implemented in the self.canvas.get_renderer() method (and indeed, there is some logic to re-use the Agg renderer for this)

matplotlib/lib/matplotlib/backends/backend_agg.py

Lines 415 to 425 in 7b2d828

	def get_renderer(self, cleared=False):
	w, h = self.figure.bbox.size
	key = w, h, self.figure.dpi
	reuse_renderer = (hasattr(self, "renderer")
	and getattr(self, "_lastKey", None) == key)
	if not reuse_renderer:
	self.renderer = RendererAgg(w, h, self.figure.dpi)
	self._lastKey = key
	elif cleared:
	self.renderer.clear()
	return self.renderer

[jklymak]

Changed the name to _get_renderer

[jklymak]

Marking as "Needs discussion" because I think there are ideas to go the other way here and make Artists more independent of the figure. I guess if that is really going to happen, then we should think about this. But I'm unconvinced the piping necessary to make Artists portable between figures is worth the considerable complication.

[jklymak]

@tacaswell I doubt I can attend the weekly meeting (travelling), but feel free to discuss this and decide whether we think we will really ever disentangle artists from the figure that owns them.

[anntzer]

FWIW, note that right now some artists can end up being drawn without knowing who their parent figure is, e.g. the PathClippedImagePatch.bbox_image attribute in the demo_text_path.py example (try printing p.bbox_image.figure at the end of the script, for example).

Perhaps we may consider that a bug and state that if you set up artists manually, it is your responsibility to make sure that they know their parent figure, but that would probably need to be documented.

[jklymak]

I guess I'd consider it a bad idea that when we add them to the figure they do not get the figure info added to them. But we can do that at the add_artist time.

[jklymak]

FWIW, note that right now some artists can end up being drawn without knowing who their parent figure is, e.g. the PathClippedImagePatch.bbox_image attribute in the demo_text_path.py example (try printing p.bbox_image.figure at the end of the script, for example).

Perhaps we may consider that a bug and state that if you set up artists manually, it is your responsibility to make sure that they know their parent figure, but that would probably need to be documented.

Following up on this, yes, I think this is a bug. The example even says that the results are dpi dependent - I don't understand how this is even supposed to work if the artist doesn't know its figure.

[anntzer]

Sure. If you don't want to fix that example here, can you open a separate issue to track that problem? I think we also need to state explicitly somewhere in the docs (and changelog) that artists must have their .figure attribute set.

[jklymak]

I take it back about that being a bug. Whether an artist has a parent figure or not depends on the artists, as you point out. This PR doesn't really worry about that - it is not trying to force all artists to not require a renderer for their get_window_extent, its just trying to make the argument optional for those artists that already have enough info to not need the renderer.

In the example you provided of BboxImage, it requires a renderer, and indeed in the fallback in BboxImage doesn't work (it calls get_figure() which returns None). I imagine the same problem occurs for any Artist not directly added to an Axes. I don't think this PR makes the situation any worse (you can still always pass a renderer to get_window_extent), and still simplifies things for most artists.

Note that the layout managers, where the extents are mostly used internally, won't drill down beyond the artists in the axes. Downstream libraries don't need to use this simplification if they don't want (they can continue to pass the renderer). The only disadvantage is a bit of inconsistency for users knowing whether an artist needs the renderer passed or not. However, most folks asking for the bbox are presumably doing something semi complicated, and should be able to handle the inconsistency (or just always pass renderer if in doubt).

[tacaswell]

My understanding of how we got here is:

• some operations on some Artists do indeed require a live renderer. However not all artists actually need the renderer
• in some of those methods the renderer was required and in others it was optional and tried to grab a cached one from someplace
• this centralizes all of the caches in one place and makes the renderer optional all the places

I think that the source of the bugs here was that anything was accessing a cached renderer and my instinct would be to go the other way and try to remove the cache ("There are are two hard problems in computer science: naming things, cache invalidation, and off-by-one bugs"). However, it is probably too late for that (as we have long had the cache and it is useful even if it gives the users a foot-cannon) so centralizing it is a net good, however we should view this cache as for the users not for us to use internally. I think it is much better for users to use our cache rather than keep their own cached renderer (which is definitely going to go out of sync because we do not know about it to invalidate it).

One thing done in this PR with I'm very 👎🏻 on is to then rely on this central cache for internal operations. I think it bakes too many assumption that a (should be optional) cache is definitely there and that the reverse connections up the Artist tree are there. I am also concerned that this is a private method so that down-stream library should probably not use it.

On a bit of a philosophical note, mpl as three layers the chrome on top (plt.plot / ax.plot / fig.subplots() / etc ) that are basically Artist factories, a middle layer which is the Artist instances which hold buckets of state, and then the draw method + renderers that produce an output that someone outside of mpl knows what to do with. Part of the state the Artsits hold is the actual data, part of the state is how to visualize it (which we specify via the class of the instance), part of the state is any "styling" like color, line style, ... (which really should be thought of as data but for another day), part of the state is some intermediate transformation (e.g. norms and colormaps for ScalarMappables), part of it is the "data to display" transformations, and part of it is a hierarchy of Artists. Many (if not all) of our difficulties come from the fact that we tend to mix up all of these kinds of state and that various parts of the library "know too much" about other parts. Because they know too much we eventually start to rely on that knowledge which leads to very brittle cross connections in the code base.

I think the medium term program we are currently engaging in is to start to tease apart all of this state into parts that are clearer and easier to reason about. One place where we get ourselves into consistent trouble is that we have lots of circular references: Figures know about their Axess and Axess know what Figure they are in and so on through the entire Artist tree. However, I am not 100% sure that we actually need the back references! Taking a look at where else we actually use Artist.figure it is mostly in the picking logic and accessing the figure transforms / dpi. It does not look trivial to eliminate the back reference right now, but it at least looks plausible. I would very much prefer that we do not further bake in the that these back connections exist (which, fair, they do) when we have an option not to.

---

I think that we should not change the signature of any of the methods, but allow None to be passed in to mean "go get the cached one if you can". I am slightly skeptical of this step being able to create and cache a renderer and very skeptical that we want to be relying on the cache when we have access to the actual renderer already.

---

The arguemnet here is is it "simpler" to do

def foo(obj):
    a = obj.a
    ....

my_obj.a = 'a'
foo(my_obj)

or

def foo(obj, a):
    ...

foo(my_obj, 'a')

I would argue that in cases where .a exists only to smuggle inputs into foo (rather than being something inherent about the data model of obj) then the second case is clearer and more explicit.

In this case I have the position that Artist.figure is not an inherent part of the Artist and failing that, a cached renderer is not an inherent part of what a Figure is, thus anyplace we need a renderer, it should be required in the signature (with an option of "do what I mean" API of taking None for users to opt-into caching).

[tacaswell]

I left a long essay. Anyone can dismiss this.

[jklymak]

Thanks @tacaswell for your thoughts.

This PR does two things:

1. makes renderer and optional kwarg for all of get_tightbbox and get_window_extent calls in the library. If not passed, we try and access the cache, if the renderer is needed at all (its usually is not). My read is that you are OK with this as a public interface?
2. it removes the renderer calls in _constrained_layout and _tight_layout. I don't see that any other internal code is changed. I can easily back this part out, I just was considering it as eating our own dogfood, rather than an important part of the PR. It also simplifies snaking renderer through a bunch of calls, but that really doesn't matter a lot, and indeed is maybe microscopically faster to only call get_renderer once.

So far as we can tell, we only explicitly need renderer for text items, we have no idea how big they are until we render them.

WRT whether we should stash figure on artists, so far as I can tell we already stash figure information on artists via the transform attribute. I think that as long as we do that, we should also explicitly add the figure. In general, I think trees are much easier to use if children know their parents. If you need to prune a child and graft to another tree, that's fine, but in general it usually makes life a lot easier if you can walk up a tree as well as down it. Artist.set_figure has been part of the library for at least 18 years, and I'm not clear what the practical advantage of backing it out would be.

[jklymak]

Latest commit reverts the changes in _constrained_layout and contour.py where the new version did not explicitly pass in the renderer. Those were the only internal uses I think the old version was changing.

[anntzer]

FWIW I have been wanting to make renderer optional for a long time too (because threading it through layers and layers of calls just so that texts can know their extents is a bit annoying). While I also agree with @tacaswell's point that cache invalidation is tricky, note that the question here is about the invalidation of figure._cachedRenderer; it doesn't really matter, IMO, whether artist.get_window_extent() fetches the renderer from artist.figure or whether we pass the renderer to it explicitly (because if we're doing this in a non-draw context, we most likely are layouting some parent artist (ultimately the toplevel figure) and are grabbing the renderer from figure._cachedRenderer anyways)? IOW, unless we carefully start avoiding refering to artist.figure, we're not really making the cache problem worse?

[anntzer]

I don't think you need to normalize here; just let the labels do it?

[jklymak]

That depends not he strategy we want to employ - it could wait to normalize until the end, but this does save a bunch of calls to _get_renderer.

[anntzer]

Fair.

[jklymak]

@tacaswell, can you reassess now that the internal uses have been cleared up. Note that this also cleans up our internal definition of the renderer to one location.

[tacaswell]

I am on board with this 👍🏻