-
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
[ENH] Improve convergence between ALE null methods #411
Conversation
Apparently, the updated version is too memory-intensive for CI, which makes a sad sort of sense.
Codecov Report
@@ Coverage Diff @@
## master #411 +/- ##
=======================================
Coverage 82.32% 82.32%
=======================================
Files 40 40
Lines 3848 3848
=======================================
Hits 3168 3168
Misses 680 680
Continue to review full report at Codecov.
|
By building our bins from the MA values after rounding up, we should no longer need a buffer.
nimare/meta/cbma/ale.py
Outdated
|
||
ma_hists = np.zeros((ma_values.shape[0], hist_bins.shape[0])) | ||
# create bin centers, then shift them into bin edges | ||
hist_bins = np.round(np.arange(0, max_poss_ale + (1.5 * step_size), step_size), 5) - ( |
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.
The 1.5 should result in the maximum possible statistic value falling within the last bin, with no trailing bins after that.
The only failing test is |
the error is coming from this line
the error occurs when This code fixes the immediate problem, I'll take a look at the code to see if there is something else strange going on:
EDIT I think the current behavior in master is strange and this pull request fixes it, currently when using ALE with the mkda kernel The bin containing 1 has a relatively (compared to using an ALE kernel) high probability meaning if we apply a threshold of 0.05, Still have some thinking to do on how to handle this scenerio. |
Thank you for digging into this! It seems like the issue, then, is that the maximum possible value for an MKDA kernel + the ALE summary statistic formula (i.e., 1) is fairly highly probable, while any summary statistic value above that is impossible, and thus p-values associated with those higher values drop down to zero. By not including impossibly high values in the histogram, I was removing these p=0 locations. One solution that seems to work is to simply include one extra bin when I create the histogram bins. Thus, the last bin's p-value will always be zero (since it exceeds the maximum possible summary statistic value), and when we grab the bin before the identified one, we will grab the maximum possible value's bin. So for ALE + MKDA, we might have the following histogram bins and associated weights: bins = [0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1.0, 1.1]
null = [1.0, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.3, 0.0] If that extra bin (1.1) isn't included, then we have a minimum p-value of 0.3, which will trigger the bug, as you discovered. The extra bin gets us our "low" p-value, although we end up with an associated summary statistic of 1.0 instead of 1.1, based on how |
I think the extra bin solution works and makes sense, so I'm going to merge this. |
Closes #396.
Changes proposed in this pull request:
Also increase resolution of SCALE analytic null histogram by same factor.Reverted since it was too memory-intensive for the CI.