-
Notifications
You must be signed in to change notification settings - Fork 15
[ENH] add roi based GLM using mars bar #340
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
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #340 +/- ##
==========================================
- Coverage 63.33% 61.35% -1.99%
==========================================
Files 111 114 +3
Lines 1762 1832 +70
==========================================
+ Hits 1116 1124 +8
- Misses 646 708 +62
Continue to review full report at Codecov.
|
|
maybe the options mentioned above might be of interest to you. |
CerenB
left a comment
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.
nice! I left some questions to get confirmation from you. And I was wondering this structure of the folders will be applied to the whole-brain analysis, it is not only for ROIs, right?
e.g. if I opt to use no ROIs, and change the resolution in the analysis, I can re-run the spatial preprocessing script and the pipeline would create a new folder (actually new folder structure)
|
|
||
| bidsCreateROI(opt); | ||
|
|
||
| opt.glm.roibased.do = true; |
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.
if this option is set to false, would it do the whole brain analysis with different resolution spatial preprocessing?
| opt.model.file = newModel; | ||
|
|
||
| spm_jsonwrite(newModel, content, struct('indent', ' ')); | ||
|
|
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 is only for this function to create names (in order to do so, we need to create different model.json files) for different resolution? so that spatial preprocssing folders would be different?
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.
Yeah this is only to create a different GLM folder name.
This is a bit "ugly" because everything in those json is the same: but I was trying to find a way to create different folder names that would not involve adding another option to the pile and would instead reuse something we already have to create GLM folder names.
Does that make sense?
Another option could have been to have a different cpp_spm-stats folder for each resolution but then all the stats would not be centralized.
I think there is only so much data curation that can be automatized.
| 'stats', ... | ||
| glmDirName); | ||
|
|
||
| if opt.glm.roibased.do |
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.
neat!
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.
Yeah that's pretty much it: one of the few thing this flag does is append a suffix to the GLM folder.
This way in this case you can use the same model json for both whole brain and roi based analysis
| @@ -0,0 +1,115 @@ | |||
| % (C) Copyright 2021 CPP BIDS SPM-pipeline developers | |||
|
|
|||
| function bidsRoiBasedGLM(opt) | |||
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 function is for running GLM on only ROIs, right? instead of whole brain. Here is a irrelevant question!
I usually do not prefer roi-based GLM, and go with whole brain. Then extract the betas from a given ROI. Do you know which functions could be useful for extracting betas in a given ROI in cpp-spm?
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.
spm_summarize ?
Remi-Gau
left a comment
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.
And I was wondering this structure of the folders will be applied to the whole-brain analysis, it is not only for ROIs, right?
Yes that's correct
|
I was just thinking. Maybe we should add some "child protection": if And then the other way around when set to false. What do you think? Maybe we should throw an error with a message saying what the expected option setting should be for each workflow to run. |
… wrong options AKA child protection
|
One thing that will need to be done in another PR is the ability to compute contrasts from the bids model and the content of the options. At the moment the roi based workflow only does it assuming for all "events" taken together. |
Other changes:
change the option to skip GLM QA:
opt.glm.QA.doit is possible to decide where the GLM output will go by using
opt.dir.stats. The default is now:cpp_spm-statshere is what the output structure could look like