-
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 parallelization option to CorrelationDecoder
and CorrelationDistributionDecoder
#738
Conversation
Codecov Report
@@ Coverage Diff @@
## main #738 +/- ##
==========================================
+ Coverage 88.60% 88.61% +0.01%
==========================================
Files 40 40
Lines 4449 4455 +6
==========================================
+ Hits 3942 3948 +6
Misses 507 507
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share 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 would like to run another test before we merge this. The attribute |
Actually, could you also fix the type at: Line 80 in 245b844
There's an extra opening parenthesis that should be removed, but I don't think it's worth a separate PR. |
Sure! |
That would be great! Thanks! |
This reverts commit 936d796.
I don't know why the linter only started complaining in this PR, given that you didn't touch the problematic code, but it definitely is correct. The flagged line is definitely too long: Line 26 in 87c3ce3
|
I tried splitting the summary line into two lines and it also failed the Style check test (see the last commit). Do you think we should shorten the summary sentence? |
That's correct. Summary sentences need to fit on one line. I think you can just drop the word "complementary" and it will fit. |
I think this is ready to merge. I ran a few tests locally and |
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! Feel free to merge.
Closes None.
Currently,
CorrelationDecoder
takes a long time to be trained. Depending on the number of studies per feature, it takes around 13 min per feature (i.e., per iteration). For large databases like Neurosynth, which has 3228 features, it will take ~29 days. Adding the parallelization option reduces the computation time by a factor ofn_cores
.Changes proposed in this pull request:
n_cores
parameter to use for parallelization.