-
Notifications
You must be signed in to change notification settings - Fork 131
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
Discuss typing of Cython API #236
Comments
Totally on board with this as we start to expose the Cython API! The appearance of Python types here was a result of the fact that the original API wasn't designed with vectorization in mind. |
This was referenced May 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Making a new issue to come back to what the Cython API should look like for best performance.
I added a couple commits to show how the vectorized functions could call our internal cython API instead of calling in to
h3lib
directly. But note this benchmark comparing against the master branch:Calling internal cython functions (commit
798da86
(#224) )On master branch (commit d1a5a3b)
geo_to_h3
is the same speed in each, buth3_to_parent
andh3_get_resolution
is roughly an order of magnitude on this branch at798da86
(#224).I believe the reasons for this is
the Cython
geo_to_h3
signature is fully typed, while theparent
signature is not. Additionally, theparent
function accepts Python, not C types.h3-py/src/h3/_cy/geo.pyx
Line 17 in d1a5a3b
h3-py/src/h3/_cy/cells.pyx
Line 133 in d1a5a3b
The
parent
andresolution
functions do extra checks withcheck_cell
andcheck_res
.I think it will be valuable in terms of code reuse for the vectorized API to call the internal Cython functions. So to keep performance high, I'd advocate for the Cython API to be fully typed. I.e.:
res = None
, useres = -1
If we also remove any Python types from Cython, our Cython API can be marked as
nogil
, which would allow our vectorized API to release the GIL.Bench script
Note this needs to be saved as an
.ipy
file and run withipython bench.ipy
Originally posted by @kylebarron in #224 (comment)
The text was updated successfully, but these errors were encountered: