Merge pull request #32 from larsmans/exceptions

Get some exception safety, using Cython's built-in exception translation

Author: nzjrs

Makefile

@@ -1,4 +1,5 @@
-pcl/_pcl.so: pcl/_pcl.pyx setup.py pcl/pcl_defs.pxd pcl/minipcl.cpp
+pcl/_pcl.so: pcl/_pcl.pyx setup.py pcl/pcl_defs.pxd pcl/minipcl.cpp \
+             pcl/indexing.hpp
 	python setup.py build_ext --inplace
 
 test: pcl/_pcl.so tests/test.py

pcl/_pcl.pyx

@@ -174,7 +174,7 @@ cdef class PointCloud:
 
         cdef cpp.PointXYZ *p
         for i in range(npts):
-            p = &self.thisptr.at(i)
+            p = cpp.getptr(self.thisptr, i)
             p.x, p.y, p.z = arr[i, 0], arr[i, 1], arr[i, 2]
 
     @cython.boundscheck(False)
@@ -190,7 +190,7 @@ cdef class PointCloud:
         result = np.empty((n, 3), dtype=np.float32)
 
         for i in range(n):
-            p = &self.thisptr.at(i)
+            p = cpp.getptr(self.thisptr, i)
             result[i, 0] = p.x
             result[i, 1] = p.y
             result[i, 2] = p.z
@@ -206,8 +206,8 @@ cdef class PointCloud:
         self.resize(npts)
         self.thisptr.width = npts
         self.thisptr.height = 1
-        for i,l in enumerate(_list):
-            p = &self.thisptr.at(i)
+        for i, l in enumerate(_list):
+            p = cpp.getptr(self.thisptr, i)
             p.x, p.y, p.z = l
 
     def to_list(self):
@@ -223,11 +223,11 @@ cdef class PointCloud:
         """
         Return a point (3-tuple) at the given row/column
         """
-        cdef cpp.PointXYZ *p = &self.thisptr.at(row, col)
+        cdef cpp.PointXYZ *p = cpp.getptr_at(self.thisptr, row, col)
         return p.x, p.y, p.z
 
     def __getitem__(self, cnp.npy_intp idx):
-        cdef cpp.PointXYZ *p = &self.thisptr.at(idx)
+        cdef cpp.PointXYZ *p = cpp.getptr_at(self.thisptr, idx)
         return p.x, p.y, p.z
 
     def from_file(self, char *f):
@@ -566,15 +566,21 @@ cdef class OctreePointCloud:
     Octree pointcloud
     """
     cdef cpp.OctreePointCloud_t *me
-   
+
     def __cinit__(self, double resolution):
+        self.me = NULL
+        if resolution <= 0.:
+            raise ValueError("Expected resolution > 0., got %r" % resolution)
+
+    def __init__(self, double resolution):
         """
         Constructs octree pointcloud with given resolution at lowest octree level
         """ 
         self.me = new cpp.OctreePointCloud_t(resolution)
-    
+
     def __dealloc__(self):
         del self.me
+        self.me = NULL      # just to be sure
 
     def set_input_cloud(self, PointCloud pc):
         """
@@ -637,9 +643,6 @@ cdef class OctreePointCloudSearch(OctreePointCloud):
         """ 
         self.me = <cpp.OctreePointCloud_t*> new cpp.OctreePointCloudSearch_t(resolution)
 
-    def __dealloc__(self):
-        del self.me
-
     def radius_search (self, point, double radius, unsigned int max_nn = 0):
         """
         Search for all neighbors of query point that are within a given radius.

pcl/indexing.hpp

@@ -0,0 +1,21 @@
+namespace {
+    // Workaround for a Cython bug in operator[] with templated types and
+    // references. Let's hope the compiler optimizes these functions away.
+    template <typename T>
+    T *getptr(pcl::PointCloud<T> *pc, size_t i)
+    {
+        return &(*pc)[i];
+    }
+
+    template <typename T>
+    T *getptr_at(pcl::PointCloud<T> *pc, size_t i)
+    {
+        return &(pc->at(i));
+    }
+
+    template <typename T>
+    T *getptr_at(pcl::PointCloud<T> *pc, int i, int j)
+    {
+        return &(pc->at(i, j));
+    }
+}

pcl/pcl_defs.pxd

@@ -16,11 +16,17 @@ cdef extern from "pcl/point_cloud.h" namespace "pcl":
         bool is_dense
         void resize(size_t) except +
         size_t size()
-        T& operator[](size_t)
-        T& at(size_t)
-        T& at(int, int)
+        #T& operator[](size_t)
+        #T& at(size_t) except +
+        #T& at(int, int) except +
         shared_ptr[PointCloud[T]] makeShared()
 
+cdef extern from "indexing.hpp":
+    # Use these instead of operator[] or at.
+    PointXYZ *getptr(PointCloud[PointXYZ] *, size_t)
+    PointXYZ *getptr_at(PointCloud[PointXYZ] *, size_t) except +
+    PointXYZ *getptr_at(PointCloud[PointXYZ] *, int, int) except +
+
 cdef extern from "pcl/point_types.h" namespace "pcl":
     cdef struct PointXYZ:
         PointXYZ()

tests/test.py

@@ -208,6 +208,20 @@ def testExtractNeg(self):
         self.assertNotEqual(self.p, p2)
         self.assertEqual(p2.size, self.p.size - 3)
 
+class TestExceptions(unittest.TestCase):
+    def setUp(self):
+        self.p = pcl.PointCloud(np.arange(9, dtype=np.float32).reshape(3, 3))
+
+    def testIndex(self):
+        self.assertRaises(IndexError, self.p.__getitem__, self.p.size)
+        self.assertRaises(Exception, self.p.get_point, self.p.size, 1)
+
+    def testResize(self):
+        # XXX MemoryError isn't actually the prettiest exception for a
+        # negative argument. Don't hesitate to change this test to reflect
+        # better exceptions.
+        self.assertRaises(MemoryError, self.p.resize, -1)
+
 class TestSegmenterNormal(unittest.TestCase):
 
     def setUp(self):
@@ -312,6 +326,9 @@ def setUp(self):
         self.t.define_bounding_box()
         self.t.add_points_from_input_cloud()
 
+    def testConstructor(self):
+        self.assertRaises(ValueError, pcl.OctreePointCloudSearch, 0.)
+
     def testRadiusSearch(self):
         good_point = (0.035296999, -0.074322999, 1.2074)
         rs = self.t.radius_search(good_point, 0.5, 1)