Regression in transforms: raises exception when applied to single point #3866

mdboom · 2014-12-01T19:18:17Z

The following code snippet (taken from #3809) which transforms a single point used to work in v1.4.2 but does not work with latest master (it raises "ValueError: Expected 2-dimensional array, got 1"). Bisection of the history showed that the regression was introduced in commit be34210 when PR #3646 was merged.

from matplotlib.backends.backend_agg import FigureCanvasAgg as FigureCanvas
from matplotlib.figure import Figure
import numpy as np

fig = Figure(figsize=(8,6))
canvas = FigureCanvas(fig)
ax = fig.add_subplot(1,1,1)
ax.transData.transform((1,1)) # <--- exception thrown here

Full traceback:

Traceback (most recent call last):
  File "regression.py", line 9, in <module>
    ax.transData.transform((1,1)) # <--- exception thrown here
  File "/home/albert/work/code/matplotlib/lib/matplotlib/transforms.py", line 1289, in transform
    return self.transform_affine(self.transform_non_affine(values))
  File "/home/albert/work/code/matplotlib/lib/matplotlib/transforms.py", line 2259, in transform_affine
    return self.get_affine().transform(points)
  File "/home/albert/work/code/matplotlib/lib/matplotlib/transforms.py", line 1583, in transform
    return self.transform_affine(values)
  File "/home/albert/work/code/matplotlib/lib/matplotlib/transforms.py", line 1667, in transform_affine
    return affine_transform(points, mtx)
ValueError: Expected 2-dimensional array, got 1

mdboom · 2014-12-01T19:18:32Z

Sorry -- I didn't realise this "feature" (which is a little dubious) was exposed all the way down to the public API. The attached patch should fix this.

tacaswell · 2014-12-02T04:46:22Z

src/_path_wrapper.cpp

-                          "O&O&:affine_transform",
-                          &vertices.converter,
-                          &vertices,
+                          "OO&:affine_transform",


Again, things I may not really want to know, but will ask anyway, what is O&O& vs OO&?

https://docs.python.org/3/c-api/arg.html#other-objects
The O means dump the object in a C pointer (vertices_obj); the O& means use a converter function (convert_trans_affine) to convert the object before dumping the result in a C pointer (trans). This is the as-modified version; in the previous version, each of the two arguments was processed by a converter.

Exactly. In order to support the fact that the vertices array can be either 1-d or 2-d, I needed to do the conversion manually below rather than using a converter function.

maxalbert · 2014-12-02T14:17:09Z

@mdboom Thanks for fixing this so quickly! Just out of interest, why did you say that this feature was a little "dubious"? From a user's perspective it seems nice that you can transform both a single point and a list of points (and this is what the documentation makes use of, too). Are you suggesting that it would be better to only allow transforming lists of points? From a user's point of view, having to write something like ax.transData.transform([(1,1)]) feels much more clumsy, though. Moreover, it may not be immediately obvious to new users what's wrong when they see the error message because intuitively they probably expect to be able to transform a single point.

pelson · 2014-12-02T15:29:29Z

src/_path_wrapper.cpp

-    CALL_CPP("affine_transform", (affine_transform(vertices, trans, result)));
-
-    return result.pyobj();
+    try {


Try catch, or just test the dimensionality of the object? Is there a preferred paradigm?

Good point, I had the same thought when I read it. Testing for dimensionality seems slightly clearer to me personally because it doesn't treat the 1d case as "effectively wrong, but ok let's support it anyway". I don't have any strong opinions on this, though.

I was trying to avoid the 4-5 lines of complex code, involving reference counting, just to convert to an array to check its dimensionality, when that's neatly encapsulated in the numpy::array_view constructor in an exception-safe way. If others feel strongly, I'm willing to change it, but I think this is less error-prone.

Again, no strong opinions from my side. It was just my first thought when I read the code, but with your explanation I'm happy to leave it this way. One question though, since I'm not really familiar with the C++ side of things: is it safe to catch a generic exception, or is there a way to specifically catch the ValueError that is triggered if the array is not 2d? I can't see much of a problem here, I'm just asking because I've been bitten by generic "catch" statements quite a few times in the past (but this was always on a much higher Python level with more potential for other exceptions to be triggered by the same piece of code, which doesn't seem to be the case here).

I think in this specific instance, the generic catch is fine -- and we don't actually have much choice: All Python exceptions "thrown" as py::exception on the C++ side anyway.

pelson · 2014-12-03T17:22:09Z

👍 but for the test from me.

mdboom · 2014-12-03T17:32:03Z

It's dubious because the return type depends on the input type, and the input type is not very well defined. I think the method for transforming a single point should probably be a separate method, but that would break backward compatibility for no major gain...

Regression in transforms: raises exception when applied to single point

pelson added the mentored: hackathon label Nov 30, 2014

tacaswell assigned mdboom Nov 30, 2014

tacaswell added this to the v1.5.x milestone Nov 30, 2014

Support transforming a single point

f006189

tacaswell added the status: needs review label Dec 1, 2014

tacaswell reviewed Dec 2, 2014
View reviewed changes

Fix mistake

56e2420

pelson reviewed Dec 2, 2014
View reviewed changes

Add assert to test

733570e

pelson added a commit that referenced this pull request Dec 3, 2014

Merge pull request #3866 from mdboom/transform-single-point

77ccc82

Regression in transforms: raises exception when applied to single point

pelson merged commit 77ccc82 into matplotlib:master Dec 3, 2014

tacaswell removed the status: needs review label Dec 3, 2014

maxalbert mentioned this pull request Dec 4, 2014

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

Merged

mdboom deleted the transform-single-point branch March 3, 2015 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression in transforms: raises exception when applied to single point #3866

Regression in transforms: raises exception when applied to single point #3866

mdboom commented Dec 1, 2014

mdboom commented Dec 1, 2014

tacaswell Dec 2, 2014

efiring Dec 2, 2014

mdboom Dec 2, 2014

maxalbert commented Dec 2, 2014

pelson Dec 2, 2014

maxalbert Dec 2, 2014

mdboom Dec 2, 2014

maxalbert Dec 2, 2014

mdboom Dec 3, 2014

pelson commented Dec 3, 2014

mdboom commented Dec 3, 2014

Regression in transforms: raises exception when applied to single point #3866

Regression in transforms: raises exception when applied to single point #3866

Conversation

mdboom commented Dec 1, 2014

mdboom commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxalbert commented Dec 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented Dec 3, 2014

mdboom commented Dec 3, 2014