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

Updated custom_projection_example.py to work with v1.2 and newer #1579

Merged
merged 2 commits into from Dec 11, 2012

Conversation

joferkington
Copy link
Contributor

custom_projection_example.py is currently broken with matplotlib v1.2.x.

Compare the custom_projection_example.py run with matplotlib 1.1 Example with 1.1

with the custom_projection_example.py run with matplotlib 1.2 Example with 1.2

@joferkington
Copy link
Contributor Author

I should have added this information here instead of on the mailing list. At any rate, here's the rationale for the changes, beyond simply fixing the example:

At some point transforms.Transform was slightly refactored. (Particularly,
this commit:
8bbe2e5 )

It's certainly makes things more logical and consistent, but it changed what methods need to be overridden when subclassing
Transform.

It looks like Phil caught the changes for the polar and geo projections, but the custom projection example was never updated. This led to the example failing for newer (1.2.0 and master) versions.

However, it's worth elaborating on the changes to the Transform class in the comments in the example.

With versions 1.1.x and older, one had to directly implement a
transform method when subclassing transforms.Transform,
otherwise a NotImplemented error would be raised.

With versions 1.2.x and newer, the preferred way appears to be to implement
things in a separate transform_affine or transform_non_affine method and
not explicitly implement a transform method.

If you implement the non-affine portion directly in the transform method
without overriding transform_non_affine, it leads to strange drawing bugs
with v1.2 that did not occur with older versions. (And thus the example being broken.)

On the other hand, for compatibility with versions 1.1 and older, you have
to explicitly implement the transform method as well, otherwise you'll get
the NotImplemented error.

Therefore, now one needs to explicitly implement both the
transform_non_affine and transform methods of a custom non-affine transform
for compatibility with 1.1 and older as well as 1.2 and newer.

Similarly, one needs to implement both the transform_path_non_affine and the
transform_path methods for compatibility with newer and older versions of
matplotlib.

# Once again, for compatibility with matplotlib v1.1 and older, we need
# to explicitly override ``transform_path``. With v1.2 and newer, only
# overriding the ``transform_path_non_affine`` method is sufficient.
transform_path = transform_path_non_affine
Copy link
Member

Choose a reason for hiding this comment

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

We could do a version specific override here. Something like:

if matplotlib.__version__ < '1.2':
    transform_path = ...
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Also a version-specific conditional would potentially be more future-proof, as well as "stretching the legs" of the new functionality. I'll add that here directly.

@pelson
Copy link
Member

pelson commented Dec 10, 2012

Thanks for doing this - much appreciated! I have commented that we might consider applying the compatibility code only if the version requires it, that way we are stretching the legs of the new functionality for newer versions, whereas we can still run the code for older. What do you think?

@pelson
Copy link
Member

pelson commented Dec 10, 2012

Other than that 👍

@pelson
Copy link
Member

pelson commented Dec 10, 2012

P.S. Once we are happy with this, we will want to apply it to the v1.2.x branch.

@mdboom
Copy link
Member

mdboom commented Dec 10, 2012

Thanks for catching this. +1 (and I agree with all of @pelson's comments).

@joferkington
Copy link
Contributor Author

I agree, the version conditional makes the intent more clear, in case someone doesn't read the comments closely. Also, it's arguably more future-proof than re-implementing the default transform method. Done in joferkington@2a73c12. Thanks!

pelson added a commit that referenced this pull request Dec 11, 2012
Updated custom_projection_example.py to work with v1.2 and newer
@pelson pelson merged commit c13f629 into matplotlib:master Dec 11, 2012
@pelson
Copy link
Member

pelson commented Dec 11, 2012

Thanks @joferkington - keep up the great work!

@joferkington
Copy link
Contributor Author

Thanks!

pelson added a commit that referenced this pull request Dec 11, 2012
Updated custom_projection_example.py to work with v1.2 and newer
@mdboom
Copy link
Member

mdboom commented Dec 11, 2012

Cherry-picked to v1.2.x and will update the online docs shortly.

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

Successfully merging this pull request may close these issues.

None yet

3 participants