Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

```bbox_inches="tight"``` support for *all* figure artists. #1448

Merged
merged 2 commits into from

4 participants

@pelson
Collaborator

This is a first pass at making bbox_inches="tight" work for anything you can throw at a figure.

I think @leejjoon implemented this functionality originally, so it would be great to get some feedback from him as to whether this is a good approach or not.

My motivation for this has been seeing all of the PRs/issues that have gone by lately relating to bbox_inches="tight", most of these have come as a result of iPython defaulting to using this setting (@fperez may be interested in this PR therefore), hopefully this should resolve all outstanding issues on that front (a lot of them have actually already been fixed with bespoke solutions, but inevitably there will be other cases which have not been specifically targeted, hence this PR).

Personally, I still feel that bbox_inches="tight" is an ingenious hack which I would sooner see done better by a geometry manager (#1109), but in lieu of that functionality, getting this right is a worthy pragmatic short-to-medium term goal.

Thanks for your time,

@pelson
Collaborator

Please note: I am working on my mac and have a couple of concerns about the fonts of the result images (that may well be unfounded). I won't be able to re-generate them with a more reliable freetype until Monday morning.

@pelson pelson commented on the diff
.travis.yml
@@ -14,4 +14,4 @@ install:
script:
- mkdir ../foo
- cd ../foo
- - python ../matplotlib/tests.py
+ - python ../matplotlib/tests.py -sv
@pelson Collaborator
pelson added a note

Note: This means we can see if any of the tests are producing standard out (if you're prepared to look at the travis log), and to determine easily which test is producing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jenshnielsen
Owner

Looks good to me. How about getting rid of the text in the table to make the test less likely to fail with different
freetype versions. Something like the following

import matplotlib.text as text
data = [[  66386,  174296,   75131,  577908,   32015],
        [  58230,  381139,   78045,   99308,  160454],
        [  89135,   80552,  152558,  497981,  603535],
        [  78415,   81858,  150656,  193263,   69638],
        [ 139361,  331509,  343164,  781380,   52269]]

colLabels = ('Freeze', 'Wind', 'Flood', 'Quake', 'Hail')
rowLabels = ['%d year' % x for x in (100, 50, 20, 10, 5)]
rows = len(data)
ind = np.arange(len(colLabels)) + 0.3  # the x locations for the groups
cellText = []
width = 0.4     # the width of the bars
yoff = np.array([0.0] * len(colLabels))
cols = ['r', 'g', 'b', 'k', 'y']
cols2 = cols2 = 5*[cols]
# the bottom values for stacked bar chart
fig, ax = plt.subplots(1,1)
for row in xrange(rows):
    plt.bar(ind, data[row], width, bottom=yoff)
    yoff = yoff + data[row]
    cellText.append(['%1.1f' % (x/1000.0) for x in yoff])
plt.xticks([])
plt.legend(['1', '2', '3', '4', '5'], loc = (1.2, 0.2))
# Add a table at the bottom of the axes
cellText.reverse()
the_table = plt.table(loc='bottom',rowColours=cols, colColours=cols, cellColours=cols2)
@jenshnielsen jenshnielsen referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@jenshnielsen jenshnielsen referenced this pull request from a commit in jenshnielsen/matplotlib
@jenshnielsen jenshnielsen verbost output from travis. Taken from #1448 4b6f587
@pelson
Collaborator

@mdboom - would you mind having a look over this? If you'd rather not have this PR merged, then I won't bother rebasing and bringing it up to speed, but if you think its worthwhile I will.

Cheers,

lib/matplotlib/artist.py
@@ -180,6 +180,15 @@ def get_axes(self):
"""
return self.axes
+ def get_window_extent(self, renderer):
+ """
+ get the axes bounding box in display space.
+ Subclasses should override for inclusion in the bounding box
+ "tight" calculation. Default is to return an empty bounding
+ box at 0, 0.
+ """
+ return Bbox([[0, 0], [0, 0]])
+
@dmcdougall Collaborator

If a subclass does not override this method. Will bbox_inches='tight' result in a figure that always contains the point (0, 0)? If so, would it be better to raise NotImplementedError instead?

@pelson Collaborator
pelson added a note

Bboxes with a width or height of 0 are not included in the final Bbox calculation, so in truth, the Bbox could be based anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/tests/test_bbox_tight.py
@@ -36,6 +39,48 @@ def test_bbox_inches_tight():
the_table = plt.table(cellText=cellText,
rowLabels=rowLabels,
colLabels=colLabels, loc='bottom')
+
+
+@image_comparison(baseline_images=['bbox_inches_tight_suptile_legend'],
+ remove_text=False, savefig_kwarg={'bbox_inches': 'tight'})
+def test_bbox_inches_tight_suptile_legend():
+ plt.plot(range(10), label='a straight line')
+ plt.legend(bbox_to_anchor=(0.9, 1), loc=2, )
@dmcdougall Collaborator

PEP8: Trailing comma and space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/backend_bases.py
@@ -2079,15 +2079,28 @@ def print_figure(self, filename, dpi=None, facecolor='w', edgecolor='w',
renderer = self.figure._cachedRenderer
bbox_inches = self.figure.get_tightbbox(renderer)
- bbox_extra_artists = kwargs.pop("bbox_extra_artists", None)
- if bbox_extra_artists is None:
- bbox_extra_artists = self.figure.get_default_bbox_extra_artists()
+ bbox_artists = kwargs.pop("bbox_extra_artists", None)
+ if bbox_artists is None:
+ bbox_artists = self.figure.get_default_bbox_extra_artists()
+
+ bbox_filtered = []
+ for a in bbox_artists:
+ bbox = a.get_window_extent(renderer)
+ # print('{0:40} - {1.width:5}, {1.height:5} '
+ # '{1}'.format(type(a), bbox))
@dmcdougall Collaborator

Are these comments needed? If not, they can be removed.

@pelson Collaborator
pelson added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff
lib/matplotlib/backend_bases.py
((7 lines not shown))
+ bbox_artists = kwargs.pop("bbox_extra_artists", None)
+ if bbox_artists is None:
+ bbox_artists = self.figure.get_default_bbox_extra_artists()
+
+ bbox_filtered = []
+ for a in bbox_artists:
+ bbox = a.get_window_extent(renderer)
+ # print('{0:40} - {1.width:5}, {1.height:5} '
+ # '{1}'.format(type(a), bbox))
+ if a.get_clip_on():
+ clip_box = a.get_clip_box()
+ if clip_box is not None:
+ bbox = Bbox.intersection(bbox, clip_box)
+ clip_path = a.get_clip_path()
+ if clip_path is not None and bbox is not None:
+ clip_path = clip_path.get_fully_transformed_path()
@dmcdougall Collaborator

PEP8: I have a feeling this line is >= 79 characters. Can you check it?

@pelson Collaborator
pelson added a note

It is indeed == 79 characters - but I believe that is a-ok :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/figure.py
((5 lines not shown))
def get_default_bbox_extra_artists(self):
- bbox_extra_artists = [t for t in self.texts if t.get_visible()]
+ bbox_artists = self.get_children()
@dmcdougall Collaborator

Just a thought. What if some of the child artists are not visible?

@pelson Collaborator
pelson added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/figure.py
((8 lines not shown))
for ax in self.axes:
if ax.get_visible():
- bbox_extra_artists.extend(ax.get_default_bbox_extra_artists())
- return bbox_extra_artists
-
-
+ bbox_artists.extend(ax.get_default_bbox_extra_artists())
+ # we don't want the figure's patch to influence the bbox calculation
@dmcdougall Collaborator

Good thinking. Are there any others we might want to exclude?

@pelson Collaborator
pelson added a note

The tests haven't flagged anything up and I can't think of anything, but that is not a definite "no".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/matplotlib/lines.py
@@ -358,7 +358,7 @@ def set_picker(self,p):
self._picker = p
def get_window_extent(self, renderer):
- bbox = Bbox.unit()
+ bbox = Bbox([[0, 0], [0, 0]])
@dmcdougall Collaborator

Again, is this the correct solution?

@pelson Collaborator
pelson added a note

I think it's fine. See above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall
Collaborator

I would like this to go in v1.3, it would be a lovely enhancement. Thank you for the hard work you've put into this, @pelson.

If there is consensus to include this, this PR needs a CHANGELOG a entry. A nice example would be nice, too. I like that you have tested this well! Good job.

@dmcdougall dmcdougall was assigned
@pelson
Collaborator

Thanks Damon. I'll rebase and address your comments subsequently.

@efiring
Owner

@pelson, it looks like it would be nice to get this merged soon. I agree with your sentiments expressed in your opening comment.

@dmcdougall
Collaborator

There were some python 2.x build errors. I've restarted the build.

@dmcdougall dmcdougall merged commit 8605579 into matplotlib:master
@jenshnielsen jenshnielsen referenced this pull request from a commit in jenshnielsen/matplotlib
@jenshnielsen jenshnielsen verbost output from travis. Taken from #1448 3eb6d96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 13, 2013
  1. @pelson
  2. @pelson

    Review actions.

    pelson authored
Something went wrong with that request. Please try again.