Regression in transforms: raises exception when applied to single point

[mdboom]

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]

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]

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

[efiring]

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.

[mdboom]

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]

@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]

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

[maxalbert]

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.

[mdboom]

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.

[maxalbert]

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).

[mdboom]

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]

👍 but for the test from me.

[mdboom]

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...