-
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 publication size weighting. General refactoring of IBMA estimators #887
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #887 +/- ##
==========================================
+ Coverage 88.17% 88.19% +0.02%
==========================================
Files 48 48
Lines 6374 6335 -39
==========================================
- Hits 5620 5587 -33
+ Misses 754 748 -6 ☔ View full report in Codecov by Sentry. |
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, just a couple details
I'm trying to think if there would be a case when a contrast wouldn't be included in the actual meta-analysis (e.g., didn't have the correct images for that particular algorithm, or was otherwise filtered from the analysis), but the re-weighing would correct the values as though that contrast was included.
nimare/meta/ibma.py
Outdated
@@ -320,6 +320,9 @@ class Stouffers(IBMAEstimator): | |||
Whether to use sample sizes for weights (i.e., "weighted Stouffer's") or not, | |||
as described in :footcite:t:`zaykin2011optimally`. | |||
Default is False. | |||
use_group_size : :obj:`bool`, optional |
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'm having second thoughts about use_group_size
, group may be more ambiguous than what we are using the variable for. group could mean:
- patient/control group
- a group of papers about shizophrenia versus depression
whereas we are using group to mean specifically the number of analyses (contrasts) within a study.
use_group_size
could be renamed to normalize_contrast_weights
and group_size
to num_contrasts
and group
to contrast_names
I think that is more aligned with what the variables are doing.
unless the labels serve more functions than identifying contrasts.
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 100% agree! I will make the changes in a bit.
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'm trying to think if there would be a case when a contrast wouldn't be included in the actual meta-analysis (e.g., didn't have the correct images for that particular algorithm, or was otherwise filtered from the analysis), but the re-weighing would correct the values as though that contrast was included.
In the new _preprocess_input
in Stouffers, the number of contrasts is estimated from self.inputs_["id"]
, so any excluded images will also be excluded from the count.
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:
use_group_size
.