-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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: special: add spherical Bessel functions to cython_special
#11385
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.
@person142 the changes look good to me. As a follow-up question, do you remember why the CI failed?
Merge conflicts, otherwise LGTM. I guess the typedef |
1b0ea5b
to
8eb1fb3
Compare
There's one failure here that's real, on 32-bit Linux on Azure:
|
Yeah I need to track that down... I think I saw that the failure was 32 bit only and was like "boy that's going to be a PITA to deal with" and keep pushing it off... |
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.
@person142 @rgommers I used my 32-bit SciPy Docker setup to look into this--I can reproduce the failure with cython==0.29.14
, but not with cython==0.29.19
(latest stable release).
We have historically bumped the minimum required Cython version pretty frequently without too much hesitation.
Perhaps we don't have to go quite as far as 0.29.19
from our current Cython>=0.29.14
, but I'm just trying to help move this forward since the reviews are positive and one of us could "bisect" the minimum required Cython version if really needed.
On the other hand, perhaps the version susceptibility will clue you in to the nature of the original issue, but I'd probably suggest just bumping, esp. if just one or two minor versions would suffice.
Ok, I did the bisection myself: |
Hm thanks for looking into that @tylerjereddy. Nothing in the relase notes leaps out at me immediately: https://cython.readthedocs.io/en/latest/src/changes.html#id13 If we're uncomfortable just bumping I can look into it, though since it's a patch version bump we should probably bump anyway. |
The spherical Bessel functions aren't currently available in Cython special because they aren't ufuncs: they have an optional `derivative` argument. But the underlying kernels are written in Cython and Cython supports optional arguments in `cdef` mode, so add custom wrappers that make them available. Closes scipygh-11029.
* bump minimum required Cython version to `0.29.18` to allow 32-bit Linux tests to pass for this feature branch/enhancement
8eb1fb3
to
020e17a
Compare
I went ahead and bumped to Cython I used a separate commit so you can always adjust/revert/whatever. I tried to catch the other places where the Cython version needed to be documented as well, but probably good to check that. The CI run should hopefully reveal any issues with the bump. |
CI looks "ok" apart from the ARM64 timeout. In theory adding more tests might contribute to that and having the low-level contribution tested on ARM might be useful, but I'd also like to make sure that the Cython version bump gets some time in CI, so I think I'll merge given positive reviewer feedback/CI and we'll keep an eye on the ARM test. |
Thanks for pushing this one through @tylerjereddy. |
Reference issue
Closes gh-11029.
What does this implement/fix?
The spherical Bessel functions aren't currently available in Cython
special because they aren't ufuncs: they have an optional
derivative
argument. But the underlying kernels are written in Cython and Cython
supports optional arguments in
cdef
mode, so add custom wrappersthat make them available.
Additional information
Most things in special are ufuncs, so I think it's reasonable to craft a
few custom wrappers for the remaining functions people care about.
The only other important one is probably
zeta
.