-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH have ax.get_tightbbox have a bbox around all artists attached to axes. #10682
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
ec57567
to
c53836c
Compare
f93b291
to
650f378
Compare
2660b18
to
30edb1f
Compare
30edb1f
to
485dd68
Compare
Is this meant to be used by |
I think thats possible, though this PR doesn't do that. Not quite sure what happens if you specify an artist that isn't in the axes (tight layout would have to accept all the artists for each axes). |
b90b15f
to
d948583
Compare
See update above: rebased and added |
41727bd
to
fd918ca
Compare
I am happy with this in principle, but we probably need to add the We need a better name than |
lib/matplotlib/axes/_base.py
Outdated
|
||
def get_tightbbox(self, renderer, call_axes_locator=True): | ||
def get_tightbbox(self, renderer, call_axes_locator=True, | ||
bbox_extra_artists=None): |
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.
please do not add this extra kwarg.
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.
Sorry, I screwed up the PR during a rebase... The reason for this is so that the artist logic that was in backend_bases.print_figure
could be refactored out and reused. So this needs to be here to make that
function work. Could make a private kwarg (is there such a thing?)
added setters and getters, and changed name to |
7e5a28e
to
b352dbf
Compare
5bada26
to
6e60ecf
Compare
@efiring Thanks for the review. Outstanding questions for others:
I don't think this was ever properly specified before this PR. Probably because |
re 1. From the pythonic point of view this would just be an attribute The only argument for set/get would be that it's widely used within matplotlib. However there are also a few counter-examples It's up to one of the more senior devs to decide which way to go. Personally, I'd have a slight preference for the attribute solution. |
Sorry, meant "attribute". OTOH I think the issue is documentation - how does the user discover this attribute in the docs? |
6e60ecf
to
0ef8d7a
Compare
An attribute can have a docstring, and sphinx can include the documentation either by listing the attribute along with other explicit members, or via the autoattribute directive: http://www.sphinx-doc.org/en/stable/ext/autodoc.html I think @tacaswell really doesn't want to start doing this (using bare attributes) until the massive traitification is done, however. But maybe he will change his mind for cases like this. |
25c45b2
to
b31f829
Compare
I’d be all for either form, with slight preference for the attribute. @tacaswell? |
b31f829
to
b18d16c
Compare
I accept @tacaswell's argument that keeping the machinery provided by the setters and getters is worthwhile for this, so ignore my suggestion to reduce it to a bare attribute. |
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.
conditional on CI
self merging as reviewed and passed CI |
Grrrrr. This has a funny interaction w/ zooming. If I zoom, then the clip path of an artist is not set properly until after a draw. But all the automatic layout depends on being able to get a sensible tighbbox during draw... Darn race conditions.... |
I just spent several hours tracking down why my annotations were getting placed incorrectly after updating from Matplotlib v2.2.3 to Matplotlib v3.3.1. The problem turned out to be |
You can also twiddle manually: |
…udes tick marks and tick mark labels, but not the axes lables, axes title, or other annotations. As of matplotlib 3.0, however, mpl_ax.get_tightbbox() returns the bounding box that includes the axes labels, axes title, and other annotations (see matplotlib/matplotlib#10682). This new behavior was messing up my axis label placement. Fortunately, one can still get the original behavior via mpl_ax.get_tightbbox(bbox_extra_artists = []), so the axes.tight_bbox property was updated to use this new kwarg.
PR Summary
This PR takes the code that was in
backend_bases.py
insavefig
for determining a figure bounding box, and puts it inax.get_tightbbox
andfig.get_tightbbox
, via a refactoring intoArtists.get_tightbbox
.Supercedes #10678 and extends already merged #9164
In previous PRs I was piecemeal adding new types of artists, but noted that
fig.savefig(bbox_inches='tight')
was actually doing the correct thing and putting the bbox around all the artists.To get the old behaviour back, a new kwarg has been added:
ax.get_tightbbox(bbox_extra_artists=[])
So now the following code
returns:
This is a breaking change, in that anyone who was expecting the bbox to not include artists, but just the axes axis elements and labels, will now get a different behaviour. They can get the old behaviour as above (specifying the empty
bbox_extra_artists
kwarg. OTOH, willing be told this is too big an API change. (it of course still needs an API change note, and likely a test or two somewhere).UPDATE 28 April:
This adds the artist property
artist.set/get_in_layout
(as discussed w/ @efiring et al on the weekly call) which specifies if an artist should be included in the tight_bbox calculation. This allows toggling artists in the tight layout or constrained layout calculations.So, now we can also do:
and get
Note that this also works for legends, which often get put outside axes and hence may not want to be part of the tight_layout machinery.
Discussion: Should artists that did not get included in the old
tight_layout
be set toFalse
by default? I'm somewhat against this because it means users have to guess or query what state the attribute is in...PR Checklist