-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT cython typedefs in sparsefuncs_fast #27333
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
MAINT cython typedefs in sparsefuncs_fast #27333
Conversation
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.
LGTM. Thanks @lorentzenchr
@@ -15,6 +15,8 @@ | |||
# use these consistently throughout the codebase. | |||
# NOTE: Extend this list as needed when converting more cython extensions. | |||
ctypedef unsigned char uint8_t | |||
ctypedef unsigned int uint32_t |
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.
This has also been added in #27334
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 solved the conflicts. LGTM.
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.
Overall this looks like a net improvement. However I think we could be more specific for the types of scipy CSR indices (see comment below).
But we can leave that for a follow-up PR if needed.
/cc @jjerphan before merging.
@@ -7,18 +7,21 @@ | |||
# License: BSD 3 clause | |||
|
|||
from libc.math cimport fabs, sqrt, isnan | |||
from libc.stdint cimport intptr_t |
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.
Do we need this or better use our intp_t
instead?
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.
intp_t
aliases npy_intp
which aliases Py_intptr_t
which aliases intptr_t
I think we better use intp_t
for consistency. WDYT?
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 don’t think. I‘m confused and would prefer C/C++ over Cython, but that’s another story.
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.
Or can someone tell what the difference between inp_t
alias Py_ssize_t
and intptr_t
alias ssize_t
is?
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.
@jjerphan If this point is clear then maybe we can merge this PR?
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.
Yes, here this choice is safe, but I think we could clarify types for indices in another PR.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Related to #25572.
What does this implement/fix? Explain your changes.
Any other comments?