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

Don’t use tight layout as it causes bouncing around of axes #745

Merged
merged 11 commits into from
Sep 13, 2015

Conversation

astrofrog
Copy link
Member

I looked into the issue described in #730, namely that the axes appear to bounce around when being resized. This is due to the tight layout option, which causes the axes to be re-drawn after every draw with a different bounding box.

This PR disables this, and tries to instead optimize space used by setting the rect for the axes created. Of course, this isn't perfect either because the axes size is relative to the window so for larger axes there is a lot of wasted space. I am going to look into whether there is a better way to do that, but in the mean time, this does produce smoother axes resizing without any bouncing.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - it would be great if you could try this out when you get a chance. I've now added a class factory that ensures that the margin between axes and the edge of the figure is constant in absolute terms. This allows us to minimize whitespace while avoiding the issues due to the tight layout option.

At the moment, this isn't perfect for the AxesCache but we can probably get it to work.

@astrofrog
Copy link
Member Author

(I will fix the test failures if we go with this approach)

@astrofrog
Copy link
Member Author

@ChrisBeaumont - I'm pretty happy with this approach now which works even for the AxesCache. Could you review this and let me know what you think?

@astrofrog
Copy link
Member Author

Fixes #730

@ChrisBeaumont
Copy link
Member

Will take a look tmrw morning

On Thursday, September 10, 2015, Thomas Robitaille notifications@github.com
wrote:

Fixes #730 #730


Reply to this email directly or view it on GitHub
#745 (comment).

@ChrisBeaumont
Copy link
Member

Cool, this seems to work well for me. Do you know how this works if a plot has multiple axes on it? I'm trying to think and I don't think there are any standard viewers that do so, so It's probably not a blocker either way.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - thanks for reviewing! None of the viewers I am using this for have multiple axes. If a user defined axes in a custom viewer I am not applying this method to those to be safe, but I could document how to use the AxesResizer since it's pretty easy (alternatively, if the custom viewer can be used only with single axes, I can attach the AxesResizer to those too.

@ChrisBeaumont
Copy link
Member

IIRC it's not easy to build a custom viewer with multiple axes (the axes are auto-built and handed off to the user). It might be nice to:

  • Attach AxesResizers to the axes in custom viewers
  • Document how to modify the parameters of those resizers, in case users want to modify the margins of their custom plots

@astrofrog
Copy link
Member Author

@ChrisBeaumont - actually at the moment custom_viewer.create_axes is not a user-definable function unless their viewer inherits from custom_viewer and overrides it. We don't mention this route in the docs - so do we want to make create_axes a user-definable function?

If so, then we can also make it that to set the axes margins, the user/developer can specify custom_viewer.axes_margins to be a list of four values, which will then get used for the margins. What do you think?

@astrofrog
Copy link
Member Author

Sorry, just realized you said the same thing about the fact the user doesn't create the axes. I will implement the axes_margins parameter so the user can set them.

``[left, right, bottom, top]``
"""

resizer = AxesResizer(axes, margins)
Copy link
Member

Choose a reason for hiding this comment

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

it might be nice to store this resizer as an attribute on the axes somewhere, in case downstream code wishes to modify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that might be a way to customize the margins then, if it's accessible as axes.resizer.margins. I worry a bit about adding axes.resizer though in case it conflicts with Matplotlib in future for any reason. Do you think we should just do it anyway and cross that bridge if it happens?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should worry about future name clashes with resizer

On Friday, September 11, 2015, Thomas Robitaille notifications@github.com
wrote:

In glue/utils/matplotlib.py
#745 (comment):

+def fix_margins(axes, margins=[1, 1, 1, 1]):

  • """
  • Make sure margins of axes stay fixed.
  • Parameters

  • ax_class : matplotlib.axes.Axes
  •    The axes class for which to fix the margins
    
  • margins : iterable
  •    The margins, in inches. The order of the margins is
    
  •    `[left, right, bottom, top]`
    
  • """
  • resizer = AxesResizer(axes, margins)

Hmm, that might be a way to customize the margins then, if it's accessible
as axes.resizer.margins. I worry a bit about adding axes.resizer though
in case it conflicts with Matplotlib in future for any reason. Do you think
we should just do it anyway and cross that bridge if it happens?


Reply to this email directly or view it on GitHub
https://github.com/glue-viz/glue/pull/745/files#r39279062.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@astrofrog
Copy link
Member Author

@ChrisBeaumont - I implemented your suggestions, thanks! I'll keep this open for another day in case you want to review again.

astrofrog added a commit that referenced this pull request Sep 13, 2015
Don’t use tight layout as it causes bouncing around of axes
@astrofrog astrofrog merged commit e86d412 into glue-viz:master Sep 13, 2015
@ChrisBeaumont
Copy link
Member

I was thinking about this a little more. I guess the main advantage of tight_layout is that it auto-chooses sensible initial margins for an arbitrary plot (and potentially complicated ones with multiple axes, weird axis labels, etc). Its drawback (fixed in this PR) is that it auto-updates these margins, leading to jumpy panning/zooming. So maybe the ideal solution is one that uses tight_layout's internal mechanism to determine good initial margins, and then passes these values to freeze_margins. That's not a high priority, but something we could consider if we start to notice that the current hard-coded frozen margins don't look good in certain situations.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - yeah, I was thinking of that too. That might be possible if we can avoid an initial draw before tight_layout figures out the best margins (otherwise we'll get jumpy behavior upon initialization)

@ChrisBeaumont
Copy link
Member

Do you actually notice jumpiness at init time? I mostly see this during pan or zoom, when a new tick label is created which changes the margins. I haven't noticed any jumpiness when you first open a window

@astrofrog
Copy link
Member Author

Ah, I will double check

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

Successfully merging this pull request may close these issues.

2 participants