FIX: use cached renderer on Legend.get_window_extent #11971
Conversation
lib/matplotlib/tests/test_legend.py
Outdated
| fig.canvas.draw() | ||
|
|
||
| leg.get_window_extent() | ||
| leg2.get_window_extent() |
There was a problem hiding this comment.
There is no assert in here. What exactly should this test? Just that get_window_extent() can be called without renderer?
The comment does tell differently. It says something about the "expected extent" and "various dpi". Both of which I don't see in the test code.
There was a problem hiding this comment.
I was lazy and did not remove the comment.
Yes, the test is just that this does not raise.
e1e4307 to
873ae23
Compare
| 'Return extent of the legend.' | ||
| return self._legend_box.get_window_extent(*args, **kwargs) | ||
| if renderer is None: | ||
| renderer = self.figure._cachedRenderer |
There was a problem hiding this comment.
So, we have lots of get_window_extent(renderer=...) in the code base. Is it all necessary or could a lot of it be safely replaced by this? I guess I can understand for objects that don't have a reference to their figure, but in general, I think most objects know how to get back to their containing figure...
There was a problem hiding this comment.
I don't think we want to always fall back to the cached renderer. There are some use-cases (such as the mixed-mode vector/raster cases where we need to be able to specify which renderer we want to be using with out having to propgate that state globally.
There was a problem hiding this comment.
I think the mixed mode renderer is the only case where this could be a problem, right?
But in fact even for the mixed mode renderer we're not doing this correctly right now, because artist.draw(renderer) will store the instance of MixedModeRenderer as the _renderer attribute, so e.g. even if the artist was drawn rasterized, by the end of the draw the MixedModeRenderer will have reset itself to vectorized and later calls to get_window_extent() will use the cached MixedModeRenderer in the vectorized state.
| @@ -976,9 +976,11 @@ def get_title(self): | |||
| 'Return the `.Text` instance for the legend title.' | |||
| return self._legend_title_box._text | |||
|
|
|||
| def get_window_extent(self, *args, **kwargs): | |||
| def get_window_extent(self, renderer=None): | |||
There was a problem hiding this comment.
That's technically a backward incompatible change (although, as I've mentioned in a previous PR doing something similar, I think this is a change that is worth making.)
|
Thanks @tacaswell |
|
I still get a kick when my PRs get merged 😄 |
…971-on-v3.0.x Backport PR #11971 on branch v3.0.x (FIX: use cached renderer on Legend.get_window_extent)
|
I have the same issue using seaborn. |
|
@Dannyad84 with what version of matplotlib? |
|
n/m had the wrong version. Updated and it worked. Thanks for the quick response!! ;) |
Closes #11970
PR Summary
PR Checklist