-
Notifications
You must be signed in to change notification settings - Fork 58
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
Refactor Correctors and remove statsmodels requirement #679
Conversation
Codecov Report
@@ Coverage Diff @@
## main #679 +/- ##
==========================================
- Coverage 85.32% 85.21% -0.12%
==========================================
Files 41 41
Lines 4503 4483 -20
==========================================
- Hits 3842 3820 -22
- Misses 661 663 +2
Continue to review full report at Codecov.
|
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.
Looks good!
Main comment is I'm curious if the method parameter documentation could be defined once and referenced in other places, Don't consider it a blocking review comment, but it's nice not to have to repeat yourself, maybe rst substitutions would do what you want? https://stackoverflow.com/questions/8320909/repeated-parameters-with-python-sphinx
I think that's what nilearn does, and I certainly like it. Would you mind if I tackled that in a separate PR? I might open an issue about it. |
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.
Looks good to me, although I wonder if these non-imaging basic functions should perhaps live in pyMARE?
That seems reasonable to me, but PyMARE doesn't use these methods at the moment, so we would probably only want to add them to that package if we also create a Corrector class within PyMARE as well. |
…to drop-statsmodels
@adelavega I've moved the new functions to PyMARE. I'll make a release once the associated PR (neurostuff/PyMARE#87) is merged. EDIT: I've released PyMARE 0.0.4rc1 and pinned it here. |
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.
LGTM!
I've opened an issue about the parameter definitions (#687), so I'm going to resolve the conflicts and merge this once CI passes. |
Closes #667.
Changes proposed in this pull request:
nimare.stats.fdr
.MKDAChi2.correct_fwe_bh
toMKDAChi2.correct_fwe_indep
(breaking change).To do: