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
ENH: SphericalVoronoi sort faster #19419
Conversation
scipy/spatial/_voronoi.pyx
Outdated
in regions]), | ||
dtype=np.intp) | ||
cdef np.ndarray[np.intp_t, ndim=1] sorted_vertices | ||
sorted_vertices = np.empty(max([len(region) for region |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make a for loop here with
max_len = 0
for k in regions:
max_len = max(max_len, len(k))
so that you don't create a temporary list in case region numbers are high. If regions have a particular type it would be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and applied that change. The benchmarks were neutral on it, but I also didn't have a reason not to make it if it helps for larger cases than are covered by the suite.
FYI: The CI stage with numpy nightly build will fail - I need to remove a deprecation warning for |
Here are two PRs to address warnings/errors present in dev CI stage: |
* avoid a large number of memoryview->ndarray conversions in the `sort_vertices_of_regions` loop * `asv continuous -e -b ".*SphericalVor.*" main treddy_sph_vor_sort_faster` ``` | Change | Before [5dc8a79] <main> | After [a0350aee] <treddy_sph_vor_sort_faster> | Ratio | Benchmark (Parameter) | |----------|----------------------------|-------------------------------------------------|---------|-----------------------------------------------------------------------| | - | 12.4±0.02ms | 10.9±0.1ms | 0.88 | spatial.SphericalVorSort.time_spherical_polygon_vertex_sorting(10000) | | - | 1.23±0ms | 1.07±0ms | 0.87 | spatial.SphericalVorSort.time_spherical_polygon_vertex_sorting(1000) | | - | 6.17±0.01ms | 5.33±0.04ms | 0.86 | spatial.SphericalVorSort.time_spherical_polygon_vertex_sorting(5000) | | - | 122±4μs | 104±1μs | 0.85 | spatial.SphericalVorSort.time_spherical_polygon_vertex_sorting(100) | | - | 11.9±0.02μs | 10.0±0.08μs | 0.84 | spatial.SphericalVorSort.time_spherical_polygon_vertex_sorting(10) | SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. PERFORMANCE INCREASED. ``` [skip cirrus] [skip circle]
* apply reviewer suggestion to improve efficiency of initialization of `sorted_vertices` [skip cirrus] [skip circle]
fb716dd
to
3f15350
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem sensible, plus everything is green and the speed up is nice
avoid a large number of memoryview->ndarray conversions in the
sort_vertices_of_regions
loopasv continuous -e -b ".*SphericalVor.*" main treddy_sph_vor_sort_faster
[skip cirrus] [skip circle]