Skip to content
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.

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

Prevent get_coordinate_pointers from mutating input coordinates #205

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

sjones94549
Copy link
Contributor

@sjones94549 sjones94549 commented Feb 3, 2022

Index.get_coordinate_pointers(coordinates) converts point-inputs to bounding boxes. However, it does so using the += operator, which has the unfortunate side effect of mutating list inputs, and breaking with numpy arrays (which sum with += rather than appending). This change switches from += to = to prevent input-mutation, and uses coordinates = *coordinates, *coordinates for numpy compatibility.

Before

In [1]: import numpy as np
   ...: import rtree
   ...:
   ...: boxes15 = np.genfromtxt('boxes_15x15.data')
   ...: idx = rtree.index.Index()
   ...: for i, coords in enumerate(boxes15):
   ...:     idx.add(i, coords)

In [2]: # Tuples work and are not mutated (good)
   ...: point = 64, 64
   ...: list(idx.intersection(point))
Out[2]: [4, 53]

In [3]: point
Out[3]: (64, 64)

In [4]: # Lists work but are mutated (bad)
   ...: point = [64, 64]
   ...: list(idx.intersection(point))
Out[4]: [4, 53]

In [5]: point
Out[5]: [64, 64, 64, 64]  # <-- Mutated

In [6]: # Numpy arrays don't work (and would also be mutated) (bad)
   ...: point = np.array([64, 64])
   ...: idx.intersection(point)  # Breaks because `+=` sums in numpy rather than appends
---------------------------------------------------------------------------
RTreeError                                Traceback (most recent call last)
<ipython-input-6-1051681b49f7> in <module>
      1 point = np.array([64, 64])
----> 2 idx.intersection(point)  # Breaks because `+=` sums in numpy rather than appends

~/opt/anaconda3/envs/rtree/lib/python3.8/site-packages/rtree/index.py in intersection(self, coordinates, objects)
    675             return self._intersection_obj(coordinates, objects)
    676
--> 677         p_mins, p_maxs = self.get_coordinate_pointers(coordinates)
    678
    679         p_num_results = ctypes.c_uint64(0)

~/opt/anaconda3/envs/rtree/lib/python3.8/site-packages/rtree/index.py in get_coordinate_pointers(self, coordinates)
    342
    343         if len(coordinates) != dimension * 2:
--> 344             raise core.RTreeError(
    345                 "Coordinates must be in the form "
    346                 "(minx, miny, maxx, maxy) or (x, y) for 2D indexes")

RTreeError: Coordinates must be in the form (minx, miny, maxx, maxy) or (x, y) for 2D indexes

After

In [1]: import numpy as np
   ...: import rtree
   ...:
   ...: boxes15 = np.genfromtxt('boxes_15x15.data')
   ...: idx = rtree.index.Index()
   ...: for i, coords in enumerate(boxes15):
   ...:     idx.add(i, coords)

In [2]: point = 64, 64
   ...: list(idx.intersection(point))
Out[2]: [4, 53]

In [3]: point
Out[3]: (64, 64)

In [4]: point = [64, 64]
   ...: list(idx.intersection(point))
Out[4]: [4, 53]

In [5]: point
Out[5]: [64, 64]

In [6]: point = np.array([64, 64])

In [7]: list(idx.intersection(point))
Out[7]: [4, 53]

In [8]: point
Out[8]: array([64, 64])

@sjones94549
Copy link
Contributor Author

@hobu thoughts? If worthwhile, should I add a test similar to:

diff --git a/tests/test_index.py b/tests/test_index.py
index c3ac97a..d9ab209 100644
--- a/tests/test_index.py
+++ b/tests/test_index.py
@@ -260,6 +260,20 @@ class IndexIntersection(IndexTestCase):

         self.assertEqual([1, 1], list(idx.intersection((0, 0, 5, 5))))

+    def test_point_intersection(self):
+        """Using a point returns expected hits"""
+        point = 64, 64
+        self.assertEqual([4, 53], list(self.idx.intersection(point)))
+        self.assertEqual((64, 64), point)
+
+        point = [64, 64]
+        self.assertEqual([4, 53], list(self.idx.intersection(point)))
+        self.assertEqual([64, 64], point)
+
+        point = np.array([64, 64])
+        self.assertEqual([4, 53], list(self.idx.intersection(point)))
+        self.assertTrue(np.array_equal(np.array([64, 64]), point))
+

...or perhaps intersection() doesn't support points?

@hobu hobu added this to the 1.0.0 milestone Feb 3, 2022
@hobu
Copy link
Member

hobu commented Feb 3, 2022

The tests would be fine, although point-wise intersection hasn't been a promised feature of this library.

@sjones94549
Copy link
Contributor Author

The tests would be fine, although point-wise intersection hasn't been a promised feature of this library.

In that case, I'll leave the test out. I do find point-wise intersections useful, but will do so at my own risk 😀

@sjones94549 sjones94549 marked this pull request as ready for review February 4, 2022 02:16
@hobu hobu merged commit 0bc99bb into Toblerity:master Feb 4, 2022
@sjones94549 sjones94549 deleted the fix-mutated-inputs branch February 4, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants