Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 3, 2014
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f006189
Support transforming a single point
mdboom Dec 1, 2014
56e2420
Fix mistake
mdboom Dec 2, 2014
733570e
Add assert to test
mdboom Dec 3, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/matplotlib/tests/test_transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,11 @@ def test_bbox_as_strings():
assert_equal(eval(format(getattr(b, k), fmt)), v)


def test_transform_single_point():
t = mtrans.Affine2D()
r = t.transform_affine((1, 1))
assert r.shape == (2,)

if __name__=='__main__':
import nose
nose.runmodule(argv=['-s','--with-doctest'], exit=False)
29 changes: 28 additions & 1 deletion src/_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ clip_path_to_rect(PathIterator &path, agg::rect_d &rect, bool inside, std::vecto
}

template <class VerticesArray, class ResultArray>
void affine_transform(VerticesArray &vertices, agg::trans_affine &trans, ResultArray &result)
void affine_transform_2d(VerticesArray &vertices, agg::trans_affine &trans, ResultArray &result)
{
if (vertices.dim(0) != 0 && vertices.dim(1) != 2) {
throw "Invalid vertices array.";
Expand Down Expand Up @@ -716,6 +716,33 @@ void affine_transform(VerticesArray &vertices, agg::trans_affine &trans, ResultA
}
}

template <class VerticesArray, class ResultArray>
void affine_transform_1d(VerticesArray &vertices, agg::trans_affine &trans, ResultArray &result)
{
if (vertices.dim(0) != 2) {
throw "Invalid vertices array.";
}

double x;
double y;
double t0;
double t1;
double t;

x = vertices(0);
y = vertices(1);

t0 = trans.sx * x;
t1 = trans.shx * y;
t = t0 + t1 + trans.tx;
result(0) = t;

t0 = trans.shy * x;
t1 = trans.sy * y;
t = t0 + t1 + trans.ty;
result(1) = t;
}

template <class BBoxArray>
int count_bboxes_overlapping_bbox(agg::rect_d &a, BBoxArray &bboxes)
{
Expand Down
26 changes: 16 additions & 10 deletions src/_path_wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,24 +419,30 @@ const char *Py_affine_transform__doc__ = "affine_transform(points, trans)";

static PyObject *Py_affine_transform(PyObject *self, PyObject *args, PyObject *kwds)
{
numpy::array_view<const double, 2> vertices;
PyObject *vertices_obj;
agg::trans_affine trans;

if (!PyArg_ParseTuple(args,
"O&O&:affine_transform",
&vertices.converter,
&vertices,
"OO&:affine_transform",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

&vertices_obj,
&convert_trans_affine,
&trans)) {
return NULL;
}

npy_intp dims[] = { vertices.dim(0), 2 };
numpy::array_view<double, 2> result(dims);

CALL_CPP("affine_transform", (affine_transform(vertices, trans, result)));

return result.pyobj();
try {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

numpy::array_view<double, 2> vertices(vertices_obj);
npy_intp dims[] = { vertices.dim(0), 2 };
numpy::array_view<double, 2> result(dims);
CALL_CPP("affine_transform", (affine_transform_2d(vertices, trans, result)));
return result.pyobj();
} catch (py::exception) {
numpy::array_view<double, 1> vertices(vertices_obj);
npy_intp dims[] = { vertices.dim(0) };
numpy::array_view<double, 1> result(dims);
CALL_CPP("affine_transform", (affine_transform_1d(vertices, trans, result)));
return result.pyobj();
}
}

const char *Py_count_bboxes_overlapping_bbox__doc__ = "count_bboxes_overlapping_bbox(bbox, bboxes)";
Expand Down