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

TST: SphericalVoronoi tests not sensitive to regions internal type #19177

Closed
tylerjereddy opened this issue Sep 2, 2023 · 0 comments · Fixed by #19358
Closed

TST: SphericalVoronoi tests not sensitive to regions internal type #19177

tylerjereddy opened this issue Sep 2, 2023 · 0 comments · Fixed by #19358
Labels
good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.spatial
Milestone

Comments

@tylerjereddy
Copy link
Contributor

tylerjereddy commented Sep 2, 2023

The regions attribute of SphericalVoronoi is defined (and currently is) a "list of lists," just like the attribute sharing the same name in the more general Voronoi class. I noticed while working on gh-19172 last night that I could simply change this to a "list of arrays" and the full SciPy test suite would pass! This isn't desirable, since some folks may depend on that structure I suppose, and the ragged data structure doesn't gain much advantage from using arrays internally in either of those classes.

Here's a sample diff that changes the return nested type without consequence:

diff --git a/scipy/spatial/_spherical_voronoi.py b/scipy/spatial/_spherical_voronoi.py
index 6b9ba8242..1f2f8e34e 100644
--- a/scipy/spatial/_spherical_voronoi.py
+++ b/scipy/spatial/_spherical_voronoi.py
@@ -224,7 +224,7 @@ class SphericalVoronoi:
         # intervals will have shape: (N+1,)
         intervals = np.cumsum(np.bincount(point_indices + 1))
         # split flattened groups to get nested list of unsorted regions
-        groups = [list(flattened_groups[intervals[i]:intervals[i + 1]])
+        groups = [flattened_groups[intervals[i]:intervals[i + 1]]
                   for i in range(len(intervals) - 1)]
         self.regions = groups

Here's a sample test modification that catches the problem:

--- a/scipy/spatial/tests/test_spherical_voronoi.py
+++ b/scipy/spatial/tests/test_spherical_voronoi.py
@@ -348,6 +348,7 @@ class TestSphericalVoronoi:
         # Tests that region integer type does not change
         # See Issue #13412
         sv = SphericalVoronoi(self.points)
+        assert isinstance(sv.regions[0], list)
         dtype = type(sv.regions[0][0])
         sv.sort_vertices_of_regions()
         assert type(sv.regions[0][0]) == dtype

Perhaps I'll label this "good first issue" and suggest making the test a little more robust than just checking a single element/region.

@tylerjereddy tylerjereddy added scipy.spatial good first issue Good topic for first contributor pull requests, with a relatively straightforward solution labels Sep 2, 2023
tylerjereddy added a commit to tylerjereddy/scipy that referenced this issue Oct 6, 2023
* Fixes scipy#19177

* enforce the nested list type for `SphericalVoronoi`
`regions`; the test module run time appears effectively
unchanged for me locally

[skip cirrus]
@j-bowhay j-bowhay added this to the 1.12.0 milestone Oct 11, 2023
j-bowhay pushed a commit that referenced this issue Oct 11, 2023
* Fixes #19177

* enforce the nested list type for `SphericalVoronoi`
`regions`; the test module run time appears effectively
unchanged for me locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.spatial
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants