-
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
Add support for the concordant
mode test in Fisher's and Stouffer's estimators
#884
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #884 +/- ##
==========================================
+ Coverage 88.15% 88.17% +0.01%
==========================================
Files 48 48
Lines 6367 6374 +7
==========================================
+ Hits 5613 5620 +7
Misses 754 754 ☔ View full report in Codecov by Sentry. |
It looks like something changed in Nilearn's |
Setting |
This could also be addressed using sequential colormaps instead. I open an issue here to see if there is a better way to approach this. |
When I checked the gallery the other day, it looked like the RTD was not properly built. Setting |
nimare/resources/references.bib
Outdated
@@ -529,3 +529,16 @@ @article{geoffroy2001poisson | |||
year={2001}, | |||
publisher={Taylor \& Francis} | |||
} | |||
|
|||
@article{winkler2016non, |
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.
is this referenced in the permutedOLS class?
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.
Good catch! I added that when I defined the mode
parameter, but later, I removed it in favor of two_sided
and forgot to remove the citation.
@@ -337,12 +358,15 @@ class Stouffers(IBMAEstimator): | |||
|
|||
_required_inputs = {"z_maps": ("image", "z")} | |||
|
|||
def __init__(self, use_sample_size=False, **kwargs): | |||
def __init__(self, use_sample_size=False, two_sided=True, **kwargs): |
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 changes the default to concordant right? we should make note that this is different from previous behavior (but I do agree with changing this default behavior)
@jdkent Thanks for the review! I added a note for the change in the |
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, thanks @JulioAPeraza!
Closes None.
Changes proposed in this pull request:
directed
tests in Fisher's and Stouffer's estimators. The new parametertwo_sided
(following Nilearn's naming convention) adds support for theconcordant
(two-sided) test.