Improve reprs of transforms. #9421

anntzer · Oct 15, 2017

This patch should make debugging the transform a bit stack easier, by
changing, for example, the repr of the transform of a line created with
plot([1, 2]) from

CompositeGenericTransform(TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())), CompositeGenericTransform(BboxTransformFrom(TransformedBbox(Bbox([[-0.05, 0.95], [1.05, 2.05]]), TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())))), BboxTransformTo(TransformedBbox(Bbox([[0.125, 0.10999999999999999], [0.9, 0.88]]), BboxTransformTo(TransformedBbox(Bbox([[0.0, 0.0], [6.4, 4.8]]), Affine2D(array([[ 100.,    0.,    0.],
    [   0.,  100.,    0.],
    [   0.,    0.,    1.]]))))))))

to

CompositeGenericTransform(
    TransformWrapper(
        BlendedAffine2D(
            IdentityTransform(),
            IdentityTransform())),
    CompositeGenericTransform(
        BboxTransformFrom(
            TransformedBbox(
                Bbox(x0=-0.05, y0=0.95, x1=1.05, y1=2.05),
                TransformWrapper(
                    BlendedAffine2D(
                        IdentityTransform(),
                        IdentityTransform())))),
        BboxTransformTo(
            TransformedBbox(
                Bbox(x0=0.125, y0=0.10999999999999999, x1=0.9, y1=0.88),
                BboxTransformTo(
                    TransformedBbox(
                        Bbox(x0=0.0, y0=0.0, x1=6.4, y1=4.8),
                        Affine2D(
                            [[ 100.    0.    0.]
                             [   0.  100.    0.]
                             [   0.    0.    1.]])))))))

This will, of course, invalidate code that relied on the exact strs, but
what can you do.

PR Summary

PR Checklist

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

QuLogic · Oct 27, 2017

Instead of re-indenting in every parent, you could define __format__ which somehow accepts an indentation level to produce the string with the right indent from the beginning. I'm not sure if this is any better.

anntzer · Oct 27, 2017

I actually had an original implementation that did that, but I think it's better not to do so, because I'd prefer keeping the format string available for (... some day) pass it down to float formatting (à la numpy/numpy#5543), rather than overloading it for what is mostly internal use.

TBH I don't think the performance of reindenting every parent is a real issue...

QuLogic · Oct 28, 2017

I added the same sort of fancy string formats to the transforms in other projections.

anntzer · Oct 28, 2017

lib/matplotlib/projections/geo.py

@@ -268,6 +267,11 @@ def __init__(self, resolution):
            Transform.__init__(self)
            self._resolution = resolution

+        def __str__(self):


Couldn't these be provided by a mixin class? transform_path_non_affine also looks repeated across, haven't checked if other methods are too

Possibly; I wasn't really looking for things to refactor, but I'm sure there's a lot of repetition in there.

I was also wondering if maybe we should def __repr__(self): return str(self) in some parent class instead of __repr__ = __str__ in every subclass?

That's a good point. Want to push it?

anntzer · Oct 28, 2017

lib/matplotlib/projections/polar.py

+    def __str__(self):
+        return ("{}(\n"
+                    "{},\n"
+                    "    use_rmin={},\n"


I have a slightly preference for this string not to be indented, or remove the explicit spaces and call indent_str for use_rmin too (either way). The idea was to make the source layout "look like" the final result. Same comment throughout.

jklymak · Nov 19, 2017

Where is this at? I was just looking at some Transforms, and their __repr__ sure is ugly!

anntzer · Nov 19, 2017

I pushed the whitespace fix, the __repr__ = __str__-to-base-class fix, and the refactoring of common code in GeoTransforms mentioned above.
I think this is good to go (if I didn't break anything just now).

jklymak · Nov 19, 2017

I tested the __repr__ and it is much better, though I'm sure I didn't test everything. All I did was

import numpy as np
import matplotlib.pyplot as plt

fig, axs = plt.subplots()
axs.plot(np.arange(10))
la = axs.set_xlabel('Test')
print(la.get_transform())

and I got:

BlendedAffine2D(
    BboxTransformTo(
        TransformedBbox(
            Bbox(x0=0.125, y0=0.10999999999999999, x1=0.9, y1=0.88),
            BboxTransformTo(
                TransformedBbox(
                    Bbox(x0=0.0, y0=0.0, x1=6.4, y1=4.8),
                    Affine2D(
                        [[ 200.    0.    0.]
                         [   0.  200.    0.]
                         [   0.    0.    1.]]))))),
    IdentityTransform())

Are there enough tests of the geo-transform code to make sure this refactor is OK?

anntzer · Nov 19, 2017

I'd assume yes (and it's really just moving common code to a base class), if not that means more tests are needed anyways...

tacaswell

Can you also add a whats_new for this?

tacaswell · Nov 19, 2017

Is there a straight forward way to at least smoke-test the reprs?

anntzer · Nov 19, 2017

done (the whatsnew)

I think numpy is going to (finally) change their floating point reprs (for the better) numpy/numpy#9941 so unless we construct exactly the right transform to only use integer values we'll (soon) have some trouble getting stable outputs.

jklymak

This is very helpful

jklymak · Nov 19, 2017

code-block needs an argument... We just went through this, but I forget what the solution was!

anntzer · Nov 19, 2017

Hey, I'm supposed to be the rst police :)
fixed

This patch should make debugging the transform a bit stack easier, by changing, for example, the repr of the transform of a line created with `plot([1, 2])` from CompositeGenericTransform(TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())), CompositeGenericTransform(BboxTransformFrom(TransformedBbox(Bbox([[-0.05, 0.95], [1.05, 2.05]]), TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())))), BboxTransformTo(TransformedBbox(Bbox([[0.125, 0.10999999999999999], [0.9, 0.88]]), BboxTransformTo(TransformedBbox(Bbox([[0.0, 0.0], [6.4, 4.8]]), Affine2D(array([[ 100., 0., 0.], [ 0., 100., 0.], [ 0., 0., 1.]])))))))) to CompositeGenericTransform( TransformWrapper( BlendedAffine2D( IdentityTransform(), IdentityTransform())), CompositeGenericTransform( BboxTransformFrom( TransformedBbox( Bbox(x0=-0.05, y0=0.95, x1=1.05, y1=2.05), TransformWrapper( BlendedAffine2D( IdentityTransform(), IdentityTransform())))), BboxTransformTo( TransformedBbox( Bbox(x0=0.125, y0=0.10999999999999999, x1=0.9, y1=0.88), BboxTransformTo( TransformedBbox( Bbox(x0=0.0, y0=0.0, x1=6.4, y1=4.8), Affine2D( [[ 100. 0. 0.] [ 0. 100. 0.] [ 0. 0. 1.]]))))))) This will, of course, invalidate code that relied on the exact strs, but what can you do.

