Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tight_layout: fix regression for figures with non SubplotBase Axes #1163

Merged
merged 1 commit into from Aug 29, 2012
Merged

tight_layout: fix regression for figures with non SubplotBase Axes #1163

merged 1 commit into from Aug 29, 2012

Conversation

pwuertz
Copy link
Contributor

@pwuertz pwuertz commented Aug 28, 2012

When encountering axes instances that do not descent from SubplotBase, tight_layout tries to print a warning, but there is a typo in that code block causing an exception and thus stopping the whole process.

Also, in my thesis I have a figure that uses an inset (mpl_toolkits.axes_grid1.inset_locator.inset_axes). I usually remove the figure borders using tight_layout(0.0). This worked fine in matplotlib 1.1.1, but the current master just bails out from tight_layout when encountering the inset axes. I think showing the warning to the user is enough, the result might turn out to be just fine...

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 28, 2012

The commit that introduced this change is ea0630a
@efiring I'm letting tight_layout process the SubplotBase instances only + warn the user that some axis might be off.. you think this is fine?

" all Axes must descend from SubplotBase")
return
warnings.warn("tight_layout can only process Axes that descend "
"from SubplotBase, results might be incorrect")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indent the continuation line(s), preferably by lining up the LH quote marks.
I would change the comma in the second line to a semicolon, and end with a period.

Rather than cycle through self.axes twice, why not move your axes calculation below up to above
the "no_go" line, then change that line to "dangerous = len(axes) < len(self.axes)" or something like that.

Maybe also change "axes" to "subplot_axes", to make it clear we are not operating on all axes.

@efiring
Copy link
Member

efiring commented Aug 29, 2012

Apart from the inline note above, I think your idea is fine. The no_go approach was overkill.

@pwuertz
Copy link
Contributor Author

pwuertz commented Aug 29, 2012

Fixed up the commit according to your suggestions.

efiring added a commit that referenced this pull request Aug 29, 2012
tight_layout: fix regression for figures with non SubplotBase Axes
@efiring efiring merged commit 0c7cdaa into matplotlib:master Aug 29, 2012
return
subplot_axes = [ax for ax in self.axes if isinstance(ax, SubplotBase)]
if len(subplot_axes) < len(self.axes):
warnings.warn("tight_layout can only process Axes that descend "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word "descend" here is a bit confusing. I think the more commonly used term in OO is "inherit". If there's no objections, since this is already merged, I'll just go ahead and fix this directly on master.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 2012/08/29 3:48 AM, Michael Droettboom wrote:

In lib/matplotlib/figure.py:

@@ -1424,16 +1424,15 @@ def tight_layout(self, renderer=None, pad=1.08, h_pad=None, w_pad=None, rect=Non

     from tight_layout import get_renderer, get_tight_layout_figure
  •    no_go = [ax for ax in self.axes if not isinstance(ax, SubplotBase)]
    
  •    if no_go:
    
  •        warnings.Warn("Cannot use tight_layout;"
    
  •                      " all Axes must descend from SubplotBase")
    
  •        return
    
  •    subplot_axes = [ax for ax in self.axes if isinstance(ax, SubplotBase)]
    
  •    if len(subplot_axes) < len(self.axes):
    
  •        warnings.warn("tight_layout can only process Axes that descend "
    

I think the word "descend" here is a bit confusing. I think the more
commonly used term in OO is "inherit". If there's no objections, since
this is already merged, I'll just go ahead and fix this directly on master.

I agree; I noticed the same thing, but didn't think to correct it. Go
ahead if you have not already done so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants