-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Walk the artist tree when preparing for saving with tight bbox. #14216
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
Conversation
At this point my may as well just let |
So what's the patch you propose? |
I'll test this out tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced we should fix this like this. AstroPy is overriding Axes.get_tightbbox
but not doing all the things Axes.get_tightbbox
does. I think the onus is on AstroPy to inherit get_tightbbox
more robustly, not for us to sledgehammer the issue with an extra draw to the detriment of the vast majority of cases that don't need this.
If there is something we can do to make it easier for AstroPy to customize get_tightbbox
to their needs, we should definitely try and do that. But I think if at all possible they should call super().get_tightbbox
to make sure they stay current with us.
@jklymak - I'd be fine with fixing Astropy - essentially what we need is a way to provide extra bounding boxes of plot items that need to be included in the calculation of the final bounding box. I guess one way would be to simply call Axes.get_tightbbox and then do a union of that and our extra bounding boxes? |
@astrofrog - that'd work; but again, BTW, since we were effectively doing the draw before with |
Ok so I've dug a little deeper into what we do in Astropy, and there are two separate things here - yes we can definitely simplify our In WCSAxes we dynamically draw e.g. the tick labels by setting up a single from matplotlib.axes import Axes
from matplotlib.text import Text
from matplotlib import pyplot as plt
class MyAxes(Axes):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.text = Text(0.5, 0.5, 'Text outside axes',
transform=self.transAxes, clip_on=False,
figure=self.figure, ha='center', va='center')
self._extra_bbox = None
def draw(self, renderer, *args, **kwargs):
print('MyAxes.draw')
result = super().draw(renderer, *args, **kwargs)
self.text.set_position((-0.1, 0.5))
self.text.draw(renderer)
self._extra_bbox = self.text.get_window_extent(renderer)
return result
def get_tightbbox(self, *args, **kwargs):
print('MyAxes.get_tightbbox')
result = super().get_tightbbox(*args, **kwargs)
if self._extra_bbox is None:
return result
else:
return result.union(self._extra_bbox)
fig = plt.figure()
ax = MyAxes(fig, [0.1, 0.1, 0.8, 0.8])
fig.add_axes(ax)
print(ax.figure)
fig.savefig('test.png', bbox_inches='tight') In this case, the label is truncated at the left of the window rather than having the bounding box be adjusted. The reason we dynamically draw the text like this is that we don't know in advance how many tick labels will be drawn and we don't want to keep adding/removing artists from the axes. The only fix I can think of without changing Matplotlib would be for us to have some kind of 'dry run' on the code that draws the artists, and only collects their bboxes but doesn't draw them. This would then get called inside |
OK, so you can't position your Its possible we should re-architect this somehow. Anyway, for now I guess I'm OK w/ this PR as a fix to get us back where we were. |
However, backends that did do something with |
Let's assume that print_foo must walk the artist tree twice when preparing to save with tight bbox (that's the origin of this discussion). The way print_foo is implemented is basically always
With #14134, when doing the "dry_run" call, we would only do the preparation to extract out the renderer object. With this PR, we do the preparation and walk the tree, but still skip the post-walking step. What did backends do to optimize dry_run? The only optimizations right now are
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes Astropy's tests - I do still plan to try and make it so that get_tightbbox
returns sensible results even without requiring Matplotlib to walk the draw tree first, but it will be good to have this fix in in the mean time.
If @astrofrog is happy with this, I am happy too. I am looking forward for this being in dev sooner than later, so we can un-xfail two of our tests over at |
PR Summary
Closes (I think?) #14180 and #14134 (comment).
@astrofrog @pllim can you confirm?
PR Checklist