Repository navigation
[MNT]: sort out renderers #22744
Description
Summary
Various methods require a renderer, in particular get_tightbbox and get_window_extent. We have quite a few inconsistencies here.
Many implementations of get_window_extent have renderer as an optional kwarg, and fallback: fallbacks are self.figure._cachedRenderer (Text, Legend) and some other get_window_extent don't need the renderer.
get_tightbbox always needs the renderer, but largely to pass to get_window_extent calls on Text objects.
tight_layout and constrained_layout need the renderer, but that is only because they want to pass it to the get_tightbbox.
Proposed fix
It seems that we don't need to specify the renderer most of the time in get_window_extent, but can safely fall back on the _cachedRenderer or fig.canvas.get_renderer, so it seems more consistent to just make this argument always optional. However, perhaps I'm missing a subtlety where the user may want to call get_window_extent(renderer=different_renderer), but I have no idea what that use case is.
- make renderer a kwarg in all get_window_extent methods.
- improve the fallbacks in all the get_window_extents: I'm not sure _cachedRenderer is adequate, and we probably want Figure._get_cachedRenderer that has some fallbacks in _cachedRenderer is None, or fakes a draw to fill the cache.
- make renderer an optional kwarg in all get_tightbbox methods.
This needs comment though from folks who understand the renderer system better: @QuLogic @anntzer @tacaswell ? Thanks!
Activity
tacaswell commented on Apr 30, 2022
From a pure implementation point of view we the best case would be if we did not have the cache (as cache invalidation is hard) at all and any method that needs a renderer is passed one in. Anything that is going to know information about going all the way to the screen is going to need a renderer. If any get_window_extent needs the renderer, they should all take it or the signature instability will come back to bite us.
However there are some usability issues with that (as we want to do the layout steps outside of the draw cycle, there are decent reasons to want to know the window extent of something not at draw time for manual layout, and forcing the renderer caching onto the user would just make everything worse). We had some experience with a backend where we could only get a graphics context in the draw loop (the old native OSX backend) and it was very difficult to work with. So the next best thing is if we manage exactly 1 cache of the renderer and access it on behalf of the user when they would need to otherwise provide it.
The worst case appears to be what we have now where we have more than one point of truth and sometimes provide it for the user.
It is also possible that if we change the default conceptual target of the transforms we provide to a virtual (but physical) coordinate system, but I do not have enough of the text code in my head to be sure at the moment.
oscargus commented on Jan 15, 2023
After #24984 at least get_window_extent is (almost) consistent.
The only thing remaining is Text, which has an extra dpi argument. However, simply removing that will only give test failures for the two tests explicitly testing it (locally on my Windows laptop, subject to missing test dependencies etc). There is really no clue to why this is needed (like if some package is relying on it). @anntzer would you know? (To me, you are the text expert.)
anntzer commented on Jan 15, 2023
No particular idea; this was added by @efiring a while ago in f72c17c, perhaps he still has some memory of it?
oscargus commented on Jan 15, 2023
Ahh, sorry, I didn't check the blame...
efiring commented on Jan 15, 2023
The docstring I added in f72c17c gives the rationale: the need to find the pixel location of a text box in a saved png so as to be able to specify the clickable target in a web page. I don't remember whether I put the dpi kwarg in because I needed that flexibility (e.g., find the extent for a figure to be saved later at a different dpi than the one currently used for display), or whether I put it there "just in case". Looking now at one place in my group's code where we are calling Text.get_window_extent for this purpose (use in html), we are calling it with no arguments. It works in this case because we are consistently specifying the dpi and calling the method after saving the figure.
oscargus commented on Jan 15, 2023
Yes, the purpose is clear from the docstring. I was more doubting if we still need it as at least the mpl codebase doesn't seem to use it. But there are clearly other applications relying on it, so I guess no harm to keep it there (despite the minor inconsistency in arguments for different classes).
Then I am tempted to say that this can be closed once #24984 is in.