Vectorized functions #147
Vectorized functions #147
Conversation
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 203 203
=========================================
Hits 203 203 Continue to review full report at Codecov.
|
|
Remove the demo notebook and move it to the demo notebooks repo. |
| from cython cimport boundscheck, wraparound | ||
| from libc.math cimport sqrt, sin, cos, asin | ||
|
|
||
| cdef double haversineDistance(double th1, double ph1, double th2, double ph2) nogil: |
There was a problem hiding this comment.
Does this belong in this binding?
I don't think we provide haversine distance support in any of the other bindings?
There was a problem hiding this comment.
I'm considering anything under h3.unstable as "not in the binding". There was a request for an optimized cell_haversine function, so I'm putting it in to make it easy for them to play with, and evaluate the speedups.
In the short term, this is hard to do anywhere except within the Cython bindings. In the future however, we could consider making it easy for folks to do this outside the Cython bindings, by letting them write their own Cython code which just does cimport h3 to get access to the C and Cython code in these bindings. Here's an issue around it: #152
I'd say that's probably the way to go in the future.
There was a problem hiding this comment.
It's pretty easy to make a vectorized haversine in numpy only. So for the future if/when vectorized functions make it out of unstable, it might be ideal to only make h3_to_geo vectorized, then the end user can have a vectorized pipeline without touching cython themselves:
# a, b arrays of hex ids
a_geo = h3_to_geo(a)
b_geo = h3_to_geo(b)
dist = haversine(a_geo, b_geo)There was a problem hiding this comment.
We will expose a haversine function when uber/h3#377 lands:
It would be easy to add that to the h3-py "vectorized" API, whenever we figure out the best way to do that. I'd be interesting to compare it to the "numpy-vectorized" version you have above!
| @@ -48,9 +48,9 @@ cdef extern from "h3api.h": | |||
| LinkedGeoLoop *_data_last "last" # not needed in Cython bindings | |||
| LinkedGeoPolygon *next | |||
|
|
|||
| H3Index geoToH3(const GeoCoord *g, int res) | |||
| H3Index geoToH3(const GeoCoord *g, int res) nogil | |||
There was a problem hiding this comment.
How do you determine which functions get nogil? Is it just the ones you want to optimize right now?
There was a problem hiding this comment.
I just added it to the functions I needed it for in this PR. We could probably add nogil to each of the functions in the API, as the nogil tag just promises that the function won't manipulate any Python objects, which is true for everything in the h3-c API.
I'll probably do a more thorough job of this when I tackle #140
No description provided.