Skip to content
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

Modules for multitesting #193

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Modules for multitesting #193

wants to merge 3 commits into from

Conversation

Claybarn
Copy link

Added modules for multiple hypothesis testing and stratified multiple hypothesis testing.

Would be great to get a second pair of eyes on this and make sure everything checks out!

Added modules for multiple hypothesis testing and stratified multiple hypothesis testing
@akglazer
Copy link
Contributor

Hi Clayton, thanks so much for this! It looks like a potentially very useful contribution. Could you add some more comments to your code and a few unit tests so we can better evaluate it?

Added comments for clarity and tests for multitest modules.
@Claybarn
Copy link
Author

Claybarn commented Mar 25, 2022

Hey Amanda, done!

I noticed what I thought was strange behavior in the stratified module. On line 130 the number of groups is used to decide whether to use mean or std as the statistic. However I believe this should be the number of conditions. I made this change for the multitest_stratified module, but if I am mistaken I can change it back to groups.

I also had the thought it would be more parsimonious to fold the multitest modules into the core and stratified modules, as the desired behavior of the multitest modules with a single test is identical to the original modules. But open to whatever!

@Claybarn
Copy link
Author

@akglazer Did you get a chance to look over the tests?

@akglazer
Copy link
Contributor

Hi Clayton! So sorry for the delay on this and thanks so much for all your work! What method are you implementing in this code? It looks like Westfall-Young maxT?

@Claybarn
Copy link
Author

Claybarn commented Jun 8, 2022

No worries! Yes this is the Westfall-Young method. If I have time in the next few weeks I will speed things up by implementing the Numba library which has worked beautifully for me in the past.

@akglazer
Copy link
Contributor

Thanks so much! We really appreciate the time you've put into this and for reminding us of the need for multiple testing corrections. We've been working on implementing Westfall-Young, along with other corrections, and think ultimately it will be better integrated in npc.py so we can leverage the Experiment class and minimize code duplication.

Removed max statistic multiple hypothesis testing correction and made corresponding adjustments to the multitest_core and multitest_stratified tests. Also touched up some of the tests to make them run smoother.
@Claybarn
Copy link
Author

I failed to notice npc and the experiment class! Makes sense. Removed the Westfall-Young method in this PR and updated the tests accordingly.

@Claybarn
Copy link
Author

Claybarn commented Nov 1, 2022

Hey @akglazer, any chance of merging this soon?

@akglazer
Copy link
Contributor

akglazer commented Nov 3, 2022

Hi Clayton, we are planning to add multiple testing functionality soon, we are just working out some changes to the code structure still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants