Fix log transforms (fixes #3809) [back port to 1.4.x]

[maxalbert]

No description provided.

[pelson]

Could do with a test. 😉

[pelson]

Does this also fix the documentation?

[maxalbert]

Yes, it should also fix the documentation. In terms of test, are you essentially thinking of running the code snippet in the description of #3809 to ensure that it doesn't raise an exception?

[WeatherGod]

There is also a "bug report" of sorts in the mailing list from about a
month ago that might be fixed by this... I'll have to dig a bit, but it
might serve as a useful test case.

On Sat, Nov 29, 2014 at 12:35 PM, maxalbert notifications@github.com
wrote:

Yes, it should also fix the documentation. In terms of test, are you
essentially thinking of running the code snippet in the description of
#3809 #3809 to ensure
that it doesn't raise an exception?

—
Reply to this email directly or view it on GitHub
#3863 (comment)
.

[WeatherGod]

Found it... could this possibly fix #3540?

On Sat, Nov 29, 2014 at 12:37 PM, Benjamin Root ben.root@ou.edu wrote:

There is also a "bug report" of sorts in the mailing list from about a
month ago that might be fixed by this... I'll have to dig a bit, but it
might serve as a useful test case.

On Sat, Nov 29, 2014 at 12:35 PM, maxalbert notifications@github.com
wrote:

Yes, it should also fix the documentation. In terms of test, are you
essentially thinking of running the code snippet in the description of
#3809 #3809 to ensure
that it doesn't raise an exception?

—
Reply to this email directly or view it on GitHub
#3863 (comment)
.

[tacaswell]

@maxalbert Yes,

@cleanup
def test_log_transform():
    fig, ax = plt.figure()
    ax.set_yscale('log')          # <--- works fine without this line
    ax.transData.transform((1,1)) # <--- exception thrown here

Would be good enough. The @cleanup is important.

[maxalbert]

@tacaswell Thanks! I have updated this PR with the suggested test. I had hoped that with the fix in #3866 it would just work, but it turns out that there were still problems because some (most?) of the transformation code on the Python side assumes that the input values are 2d.

I can see two solutions to this:

(1) Do not support transforming a single point but always require the input to be a list of points. This would at least ensure consistency, but it would be somewhat unintuitive for the user and also require changes in the documentation.

(2) Support transforming a single point. This feels nicer from a user's point of view so I have chosen it for the current PR.

My current solution in order to avoid messing with too much of the existing code is to unconditionally convert the input to a 2d array in Transform.transform(), then apply whatever transform was asked for and finally convert the result back to the original shape. Does this make sense, or are you aware of any potential problems down the line?

[pelson]

I think this is looking good. Thanks @maxalbert. @mdboom - would you mind commenting?

[maxalbert]

@WeatherGod I just double-checked but unfortunately this does not fix #3540.

[mdboom]

Thanks for this. I'm concerned about the performance impact of this pretty central piece of code -- but I have no suggestion to improve it. As I mentioned in #3866 I'm not crazy about the way transform can take either a set of points or a single point like this. Eventually, I'd like to just stop supporting the single point case and this could revert back. But in the meantime, we do need to support the interface we have without breaking in usual cases, so a reluctant 👍 from me.

[maxalbert]

I was wondering about the performance impact as well so I just did some quick-and-dirty profiling in IPython. I'm simply using

import matplotlib.pyplot as plt
fig, ax = plt.subplots()

as the setup and am timing various transforms by applying ax.transData to arrays of various shapes and data types. The transform prints as

CompositeGenericTransform(TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())), CompositeGenericTransform(BboxTransformFrom(TransformedBbox(Bbox([[0.0, 0.0], [1.0, 1.0]]), TransformWrapper(BlendedAffine2D(IdentityTransform(),IdentityTransform())))), BboxTransformTo(TransformedBbox(Bbox([[0.125, 0.1], [0.9, 0.9]]), BboxTransformTo(TransformedBbox(Bbox([[0.0, 0.0], [8.0, 6.0]]), Affine2D(array([[ 80.,   0.,   0.],
       [  0.,  80.,   0.],
       [  0.,   0.,   1.]]))))))))

so it looks reasonably complicated. Do you think it's representative for a typical transform?

Anyway, here are the results:

Current master:

%timeit ax.transData.transform([0, 0])
10000 loops, best of 3: 52.1 µs per loop

%timeit ax.transData.transform([0., 0.])
10000 loops, best of 3: 51.9 µs per loop

%timeit ax.transData.transform([[0, 0], [1, 1]])
10000 loops, best of 3: 31.1 µs per loop

%timeit ax.transData.transform([[0., 0.], [1., 1.]])
10000 loops, best of 3: 31.1 µs per loop

pts = np.random.randint(0, 10, size=(20, 2))
%timeit ax.transData.transform(pts)
10000 loops, best of 3: 30 µs per loop

pts = np.random.random_sample((20, 2))
%timeit ax.transData.transform(pts)
10000 loops, best of 3: 27.2 µs per loop

On branch fix_log_transforms:

%timeit ax.transData.transform([0, 0])
10000 loops, best of 3: 40 µs per loop

%timeit ax.transData.transform([0., 0.])
10000 loops, best of 3: 37.2 µs per loop

%timeit ax.transData.transform([[0, 0], [1, 1]])
10000 loops, best of 3: 39.3 µs per loop

%timeit ax.transData.transform([[0., 0.], [1., 1.]])
10000 loops, best of 3: 36.6 µs per loop

pts = np.random.randint(0, 10, size=(20, 2))
%timeit ax.transData.transform(pts)
10000 loops, best of 3: 34.8 µs per loop

pts = np.random.random_sample((20, 2))
%timeit ax.transData.transform(pts)
10000 loops, best of 3: 32.6 µs per loop

These numbers are pretty reproducible. It looks like transforming a single point is actually faster now. There is a slight overhead for 2d arrays but it doesn't appear to be huge, so hopefully we won't see too much of a performance impact for now.

I take your point about treating the single-point case differently, though. I guess ultimately it's a tradeoff of user convenience vs. code tidiness (and perhaps performance). You core developers certainly have a much better feeling for what's best in this situation and I'm happy to go with that. If there is a consensus that single-point transforms shouldn't be supported despite this being a (slight) inconvenience for users then I'm happy to scrap this PR and open a new one.

[mdboom]

Thanks for the timings. I should have mentioned that my concerns were purely theoretical -- it's nice to have solid numbers.

I think the thing to do is to merge this now (now that my timing concerns are mostly assuaged), and consider changing the API later in a more considered way. In a sense, this is just a bugfix at this point.

[WeatherGod]

Just my two cents, I think it makes sense to accept single points as
inputs. Consider how numpy works fine for scalar, lists and arrays. I think
this sort of expectation carries over to other places as well.

On Fri, Dec 5, 2014 at 9:39 AM, Michael Droettboom <notifications@github.com

wrote:

Thanks for the timings. I should have mentioned that my concerns were
purely theoretical -- it's nice to have solid numbers.

I think the thing to do is to merge this now (now that my timing concerns
are mostly assuaged), and consider changing the API later in a more
considered way. In a sense, this is just a bugfix at this point.

—
Reply to this email directly or view it on GitHub
#3863 (comment)
.

[tacaswell]

backported as 9d97a21