jklymak · Nov 19, 2017

I was just saving you clicking into the test failure - you are still definitely the rst enforcer!

anntzer force-pushed the transforms-repr branch 2 times, most recently from 3603921 to 6960534 Compare October 16, 2017 06:15

anntzer commented Oct 28, 2017

View reviewed changes

tacaswell added this to the v2.2 milestone Oct 29, 2017

tacaswell approved these changes Nov 19, 2017

View reviewed changes

jklymak approved these changes Nov 19, 2017

View reviewed changes

anntzer force-pushed the transforms-repr branch from aad58ce to 710ed64 Compare November 19, 2017 18:30

anntzer and others added 7 commits November 19, 2017 10:33

Add fancy repr to Polar transforms.

862909c

Add fancy repr to geographic transforms.

66c46c7

Whitespace fixes.

fa69001

Move Transform.__repr__ = __str__ to base class.

a8721cd

Factor out common _GeoTransform code.

cbb72b1

Add whatsnew entry.

dbefd27

anntzer force-pushed the transforms-repr branch from 710ed64 to dbefd27 Compare November 19, 2017 18:33

QuLogic merged commit 07d2e94 into matplotlib:master Dec 1, 2017

anntzer deleted the transforms-repr branch December 1, 2017 08:39

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

anntzer mentioned this pull request May 5, 2018

Define __repr__, not __str__ for transforms. #11173

Closed

6 tasks

Search code, repositories, users, issues, pull requests...

Uh oh!

Improve reprs of transforms. #9421

Improve reprs of transforms. #9421

Uh oh!

Conversation

anntzer commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

QuLogic commented Oct 27, 2017

Uh oh!

anntzer commented Oct 27, 2017

Uh oh!

QuLogic commented Oct 28, 2017

Uh oh!

anntzer Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

QuLogic Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

anntzer Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

anntzer Oct 28, 2017

Choose a reason for hiding this comment

Uh oh!

jklymak commented Nov 19, 2017

Uh oh!

anntzer commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Nov 19, 2017

Uh oh!

anntzer commented Nov 19, 2017

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Nov 19, 2017

Uh oh!

anntzer commented Nov 19, 2017

Uh oh!

jklymak commented Nov 19, 2017

Uh oh!

Uh oh!

anntzer commented Oct 15, 2017 •

edited

Loading

anntzer commented Nov 19, 2017 •

edited

Loading

tacaswell commented Nov 19, 2017 •

edited

Loading

anntzer commented Nov 19, 2017 •

edited

Loading