Configuring automatic use of tight_layout #1136

Merged
merged 2 commits into from May 13, 2013

Conversation

Projects
None yet
6 participants
Contributor

pwuertz commented Aug 22, 2012

This PR extends the introduction of the automatic use of tight_layout (#774)

A user currently cannot make use of the automatic layout for creating figures without padding. Use-cases for border-less figures are graphical user interfaces and embedding of figures in documents. The first commit in the PR fixes that by adding the necessary keywords to the newly introduced tight_layout kwarg in figure.

UPDATE (the following commits have been removed from the PR):
The second commit fixes a usability glitch when manually calling tight_layout. Tight_layout grabs a renderer from the active backend for determining the font metrics. Most probably this is the default renderer from an interactive backend like GtkAgg or QtAgg. If the user now chooses to save figure using the svg backend this causes inconsistent results. Instead of calculating the metrics for the target backend, matplotlib creates a layout based on whichever backend that was previously set. Tight_layout should not be used as a standalone function and the patch makes a call to the old methods enable the automatic use of tight_layout for the given figure.

The third commit updates the baseline images for test_tightlayout since the layouts in the test results slightly differ from the previous references.

Contributor

leejjoon commented Aug 28, 2012

I am not very comfortable with the second commit. My inclination is that we keep "tight_layout" as one-time action for now. I am aware of the usability glitch you mentioned, but if this matters, we can just call set_tight_layout(True)? I expect there are lots of cases that "tight_layout" does not work well, and I guess it helps that we keep "tight_layout" as one-time action command. Other than that, the PR looks good to me.

Contributor

pwuertz commented Aug 28, 2012

Ok.. l removed the second and third commit from the PR. I just thought it could be confusing for users if the one-time action command and the set_tight_layout activation provide different results.

Contributor

pwuertz commented Aug 28, 2012

Can this still be merged for 1.2 ? It's technically not a new feature but corrects a missing functionality in the tight-layout-goes-into-figure patch..

Contributor

leejjoon commented Aug 28, 2012

I guess this is Michael's call, @mdboom, what do you think?

Owner

efiring commented Sep 5, 2012

I haven't looked very closely, but my reactions are
(1) having the additional configurability seems good,
(2) piling on so many additional Figure.init kwargs is not so good,
(3) I'm a little worried about future conflicts between a complex tight_layout and an even more complex layout engine, if that comes to pass.

I don't know what can be done about (3); but for (2), I think that rather than standalone kwargs, it would be better to have a single kwarg, "tight_layout_kw", which would be a dictionary, so the subsequent call would be "tight_layout(renderer, **self.tight_layout_kw)".

An alternative would be to let the existing tight_layout kwarg accept such a dictionary directly.

Contributor

pwuertz commented Sep 5, 2012

@efiring
3. I guess the integration of a layout engine will break many things, making a change in the api inevitable.. something for a matplotlib 2.0 or 3.0 version perhaps. I mean.. you don't want to support concurring layout managers right?
2. What do you think about removing the new parameters from Figure.init? I can still use the setters to get the desired functionality..

Contributor

pwuertz commented Sep 6, 2012

@efiring: Removed the parameters from Figure.init. The automatic use of tight_layout can be configured using the figure's properties.

Owner

efiring commented Sep 6, 2012

@pwuertz, I'm sorry to be slow and indecisive on this, but I think the design needs some additional thought by others, and a clear idea of intended use cases and future evolution, if any. I have no argument with your point that some configurability is needed; it is just a question of how to do it in a way that we will not regret. Longer term, do we need or want all these getters/setters? It is much easier to add to the public API than to remove things from it, so it is worthwhile being a bit cautious. @WeatherGod, @mdboom, any ideas here? Hold off on this until after 1.2? Or merge it? Or use the tight_layout_kw approach I suggested above? Use simple attributes instead of getters/setters? Mark all this "experimental" in docstring and any other documentation?

Contributor

pwuertz commented Sep 6, 2012

@efiring Ok I think I got it: How about adding these as optional parameters to your set_tight_layout method?

Contributor

pwuertz commented Oct 8, 2012

Another proposal that allows you to provide a dictionary for set_tight_layout. No more additional getters/setters.

Contributor

mdehoon commented Nov 17, 2012

I haven't followed this issue from the beginning, but I noticed that demo_tight_layout.py fails with the Mac OS X backend. I mention this here since this problem originates from the fact that tight_layout grabs a renderer from the active backend for determining the font metrics (see pwuertz's first comment). Since tight_layout is called outside of the event loop, at that point under Mac OS X the renderer does not have a valid graphics context associated with it. Demo_tight_layout.py then fails as follows:

