Configuring automatic use of tight_layout #1136
Conversation
|
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. |
|
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. |
|
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.. |
|
I guess this is Michael's call, @mdboom, what do you think? |
|
I haven't looked very closely, but my reactions are 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. |
|
@efiring |
|
@efiring: Removed the parameters from Figure.init. The automatic use of tight_layout can be configured using the figure's properties. |
|
@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? |
|
@efiring Ok I think I got it: How about adding these as optional parameters to your set_tight_layout method? |
|
Another proposal that allows you to provide a dictionary for set_tight_layout. No more additional getters/setters. |
|
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 AFAICT this bug is related to the current issue reported by pwuertz. If not, I can open a separate issue for it. |
|
@mdehoon Please open a separate issue. |
| tight = bool(tight) | ||
| self._tight = tight | ||
| self._tight = bool(tight) | ||
| self._tight_parameters = tight if type(tight) is dict else {} |
|
This has my 👍 once it has a test. @efiring - how do you stand on this PR? |
|
@pelson, the implementation looks fine to me, now. 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. |
|
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 In the new |
|
This looks like a baseline vs bottom positioning error in the agg backend. Any chance that this is related to #1810 ? |
|
I'll update the png baseline image once #1968 is resolved. Please don't merge until then. |
|
Good to go if you are ok with the test. |
| fig.set_tight_layout({'pad': .1}) | ||
| ax = fig.add_subplot(111) | ||
| example_plot(ax, fontsize=24) | ||
| plt.tight_layout() |
There was a problem hiding this comment.
What does the second call to tight_layout do?
There was a problem hiding this comment.
It's a copy/paste leftover from test_tight_layout1. I'll remove it at once.
There was a problem hiding this comment.
Phew - I thought I'd miss understood the api. Thanks @pwuertz.
Configuring automatic use of tight_layout
|
@pwuertz - would you mind adding a PR which adds a "what's new" entry? |
|
@pelson I think I'd like to delay that until this behaviour can be controlled via RC parameters. |
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_layoutkwarg infigure.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.