From 22a17cb5b8853d2fe4103541d343ab7377a63e1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 25 Jul 2019 11:46:51 +0200 Subject: [PATCH] python: slightly improve perf of __getitem__ on matrices/vectors. Also test them properly. The full optimization is waiting for a pybind PR to get merged. --- src/python/magnum/math.matrix.h | 25 ++++++++++++++++++++----- src/python/magnum/math.vector.h | 15 ++++++++++++--- src/python/magnum/test/test_math.py | 16 ++++++++++++++++ 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/python/magnum/math.matrix.h b/src/python/magnum/math.matrix.h index 7367eb7..f2654ee 100644 --- a/src/python/magnum/math.matrix.h +++ b/src/python/magnum/math.matrix.h @@ -164,23 +164,38 @@ template void rectangularMatrix(py::class_& c) { .def(py::self != py::self, "Non-equality comparison") /* Set / get. Need to throw IndexError in order to allow iteration: - https://docs.python.org/3/reference/datamodel.html#object.__getitem__ */ + https://docs.python.org/3/reference/datamodel.html#object.__getitem__ + Using error_already_set is slightly faster than throwing index_error + directly, but still much slower than not throwing at all. Waiting + for https://github.com/pybind/pybind11/pull/1853 to get merged. */ .def("__setitem__", [](T& self, std::size_t i, const typename VectorTraits::Type& value) { - if(i >= T::Cols) throw pybind11::index_error{}; + if(i >= T::Cols) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } self[i] = value; }, "Set a column at given position") .def("__getitem__", [](const T& self, std::size_t i) -> typename VectorTraits::Type { - if(i >= T::Cols) throw pybind11::index_error{}; + if(i >= T::Cols) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } return self[i]; }, "Column at given position") /* Set / get for direct elements, because [a][b] = 2.5 won't work without involving shared pointers */ .def("__setitem__", [](T& self, const std::pair& i, typename T::Type value) { - if(i.first >= T::Cols || i.second >= T::Rows) throw pybind11::index_error{}; + if(i.first >= T::Cols || i.second >= T::Rows) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } self[i.first][i.second] = value; }, "Set a value at given col/row") .def("__getitem__", [](const T& self, const std::pair& i) { - if(i.first >= T::Cols || i.second >= T::Rows) throw pybind11::index_error{}; + if(i.first >= T::Cols || i.second >= T::Rows) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } return self[i.first][i.second]; }, "Value at given col/row") diff --git a/src/python/magnum/math.vector.h b/src/python/magnum/math.vector.h index 22b8e50..b71837a 100644 --- a/src/python/magnum/math.vector.h +++ b/src/python/magnum/math.vector.h @@ -191,13 +191,22 @@ template void vector(py::module& m, py::class_& c) { .def(py::self >= py::self, "Component-wise greater than or equal comparison") /* Set / get. Need to throw IndexError in order to allow iteration: - https://docs.python.org/3/reference/datamodel.html#object.__getitem__ */ + https://docs.python.org/3/reference/datamodel.html#object.__getitem__ + Using error_already_set is slightly faster than throwing index_error + directly, but still much slower than not throwing at all. Waiting + for https://github.com/pybind/pybind11/pull/1853 to get merged. */ .def("__setitem__", [](T& self, std::size_t i, typename T::Type value) { - if(i >= T::Size) throw pybind11::index_error{}; + if(i >= T::Size) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } self[i] = value; }, "Set a value at given position") .def("__getitem__", [](const T& self, std::size_t i) { - if(i >= T::Size) throw pybind11::index_error{}; + if(i >= T::Size) { + PyErr_SetString(PyExc_IndexError, ""); + throw pybind11::error_already_set{}; + } return self[i]; }, "Value at given position") diff --git a/src/python/magnum/test/test_math.py b/src/python/magnum/test/test_math.py index df97c13..a972a84 100644 --- a/src/python/magnum/test/test_math.py +++ b/src/python/magnum/test/test_math.py @@ -241,6 +241,12 @@ class Vector(unittest.TestCase): b.xy += (1, -1) self.assertEqual(b, Vector4i(4, 3, 15, 6)) + # OOB access should fire, as that's needed for iteration + with self.assertRaises(IndexError): + a[5] + with self.assertRaises(IndexError): + a[3] = 1.1 + def test_iterate(self): a = [i for i in Vector4(1.0, 3.25, 3.5, -1.125)] # assertAlmostEqual doesn't work on lists so i'm using values directly @@ -560,6 +566,16 @@ class Matrix(unittest.TestCase): a[0, 1] = 2.5 self.assertEqual(a[0], Vector3(1.0, 2.5, 3.0)) # yes, 2.5 + # OOB access should fire, as that's needed for iteration + with self.assertRaises(IndexError): + a[2] + with self.assertRaises(IndexError): + a[1, 3] + with self.assertRaises(IndexError): + a[2] = Vector3() + with self.assertRaises(IndexError): + a[2, 1] = 0.5 + @unittest.expectedFailure def test_set_two_brackets(self): a = Matrix2x3((1.0, 2.0, 3.0),