-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
ENH: stats: add array API support to combine_pvalues #20900
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
Just |
c2e8b70
to
9183338
Compare
Ready for review |
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.
Since _axis_nan_policy
doesn't add axis
support for alternative backends, would it be much trouble to add axis
manually?
|
||
def test_stouffer(self, xp): | ||
Z, p = stats.combine_pvalues(xp.asarray([.01, .2, .3]), method='stouffer') | ||
xp_assert_close(p, xp.asarray(0.01651), rtol=1e-3) |
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 know you were just copying what is here, but I'd be surprised if you can't use rtol
equal to the precision of the reference p-value, since it is always much coarser than the precision of the floating point dtype.
I'd also be willing to review a refactor that parameterizes over case
, where case
includes the method
and reference p-value (and preferably statistic, since it's looks like it's currently untested). Testing Stoufer weights could be moved to a separate test, and testing the cases in which the p-values are equal could either be a separate test (parameterized over all methods) or removed if it's not actually important.
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.
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.
Them the reference p-value is probably wrong. Fine to leave it alone for this PR, but maybe when this is refactored I'll look for a better reference that we can cite in the test.
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.
Well this is indeed what R gives.
library(metap)
x = c(0.01, 0.2, 0.3)
sumz(x)
sumz = 2.131791 p = 0.01651203
I'll see if there are any hints in the code or documentation as to why there is such a discrepancy.
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.
No such hints. I think It's worth follow-up in a refactoring PR. I don't think an issue is needed if you were willing to submit such a PR soon. LMK either way.
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 correct; just a few thoughts on how we could reduce the diff and improve the existing code. It's OK if the improvements are here or a separate PR or neither, but I'd be happy to review them.
Happy to tackle the test refactor and native axis argument but would prefer them to be follow ups |
Thanks @j-bowhay. Re test-refactor and native options(digits=16)
library(metap)
x = c(0.01, 0.2, 0.3)
sumz(x) Fine to use default tolerances in |
towards #20544
Adds array API support to
combine_pvalues