8000 ENH have ax.get_tightbbox have a bbox around all artists attached to axes. by jklymak · Pull Request #10682 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

jklymak
Copy link
Member
@jklymak jklymak commented Mar 5, 2018

PR Summary

This PR takes the code that was in backend_bases.py in savefig for determining a figure bounding box, and puts it in ax.get_tightbbox and fig.get_tightbbox, via a refactoring into Artists.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

import matplotlib.pyplot as plt

fig, ax = plt.subplots(figsize=(5,2))
ax.set_xlim([0, 1])
ax.text(1, 0.5, 'This is a long overflow')
print('before', ax.get_tightbbox(fig.canvas.get_renderer(),bbox_extra_artists=[]))
print('now   ', ax.get_tightbbox(fig.canvas.get_renderer()))

returns:

before Bbox(x0=61.2, y0=-3.4, x1=922.125, y1=363.0)
now    Bbox(x0=61.2, y0=-3.4, x1=1210.25, y1=363.0)

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:

fig, ax = plt.subplots(figsize=(5,2))
ax.set_xlim([0, 1])
t = ax.text(1, 0.5, 'This is a long overflow')
t.inbbox = True  # default
print('inbbox true', ax.get_tightbbox(fig.canvas.get_renderer()))
t.set_in_layout(False)
print('inbbox false ', ax.get_tightbbox(fig.canvas.get_renderer()))
t.set_in_layout(True)
print('inbbox true ', ax.get_tightbbox(fig.canvas.get_renderer()))

and get

in_layout  true  Bbox(x0=61.18055555555556, y0=-3.4444444444444535, x1=1210.25, y1=363.0)
in_layout false  Bbox(x0=61.18055555555556, y0=-3.4444444444444535, x1=922.125, y1=363.0)
in_layout  true  Bbox(x0=61.18055555555556, y0=-3.4444444444444535, x1=1210.25, y1=363.0)

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 to False by default? I'm somewhat against this because it means users have to guess or query what state the attribute is in...

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • Documentation is sphinx and numpydoc compliant
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak added topic: geometry manager LayoutEngine, Constrained layout, Tight layout API: consistency API: changes labels Mar 5, 2018
@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch 2 times, most recently from ec57567 to c53836c Compare March 5, 2018 15:42
@jklymak jklymak added this to the v2.2.1 milestone Mar 5, 2018
@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch 2 times, most recently from f93b291 to 650f378 Compare March 5, 2018 20:02
@tacaswell tacaswell modified the milestones: v2.2.1, v3.0 Mar 5, 2018
@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch 4 times, most recently from 2660b18 to 30edb1f Compare March 6, 2018 05:06
@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch from 30edb1f to 485dd68 Compare March 24, 2018 21:07
@ImportanceOfBeingErnest
Copy link
Member

Is this meant to be used by tight_layout, such that one could call fig.tight_layout(bbox_extra_artists=[])?

@jklymak
Copy link
Member Author
jklymak commented Apr 12, 2018

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).

@jklymak
Copy link
Member Author
jklymak commented Apr 29, 2018

See update above: rebased and added artist.inbbox to determine if an artist is in a bbox calculation or not.

@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch 2 times, most recently from 41727bd to fd918ca Compare April 29, 2018 15:38
@tacaswell
Copy link
Member

I am happy with this in principle, but we probably need to add the get_* and set_* methods to keep the API guessable.

We need a better name than inbbox as bbox is a bit arcane and inbbox reads to be as a question not an instruction.


def get_tightbbox(self, renderer, call_axes_locator=True):
def get_tightbbox(self, renderer, call_axes_locator=True,
bbox_extra_artists=None):
Copy link
Member

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.

Copy link
Member Author

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?)

@jklymak
Copy link
Member Author
jklymak commented Apr 30, 2018

added setters and getters, and changed name to artist.set/get_in_layout and made artist._in_layout private.

@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch from 7e5a28e to b352dbf Compare April 30, 2018 21:31
@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch from 5bada26 to 6e60ecf Compare July 5, 2018 21:30
@jklymak
Copy link
Member Author
jklymak commented Jul 5, 2018

@efiring Thanks for the review. Outstanding questions for others:

  1. should this be a artist.set/get_in_layout or should it just be a property (artist.in_layout)?
  2. Am I referring to the co-oridnate system of the bounding boxes correctly? get_tightbbox returns bounding boxes in pixels relative to the 0, 0 as the bottom left of the figure viewport. I'm referring to this as:
bbox : `.BboxBase`
            containing the bounding box (in figure pixel co-ordinates).

I don't think this was ever properly specified before this PR. Probably because get_tightbbox and get_window_extents verge on private methods.

@timhoffm
Copy link
Member
timhoffm commented Jul 5, 2018

re 1. From the pythonic point of view this would just be an attribute artist.in_layout. A property does not make sense here. Properties are basically there to attach code to reading or writing an attribute without changing the signature. You can just use the attribute and turn it into a property later if needed.

The only argument for set/get would be that it's widely used within matplotlib. However there are also a few counter-examples artist.axes, artist.stale.

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.

@jklymak
Copy link
Member Author
jklymak commented Jul 5, 2018

From the pythonic point of view this would just be an attribute artist.in_layout.

Sorry, meant "attribute".

OTOH I think the issue is documentation - how does the user discover this attribute in the docs?

@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch from 6e60ecf to 0ef8d7a Compare July 5, 2018 22:10

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.

@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch 5 times, most recently from 25c45b2 to b31f829 Compare July 7, 2018 23:00
@jklymak
Copy link
Member Author
jklymak commented Jul 8, 2018

I’d be all for either form, with slight preference for the attribute. @tacaswell?

@jklymak jklymak force-pushed the enh-ax-get-tightbbox branch from b31f829 to b18d16c Compare July 9, 2018 19:20
@efiring
Copy link
Member
efiring commented Jul 9, 2018

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.

Copy link
Member
@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditional on CI

@jklymak
Copy link
Member Author
jklymak commented Jul 9, 2018

self merging as reviewed and passed CI

@jklymak jklymak merged commit 632305e into matplotlib:master Jul 9, 2018
@jklymak jklymak deleted the enh-ax-get-tightbbox branch July 9, 2018 20:09
@jklymak
Copy link
Member Author
jklymak commented Jul 10, 2018

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....

@breedlun
Copy link
Contributor

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 ax.get_tightbbox() was not returning the same result as it did under Matplotlib v2.2.3. I am, however, pleased that I can get the old behavior with ax.get_tightbbox(bbox_extra_artists=[]). Thanks for keeping some degree of backwards compatibility.

@jklymak
Copy link
Member Author
jklymak commented Oct 20, 2020

You can also twiddle manually: artist.set_in_layout(False)...

breedlun pushed a commit to breedlun/clearplot that referenced this pull request Oct 20, 2020
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: changes API: consistency Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0