-
-
Notifications
You must be signed in to change notification settings - Fork 8k
FIX: CL avoid fully collapsed axes #11627
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
FIX: CL avoid fully collapsed axes #11627
Conversation
This is minor, and a safety catch, so marking RC for 3.0....
Would it make sense, and be reasonably easy, to disable the bbox and constraint calculations when zoom-to-rect or zoom/pan are on?
Yeah if there is a method to detect that’s what the redraw is for, I think that’d be the safest thing most of the time. Is it easy to know that the redraw was triggered for that reason?
EDIT: done - easier than I though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move this whole check to a local function layout_is_valid(fig) and then just:
if layout_is_valid(fig):
# Now set the position of the axes...
...
or even with a second function apply_layout(fig)
if layout_is_valid(fig):
apply_layout(fig):
else:
warnings.warn(...)
I think this would help keeping an overview what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a taste thing. 6 lines is kind of under my "move to its own function" threshold. When debugging I find it pretty annoying to have to chase down chains of three-line rabbit holes that do something trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are height that this is a matter of taste
My rule of thumb is: A couple of lines that have a distinguished purpose usually make up a good function. Even more so, if I need a comment to explain what they are doing. The function name can often replace the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well thats fair 😉...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... done - agree that is a bit nicer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 👍
7034418 to
362c34c
Compare
362c34c to
6d8c9c1
Compare
Uh oh!
There was an error while loading. Please reload this page.
PR Summary
Applying zoom to an axes with constrained_layout caused problems because the bounding box for a line on the axes was not correctly calculated when constrained_layout needed it to be. That could lead to a collapsed axes, and singular matrices for other transformations.
This PR doesn't fix the underlying problem with the zoom not setting the transform quick enough, but since layout probably doesn't need to change much under zoom, that's likely OK. However, it does stop constrained layout from being applied if any of the axes have zero width or height.
Note this was not a problem before #10682, because now
ax.get_tightbboxchecks all the artists in the axes.The only drawback I can see is if someone deliberately had a zero-width/height axes on their figure. But I don't think constrained_layout would work well in that case anyways.
UPDATE: Per @efiring suggestion, I've also turned constrained_layout off if 'PAN' or 'ZOOM' are active. I think this is much safer.
PR Checklist