-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
ENH: array types, signal: delegate to CuPy and JAX for correlations and convolutions #20772
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
Conversation
scipy.submodule.func
to cupyx.scipy.submodule.func
cupyx.scipy.submodule.func
cupyx.scipy.submodule.func
cupyx.scipy.submodule.func
Thanks for the suggestion @lucascolley ! This is indeed a nice exercise to check how general it is. The last commit adds the dispatch. Curiously,
Am adding |
I assume it's |
e8d894b
to
1443958
Compare
There is at least one issue which looks like f32/f64 tolerance and there's a bunch of instances where jax returns float64 while cupyx and scipy preserve int64. I'd be very wary of relaxing scipy tests on these. Either it's fixed upstream (if jax.scipy follows scipy), or we special-case it in tests (not in this PR I guess), or it just stays an xfail.
I'm not going to argue with whatever name you prefer. Mind pushing a tweak? |
As long as that is deliberate from If you still believe that |
cupyx.scipy.submodule.func
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.
For the CI failures, I suspect you need to restrict pytest.mark.usefixtures("skip_xp_backends")
to functions/classes you have converted, rather than putting it in pytestmark
, for now.
We should get CI green for NumPy first by being explicit about what dtype we expect (or using |
403ed97
to
b9e4f12
Compare
OK, reworked to mimic Have to admit I don't much like the One other simplification is about who is responsible for when an accelerator only implements a part of the scipy API. For instance, |
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.
@mdhaber could you take a look at _support_alternative_backends.py
as the author of the equivalent in special
?
1d17a32
to
6d825ee
Compare
6d825ee
to
2161976
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.
I had a look over this, spotted a few things, and probably missed a bunch more! Maybe we should just merge this since since there's a decent period before the next release to iron out any issues
my term has just finished so I should have time to take a look at this |
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.
approving the production code changes! (modulo a few observations)
I'll take a look at the test changes once the existing comments have been addressed
159a28a
to
41d4c3c
Compare
41d4c3c
to
6d6543f
Compare
Addressed review comments.
Certainly would be happy to :-). NB smoke-docs is going to fail left and right because of scipy/scipy_doctest#175, here and elsewhere. |
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 LGTM, thanks Evgeni & reviewers! Just a few comments
Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
CI is green modulo two known and unrelated issues (mypy and smoke-docs), there are two approvals, merging as approved and CI-green. Thanks to all reviewers! |
Which of the prs stacked on this is the "next"? |
Towards gh-20678
What does this implement/fix?
Delegate
signal.convolve
and its ilk tocupyx.scipy.signal.convolve
if inputs are cupy arrays. For other array types, do the usual convert to numpy, run, convert back dance.Additional information
CuPy provides a near-complete clone of the scipy API in the
cupyx.scipy
namespace. We can treat these CuPy functions as accelerators: if a scipy function detects that its arguments are cupy-compatible, it can delegate all work to the cupyx function.