Allow automatic use of tight_layout. #774

Merged
merged 2 commits into from Aug 21, 2012

Conversation

Projects
None yet
6 participants
@efiring
Member

efiring commented Mar 18, 2012

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.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Mar 18, 2012

Member

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.

Member

efiring commented Mar 18, 2012

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.

@leejjoon

This comment has been minimized.

Show comment Hide comment
@leejjoon

leejjoon Mar 19, 2012

Contributor

One thing we may do is to prevent tight_layout if the figure contains any Axes that are not instances of Subplot.

Contributor

leejjoon commented Mar 19, 2012

One thing we may do is to prevent tight_layout if the figure contains any Axes that are not instances of Subplot.

@jdh2358

View changes

lib/matplotlib/figure.py
+ tight = rcParams['figure.autolayout']
+ tight = bool(tight)
+ self._tight = tight
+ tight = property(_get_tight, _set_tight,

This comment has been minimized.

Show comment Hide comment
@jdh2358

jdh2358 Mar 22, 2012

Collaborator

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.

@jdh2358

jdh2358 Mar 22, 2012

Collaborator

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.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Jul 22, 2012

Member

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?

Member

efiring commented Jul 22, 2012

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?

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Aug 12, 2012

Member

Here is another PR that might be just about ready for inclusion before the freeze.

Member

WeatherGod commented Aug 12, 2012

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:

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Aug 12, 2012

Member

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.

@WeatherGod

WeatherGod Aug 12, 2012

Member

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.

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 12, 2012

Member

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.

@efiring

efiring Aug 12, 2012

Member

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.

@WeatherGod

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Aug 12, 2012

Member

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.

Member

WeatherGod commented Aug 12, 2012

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.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 12, 2012

Member

@pelson: are you going to be able to support use_gridspec in your improved colorbar positioning scheme, #956?

Member

efiring commented Aug 12, 2012

@pelson: are you going to be able to support use_gridspec in your improved colorbar positioning scheme, #956?

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 16, 2012

Contributor

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 draw when it is clear which renderer is actually used.

If autolayout is enabled/disabled via rc parameters, shouldn't the padding values go into rc and Figure as well?

Contributor

pwuertz commented Aug 16, 2012

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 draw when it is clear which renderer is actually used.

If autolayout is enabled/disabled via rc parameters, shouldn't the padding values go into rc and Figure as well?

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 17, 2012

Member

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

Member

efiring commented Aug 17, 2012

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

efiring added some commits Mar 18, 2012

Allow automatic use of tight_layout.
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().
figure: improved auto tight_layout support
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.
@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 17, 2012

Member

Rebased against master to take into account the refactoring from commit 5154615.

Member

efiring commented Aug 17, 2012

Rebased against master to take into account the refactoring from commit 5154615.

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 17, 2012

Contributor

@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 ^^

Contributor

pwuertz commented Aug 17, 2012

@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 ^^

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 19, 2012

Member

I see no reason not to merge this issue. +1 from me.

Member

pelson commented Aug 19, 2012

I see no reason not to merge this issue. +1 from me.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 19, 2012

Member

In spite of being the author, I am a bit worried about it for two reasons:

  1. To be half-way reasonable, it really requires the change to colorbar defaults that I have included, use_gridspec=True. This conflicts with @pelson's #956, which makes more fundamental colorbar improvements, but does not support gridspec.

  2. tight_layout is a stopgap in the absence of a real layout manager. If there is a realistic prospect of getting such a manager in, say, a 6-month time frame, then it might be better not to make tight_layout more prominent.

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.

Member

efiring commented Aug 19, 2012

In spite of being the author, I am a bit worried about it for two reasons:

  1. To be half-way reasonable, it really requires the change to colorbar defaults that I have included, use_gridspec=True. This conflicts with @pelson's #956, which makes more fundamental colorbar improvements, but does not support gridspec.

  2. tight_layout is a stopgap in the absence of a real layout manager. If there is a realistic prospect of getting such a manager in, say, a 6-month time frame, then it might be better not to make tight_layout more prominent.

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.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 20, 2012

Member

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?

Member

pelson commented Aug 20, 2012

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?

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Aug 20, 2012

Member

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.

Member

efiring commented Aug 20, 2012

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.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 21, 2012

Member

Agreed. Merging.

Member

pelson commented Aug 21, 2012

Agreed. Merging.

pelson added a commit that referenced this pull request Aug 21, 2012

Merge pull request #774 from efiring/figure_tight_kwarg
Allow automatic use of tight_layout.

@pelson pelson merged commit 62fe17f into matplotlib:master Aug 21, 2012

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 22, 2012

Contributor

Please consider making auto-tight-layout configurable
#1136

Contributor

pwuertz commented Aug 22, 2012

Please consider making auto-tight-layout configurable
#1136

@efiring efiring deleted the efiring:figure_tight_kwarg branch Jun 23, 2016

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