$ python -i examples/pylab_examples/demo_tight_layout.py
Traceback (most recent call last):
File "examples/pylab_examples/demo_tight_layout.py", line 15, in
plt.tight_layout()
...
RuntimeError: CGContextRef is NULL

AFAICT this bug is related to the current issue reported by pwuertz. If not, I can open a separate issue for it.

tobson commented Mar 23, 2013

@mdehoon Please open a separate issue.

Contributor

mdehoon commented Mar 24, 2013

@tobson OK, done; see #1852.

@pelson pelson commented on an outdated diff Apr 18, 2013

lib/matplotlib/figure.py
"""
if tight is None:
tight = rcParams['figure.autolayout']
- tight = bool(tight)
- self._tight = tight
+ self._tight = bool(tight)
+ self._tight_parameters = tight if type(tight) is dict else {}
@pelson

pelson Apr 18, 2013

Member

use isinstance here

Member

pelson commented Apr 18, 2013

This has my 👍 once it has a test. @efiring - how do you stand on this PR?

Owner

efiring commented May 1, 2013

@pelson, the implementation looks fine to me, now. Do you want to hold out for a test?

Member

pelson commented May 1, 2013

Do you want to hold out for a test?

It would be nice. I don't have confidence that this wont regress at some point otherwise.

Contributor

pwuertz commented May 2, 2013

The test has been added, but there is something odd about the agg/png test results. The image comparison method doesn't notice it, but the y-label position is shifted to the left. You'll see the difference in tight_layout1.png, whereas the svg and pdf outputs provide the intended results.

In the new tight_layout8 test the padding is reduced further and the ylabel in the agg output is clipped, which is definitely not the desired result.

Contributor

pwuertz commented May 2, 2013

This looks like a baseline vs bottom positioning error in the agg backend. Any chance that this is related to #1810 ?

Contributor

pwuertz commented May 2, 2013

I'll update the png baseline image once #1968 is resolved. Please don't merge until then.

Contributor

pwuertz commented May 2, 2013

Good to go if you are ok with the test.

@pelson pelson and 1 other commented on an outdated diff May 2, 2013

lib/matplotlib/tests/test_tightlayout.py
@@ -135,3 +135,12 @@ def test_tight_layout7():
ax.set_title('Left Title', loc='left', fontsize=fontsize)
ax.set_title('Right Title', loc='right', fontsize=fontsize)
plt.tight_layout()
+
+@image_comparison(baseline_images=['tight_layout8'])
+def test_tight_layout8():
+ 'Test automatic use of tight_layout'
+ fig = plt.figure()
+ fig.set_tight_layout({'pad': .1})
+ ax = fig.add_subplot(111)
+ example_plot(ax, fontsize=24)
+ plt.tight_layout()
@pelson

pelson May 2, 2013

Member

What does the second call to tight_layout do?

@pwuertz

pwuertz May 2, 2013

Contributor

It's a copy/paste leftover from test_tight_layout1. I'll remove it at once.

@pelson

pelson May 2, 2013

Member

Phew - I thought I'd miss understood the api. Thanks @pwuertz.

pelson merged commit 3ed1397 into matplotlib:master May 13, 2013

1 check passed

default The Travis build passed
Details
Member

pelson commented May 13, 2013

@pwuertz - would you mind adding a PR which adds a "what's new" entry?

pwuertz deleted the pwuertz:tight-layout-parameters branch May 13, 2013

Contributor

pwuertz commented May 22, 2013

@pelson I think I'd like to delay that until this behaviour can be controlled via RC parameters.

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