Skip to content

Conversation

j-bowhay
Copy link
Member

@j-bowhay j-bowhay commented Jun 5, 2024

towards #20544

Adds array API support to combine_pvalues

@github-actions github-actions bot added scipy.stats enhancement A new feature or improvement labels Jun 5, 2024
@j-bowhay
Copy link
Member Author

j-bowhay commented Jun 5, 2024

Just test_monotonicity to go

@j-bowhay j-bowhay changed the title WIP:ENH: stats: add array API support to combine_pvalues ENH: stats: add array API support to combine_pvalues Jun 5, 2024
@j-bowhay j-bowhay force-pushed the xp_combine_pvalues branch from c2e8b70 to 9183338 Compare June 5, 2024 23:37
@j-bowhay j-bowhay requested a review from mdhaber June 5, 2024 23:37
@j-bowhay
Copy link
Member Author

j-bowhay commented Jun 5, 2024

Ready for review

@lucascolley lucascolley added the array types Items related to array API support and input array validation (see gh-18286) label Jun 6, 2024
Copy link
Contributor

@mdhaber mdhaber left a 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)
Copy link
Contributor

@mdhaber mdhaber Jun 6, 2024

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tightening the tolerance causes a failure on all backends
image

Copy link
Contributor

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.

Copy link
Contributor

@mdhaber mdhaber Jun 7, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor

@mdhaber mdhaber left a 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.

@j-bowhay
Copy link
Member Author

j-bowhay commented Jun 7, 2024

Happy to tackle the test refactor and native axis argument but would prefer them to be follow ups

[skip ci]
@mdhaber mdhaber merged commit b4c57df into scipy:main Jun 7, 2024
@mdhaber
Copy link
Contributor

mdhaber commented Jun 7, 2024

Thanks @j-bowhay. Re test-refactor and native axis argument, would you also calculate statistic values and recalculate p-values with metap? E.g. for Stouffer:

options(digits=16)
library(metap)
x = c(0.01, 0.2, 0.3)
sumz(x)

Fine to use default tolerances in xp_assert_close, but I think it will be worth checking out any failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array types Items related to array API support and input array validation (see gh-18286) enhancement A new feature or improvement scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants