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: stats.monte_carlo_test: add array API support #20604
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.
Some self-review to aid the reviewer.
@@ -1,13 +1,18 @@ | |||
import numpy as np |
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.
Again, didn't want to add to the mess of imports.
Very nice! This diff should be pretty clean once |
Thanks for taking a look @rgommers; responses are above. Another array API thing you might have thoughts about is the idea of making the default tolerance of |
If |
? You referred to the separate stats tracker. |
oops, sorry for the noise. temporary blindness there |
@rgommers I know you'll be out for a bit - shall I ask others to review/merge this in the meantime, or did you want to continue with it? |
Thanks for asking. Please don't feel like you have to wait for me. |
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.
Replied to comments. Thanks @j-bowhay!
[skip cirrus] [skip circle]
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 all looks idiomatic to me, at a glance at least 👍
scipy/stats/tests/test_resampling.py
Outdated
x = rng.random(10) | ||
x = xp.asarray(rng.standard_normal(size=10)) | ||
|
||
xp_test = array_namespace(x) |
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.
can we add a comment for why this is needed here too? (thanks for adding in the other places)
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.
Think this should be good to go once the merge conflicts and Lucas's comment is resolved
Thanks, all! I think I'll do |
Reference issue
Towards gh-20544
What does this implement/fix?
This PR adds array-API support for
scipy.stats.monte_carlo_test
.Additional information
Example (15~50x speedup locally, depending on dtype):