Allow automatic use of tight_layout. #774
Conversation
|
This is definitely not ready for use as-is; I see that it does something half-way reasonable with a colorbar only if the colorbar has been created with use_gridspec=True. |
|
One thing we may do is to prevent tight_layout if the figure contains any Axes that are not instances of Subplot. |
| tight = rcParams['figure.autolayout'] | ||
| tight = bool(tight) | ||
| self._tight = tight | ||
| tight = property(_get_tight, _set_tight, |
There was a problem hiding this comment.
I understand a foolish consistency is the hobgoblin of little minds, but shouldn't we stick with the normal setters and getters rather than introducing properties, so this will be exposed to the whole artist set/get introspection? It seems like it would add confusion to have some properties sets as plain-ol-attributes-or-properties, and some as setters/getters.
|
This is now ready for reconsideration; I followed the request of @jdh2358 , and the suggestion of @leejjoon . I also made the colorbar default be use_gridspec=True. The remaining question include (1) should we have something like this at all? and (2) if so, instead of recycling the existing rcParam name figure.autolayout, should it be figure.tight_layout to match the function and kwarg? Or are there any other changes needed? |
|
Here is another PR that might be just about ready for inclusion before the freeze. |
| @@ -835,6 +860,13 @@ def draw(self, renderer): | |||
| if not self.get_visible(): return | |||
| renderer.open_group('figure') | |||
|
|
|||
| if self.get_tight_layout() and self.axes: | |||
There was a problem hiding this comment.
What exactly is being tested wrt self.axes? Being None? Being of length greater than zero. I know PEP8 encourages this sort of semantics, but it is a pain in the butt to me to understand the intention.
There was a problem hiding this comment.
self.axes is used all over the place in Figure; it is a read-only list of axes. It is being tested for emptiness. I could enclose it in len(), but it would not add anything.
|
I think I am fine with autolayout being recycled for this. Since this does change a couple of defaults though (specifically, use_gridspec in the colorbar() method is changed from false to true), this is going to need to be noted somewhere. pyplot.py will need to be regenerated, and an entry added to users/whats_new.rst for this feature. |
|
I like it! Embedding the layout functionality in the figure so it is called on demand is actually the right thing to do. There is a usability glitch in the current design. When calling tight_layout, it grabs a renderer from the active backend for determining the font metrics. Most probably this is the renderer from an interactive backend like GtkAgg or QtAgg. If the user now chooses to save the figure to a file the font metrics might change. The layout should be based on the renderer of the target canvas only, which means that tight_layout must be called within If autolayout is enabled/disabled via rc parameters, shouldn't the padding values go into rc and Figure as well? |
|
@pwuertz, I don't understand what you are suggesting in the middle paragraph of your comment above. tight_layout is called in the Figure.draw() method with whatever renderer is in use at the time. When savefig is called, tight_layout gets called with the non-interactive renderer being used. Is your point that what you see on the screen might not match what is printed? That is true even without tight_layout. Or is your question about manually calling the Figure.tight_layout() method without supplying a renderer? In other words, are suggesting a change to this pull request, apart from adding padding values to rc? |
In common interactive usage, stretching a figure across the screen leaves too much wasted space; Figure.tight_layout() can solve this problem at least for simple plots. This changeset uses a previously existing but unused rcParam and a Figure kwarg to allow the user to have tight_layout executed automatically with every Figure.draw().
1) Use getter and setter instead of property 2) in tight_layout, don't proceed if an Axes is not a SubplotBase 3) in colorbar, make use_gridspec default to True.
|
Rebased against master to take into account the refactoring from commit 5154615. |
|
@efiring Ah sorry, with "current design" I meant the way we have been using tight_layout up to this point. The way you embedded it into draw() is perfect because now it will always use the right renderer. So what I was trying to say is that this doesn't just add more convenience, this is actually the right way of using tight layout ^^ |
|
I see no reason not to merge this issue. +1 from me. |
|
In spite of being the author, I am a bit worried about it for two reasons:
A counterpoint to point 2 is that the use of the autolayout rc param could simply be switched to apply to the geometry manager if/when it becomes available, so code using it would still work reasonably. |
|
I can't imagine a geometry manager implementation that doesn't break a whole host of things and my feeling is that it would be a version 2.0 magnitude change. Many people are aware of tight layout, so I see no reason to not making it configurable via the rc params. I do take your point about changing the default behaviour though. Is there a reason why it is not enough to simply be able to enable the feature, rather than make it the default? |
|
There may be some confusion here. The pull request does not make tight_layout the default, it makes it possible for one to make it default via an rcParam. The only change in default behavior is that colorbar defaults to use_gridspec=True. The reason for this is that if tight_layout is not used, using gridspec for the colorbar makes no difference; but if tight_layout is used, and a colorbar is included, and that colorbar does not use gridspec, the result is horrible--always. I don't see any point in making it easy to enable tight_layout by default if one then has to remember to override the obscure and recent use_gridspec kwarg for colorbar. As far as I can see, the only problem with the change in colorbar default to use_gridspec=True is that it conflicts with your #956 in its present form. One option would be to merge this tight_layout changeset now, and wait on #956 until it supports gridspec. I don't know how hard adding that support will be; so far, I have avoided looking closely at gridspec and axesgrid. My guess is that it will not actually be very difficult--but then that is easy for me to say, as I am not volunteering to do it. |
|
Agreed. Merging. |
Allow automatic use of tight_layout.
|
Please consider making auto-tight-layout configurable |
In common interactive usage, stretching a figure across the screen
leaves too much wasted space; Figure.tight_layout() can solve this
problem at least for simple plots. This changeset uses a previously
existing but unused rcParam and a Figure kwarg to allow the user
to have tight_layout executed automatically with every Figure.draw().
There may be all sorts of gotcha's with this, but at least for much routine work I think something along these lines will be a real help. The lack of fit between the contents of a figure and the figure boundary has been a long-time complaint of many users. Being able to manually fix it figure by figure by calling tight_layout is better than nothing, but I am hoping that tight_layout is now good enough to warrant letting it be used automatically when desired.
I recycled an inactive rcParams entry left over from 2008, figure.autolayout, but used the shorter "tight" as the name of the Figure kwarg and property. It would probably make sense to change the rcParams entry to match the kwarg name.