Skip to content

fix tight_layout bug #11737 #11739

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

Closed
wants to merge 1 commit into from

Conversation

fredrik-1
Copy link
Contributor

I believe that the issue in #11737 where some Spines that had a bounding box outside of the figure. This pr seems to solve the bug I found but I don't know enough about the details about the code to know if it is a good solution or any real solution at all.

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member

jklymak commented Jul 22, 2018

I'm not sure whats going on, but ignoring the spines in the tight_layout is probably not a good solution. I'll look into it later.

@fredrik-1
Copy link
Contributor Author

I wounder if it is a similar issue as #10989?

The plot actually looks ok again if tight_layout() is called one extra time.

Ignoring the spines are probably not a good idea but the tightbboxes for the spines are not correct when calculated in axes.get_tightbbox but are correct when calculated from ax from the python prompt.

@jklymak
Copy link
Member

jklymak commented Jul 22, 2018

Sorry on a phone right now but I am pretty certain I addressed the spine issue in a recent PR. But maybe just for constrained layout and not tight layout? Look at my recent closed PRs.

@jklymak
Copy link
Member

jklymak commented Jul 23, 2018

OK, see #11627. This is the same problem, caused by putting all child artists (including the spines) in the tight bbox calculation (#10682).

The problem with spines under zoom is that they don't get their transform updated in time for the tight_bbox calculation. That is the fundamental problem.

The issue with your fix is that if the spine is displaced from the axes position it will not be included in the tight_bbox and the subplot spacing won't be adequately large enough.

So, this bug all makes sense. I'm not sure what the fix for the spines not having the correct transform is, but that'd be the right thing to do.

The other thing we did for constrained_layout is not have it adjust the axes sizes if zoom or pan were on. Thats actually something that would make sense to me for fig.set_tight_layout(True) We don't want to keep changing the axes size while zooming is happening. But, others may disagree.

if bbox is not None and (bbox.width != 0 or bbox.height != 0):
bb.append(bbox)
if not isinstance(a, mspines.Spine):
bbox = a.get_tightbbox(renderer)
Copy link
Member

Choose a reason for hiding this comment

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

Blocking on this as I don't think this is a good solution.

@fredrik-1
Copy link
Contributor Author

I updated my fix to just be used when tight_layout=True.

This is mostly a hack but the behavior is probably similar to the behavior in 2.2.2 so it shouldn't break peoples working process. I rather want this behavior than the behavior in #11742

@fredrik-1 fredrik-1 closed this Jul 23, 2018
@fredrik-1 fredrik-1 reopened this Jul 23, 2018
@fredrik-1
Copy link
Contributor Author

I changed the order in figure.draw such that tight layout is calculated after the draw of the other artists.

The behavior is quite good and what I would expect in most cases but the tight_layout calculation is sometimes still slightly incorrect.

@jklymak
Copy link
Member

jklymak commented Jul 23, 2018

This won't position the axes properly until the second draw...

@fredrik-1
Copy link
Contributor Author

I see now that something doesn't work. It seems to work ok with one subplot but not with several.

But the thing I am after is that I think that tight_layout should be applied after the first draw and redraw the figure. But I can't find a way that does that...

@jklymak
Copy link
Member

jklymak commented Jul 23, 2018

OK, #11754 fixes the root of the problem, I hope.

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 23, 2018
@jklymak jklymak added this to the v3.0 milestone Jul 23, 2018
@fredrik-1 fredrik-1 closed this Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants