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

TST/FIX twinx and twiny w/ constrainedlayout #10407

Merged
merged 1 commit into from
Feb 10, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 9, 2018

PR Summary

As per #10404, the twinx/y code in _base.py for constrainedlayout was not being called in any of the tests, and hence an import was inadvertently erased. This adds the import and two tests. I also tested interactively, and everything works as it should to window resizing.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 9, 2018
@jklymak jklymak added this to the v2.2 milestone Feb 9, 2018
@QuLogic
Copy link
Member

QuLogic commented Feb 9, 2018

It's too bad this was not noticed before, but you should probably use style='default' for any new test images.

@jklymak
Copy link
Member Author

jklymak commented Feb 9, 2018

Do you mean style='mpl20'? Confused why I'd set it to the default (presumably not specifying is the default).

@QuLogic
Copy link
Member

QuLogic commented Feb 9, 2018

Not specified defaults to classic, or we'd have to recreate all the test images. mpl20 is also good, and maybe better to be more explicit.

@jklymak
Copy link
Member Author

jklymak commented Feb 9, 2018

I can do that for the two that aren't merged here. Happy to do it for the other ones as well, but there will still be dupes in the repo.

@jklymak
Copy link
Member Author

jklymak commented Feb 9, 2018

Actually, removed the images. Not really needed for the test, which is just that the axes track each other....

Happy to change the other (already committed) image tests if thats warranted.

ax2 = ax.twinx()
example_plot(ax)
example_plot(ax2, fontsize=24)
fig.draw(fig.canvas.get_renderer())
Copy link
Member

Choose a reason for hiding this comment

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

fig.canvas.draw() is what's used elsewhere.

@QuLogic
Copy link
Member

QuLogic commented Feb 9, 2018

The images are already there, so I guess there's no saving in changing the style now. Just best to remember it the next time they're updated.

@QuLogic
Copy link
Member

QuLogic commented Feb 10, 2018

codecov says the line with the undefined name is not getting hit with these new tests.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

I can run the tests without the import and it still passes, so it seems they are not checking that code just yet.

@jklymak
Copy link
Member Author

jklymak commented Feb 10, 2018

Great catch @QuLogic Thanks a lot. That code path was never getting called, because only the twinx logic in _subplots is used. Guess thats why I never noticed there was no import 🐑

What really has me flummoxed is that twinx works w/o that logic. I added to subplot anyway, because I think its important to keep a layoutbox with each axis. But it never got into the situation where that was causing a problem.

@jklymak
Copy link
Member Author

jklymak commented Feb 10, 2018

Small fix - the layout box in the twinned axis was already being made, so this just constrains it to be the same size as its twin. This was already happening due to logic @efiring added, but this makes it explicit in the layout constraints, which seems a good idea if things get complicated.

@jklymak jklymak added the topic: geometry manager LayoutEngine, Constrained layout, Tight layout label Feb 10, 2018
@QuLogic QuLogic merged commit ca0195c into matplotlib:master Feb 10, 2018
@jklymak
Copy link
Member Author

jklymak commented Feb 10, 2018

Thanks again for your help @QuLogic I'd missed that that code wasn't even being called.

@jklymak jklymak deleted the tst-constrainedlayout-twin branch February 10, 2018 23:04
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 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. topic: geometry manager LayoutEngine, Constrained layout, Tight layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants