Skip to content

FIX: secondary_axis resize #13419

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

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 12, 2019

PR Summary

As pointed out by @ImportanceOfBeingErnest the new secondary_axis (#11859) didn't survive a resize. This new version does.

Is there a good way to test resizing a canvas?

closes: #13417

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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 jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 12, 2019
@jklymak jklymak added this to the v3.1.0 milestone Feb 12, 2019
def secondary_locator(ax, renderer):
bb = mtransforms.TransformedBbox(_rect, parent.transAxes)
Copy link
Member Author

Choose a reason for hiding this comment

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

@anntzer I knew there was a reason for this song and dance...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment mentioning this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Comment added.

@jklymak jklymak force-pushed the fix-secondary-axis-resize branch from 58b40fb to 5cd9676 Compare February 12, 2019 03:26
@ImportanceOfBeingErnest
Copy link
Member

Concerning a test, the ugly result would be reproduced when calling fig.set_size_inches((7,4)) after creating the secondary axes and before drawing/saving it.

@jklymak
Copy link
Member Author

jklymak commented Feb 12, 2019

Added test - fails on master, passes w/ this PR

@jklymak jklymak force-pushed the fix-secondary-axis-resize branch from 5114f7d to f96eba8 Compare February 12, 2019 06:11
@anntzer
Copy link
Contributor

anntzer commented Feb 12, 2019

Good catch.
I think the standard-ish way to avoid the fact that inverted() is "eager" would be

    bb = mtransforms.TransformedBbox(
        mtransforms.Bbox.from_bounds(*rect),
        mtransforms.BboxTransformFrom(parent.bbox)
        + mtransforms.BboxTransformTo(parent.figure.bbox))

    def secondary_locator(ax, renderer):
        return bb

    return secondary_locator

which also works, and avoids constructing new transform nodes at each call (#12031).
Or alternatively, use Transform.transform_bbox which constructs "dumb" bboxes that don't keep track of their transform stack.

@jklymak
Copy link
Member Author

jklymak commented Feb 12, 2019

That makes there be no secondary axis rendered at all (or I guess its located off page?)

I'm pretty sure parent_bbox has to be the draw-time bbox, not the locator instantiation bbox.

@anntzer
Copy link
Contributor

anntzer commented Feb 12, 2019

Indeed, sorry.
I think such an approach could be made to work, but in the meantime this looks like a good fix.

@dstansby dstansby merged commit daaf978 into matplotlib:master Feb 14, 2019
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.

Secondary axis doesn't resize properly
6 participants