-
Notifications
You must be signed in to change notification settings - Fork 303
WIP: Adding ICA_AROMA functionality #539
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
Hey. This is a great contribution. In fact we planned to do this - see #334 (comment) so I am really happy to see your contribution! I prefer reusing rather than reinventing so using the Nipype interfaces as you are doing is preferable to hijacking the code. A few things I would suggest:
Things worth considering, but potentially a separate PR:
Once again - thanks for this PR! |
Thanks you for your comments! Would the template: MNI152NLin2009cAsym always have to be specified to run ICA_AROMA, since it looks like it's currently not mandatory.
|
On Sat, May 27, 2017 at 5:33 PM, James Kent ***@***.***> wrote:
Would the template: MNI152NLin2009cAsym always have to be specified to run
ICA_AROMA, since it looks like it's currently not mandatory.
Currently ANTs normalization is only performed if "template" is specified
as one of output spaces (which is the default). I would change this to "or
ICA AROMA is required".
-
I can see about reporting the good/bad components, but I will at least
run melodic separately to generate the report.
Great!
-
aggressive v. non-aggressive. This is completed via fsl_regfilt, where
the non-aggressive denoising is completed through partial regression (e.g.
variance shared with the other components is kept in the data) versus full
regression (e.g. the shared variance is removed from the data). So I would
want to return the coefficients from the partial regression, I think.
fsl_regfilt does not return the coefficients sadly, so I would need to
calculate that myself, perhaps with this?
<http://scikit-learn.org/stable/modules/generated/sklearn.cross_decomposition.PLSRegression.html>
I don't think we need to run fsl_regfilt for this. The aggressive
regressors would be just the timecourses of the "bad" ICs. The
non-agressive regressors would be the timecourses of "bad" ICs after
regressing out timecourses of "good" ICs. The main difference is that the
non-agressive regressors do not share any variance with timecourses of
"good" ICs. This should be pretty straightforward to do, but we can leave
it to a separate PR.
-
It would be easier just to pass in the epi_mni and forgo the
transforms, is this less recommended? I guess I'm less sure about whether
the ouputs are organized so we can pass one affine and one warp if there
are fieldmaps in the processing stream.
I think ICA AROMA has a mode where you do not pass any transformations and
the inputs are just assumed to be in the MNI space. We should use that.
…
-
I will look into the published smoothing method
Thanks!
|
I'm still not done with this. Things I still need to work on:
Just making an update to keep consistent with working on the code/learn the nipype framework. |
On Wed, May 31, 2017 at 8:35 PM, James Kent ***@***.***> wrote:
I'm still not done with this. Things I still need to work on:
-
ica aroma partial regression: I've given it some effort, but I can't
replicate the results from fsl_regfilt for non-aggressive denoising. My
attempt followed this line of reasoning.
linear regression: y_hat = mx +b, so the residual (or partial) after
regression should be y - y_hat, right? I can send a link to some example
data if what I did wasn't a simple mathematical error.
That sounds correct. Are you including all "good" IC in the right hand
side of the equation? We can also leave it to another PR.
-
I still need to work on the melodicRPT, I'll continue reading up on
that to add that functionality.
Let me know if you have any questions. Here's an example how this can be
…
-
clean up the code and get formatting consistent. I still don't have a
good sense of what should be a method, a node, and a workflow, and I saw in
a tutorial that some simple mathematical operations (like number output
from ImageStats) can be modified before being passed into another node.
Just making an update to keep consistent with working on the code/learn
the nipype framework.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#539 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp_fKsvtldaYlP8yzhlF24MzCUJ-Iks5r_jGQgaJpZM4NoPBt>
.
|
RE: regression
Where y is the "bad" motion IC, and x is a matrix of the good_ICs. Maybe will leave it to a separate PR if I can't figure it out soon. I asked on the FSL listserve to possibly get an answer |
Great move!
…On Thu, Jun 1, 2017 at 1:35 PM, James Kent ***@***.***> wrote:
RE: regression
here is the relevant bit of code:
def calc_residuals(x,y):
X = np.column_stack(x+[[1]*len(x[0])])
beta_hat = np.linalg.lstsq(X,y)[0]
y_hat = np.dot(X,beta_hat)
residuals = y - y_hat
return residuals
Where y is the "bad" motion IC, and x is a matrix of the good_ICs. Maybe
will leave it to a separate PR if I can't figure it out soon. I asked on
the FSL listserve
<https://www.jiscmail.ac.uk/cgi-bin/webadmin?A2=FSL;719d1657.1706> to
possibly get an answer
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#539 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp60uj1IVUf55hnVDAMiq-BL7knxIks5r_yCKgaJpZM4NoPBt>
.
|
Keeping up with the Kardashians
Okay, I'm getting closer to making this a reality, there are still several TODOs before it's ready for a formal review:
^it looks like a simple error but I can't find the solution right now.
Good news: within a docker container, I have it working (albiet the melodic report error above), but the confounds tsv file is generated with the expected output (the partial regression implemented here is not identical to fsl_regfilt's partial regression, but it's close) |
Great progress! I had a look at the code, but unfortunately I cannot see what could be wrong with the way you use MELODICRPT. Things worth checking:
|
fmriprep/workflows/confounds.py
Outdated
|
||
from niworkflows.nipype.interfaces.nilearn import SignalExtraction | ||
from fmriprep.interfaces.utils import prepare_roi_from_probtissue | ||
|
||
|
||
def init_discover_wf(bold_file_size_gb, name="discover_wf"): | ||
def init_discover_wf(bold_file_size_gb, name="discover_wf",use_aroma=False): |
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.
use_aroma
should not have a default set here. This might be the core of the error you are getting.
fmriprep/workflows/confounds.py
Outdated
[('aroma_confounds','aroma_confounds')]), | ||
#TODO change melodic report to reflect noise and non-noise components | ||
(melodic, outputnode, | ||
[('out_report','ica_aroma_report')]), |
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.
ica_aroma_report
=> out_report
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 catch!
Thanks @chrisfilo and @effigies, you guys are rockstars! That fixed the report error I was getting in the workflow. Sorry that re-running takes forever, when nipy/nipype#2056 is implemented, debugging will be way faster (I think). |
Keeping up with the kardashians
Okay. Let me try to summarize the situation. As I think I've said, I've never used MELODIC or AROMA; hopefully I understand roughly what's going on, but I recognize what follows is likely to be laughably simplistic. Please correct me if it's also wrong.
If that's basically right, then I think your strategy seems reasonable. I believe the BIDS preference is always for TSV, not CSV, so I'd use TSV for both of your files. We should also avoid
Or something like that? Since the MELODIC output is actually not specific to AROMA post-processing, I don't think we need to label it with both AROMA and MELODIC. I'm open to alternative capitalization schemes. |
Looks like you got it right for points 1 and 2, and I'm not sure if I have it right for point 3, but I'll try explaining what I think I know more.
I also believe the preference is for tsv, but this is another special case. I include the noise IC classification as a csv because fsl_regfilt does not accept tab separated values with it's "-f" flag. However, fsl_regfilt does not tell you it can't read the .tsv, instead it only filters the first index/number in the list. Making the AROMAnoiseICs.tsv could lead to misuse on the user end if they try to pass the tsv directly to fsl_regfilt without knowing that quirk. ugh, that is annoying. ^^clarification of fsl_regfilt, it does not directly read in a file for the -f flag, rather you cat the contents of the file following the -f flag like so: |
Alright. I think in any case, it's reasonable to go ahead in this PR and not produce the non-aggressive regressors, and just add documentation. If future investigation shows we can do it for users without producing a column per voxel, that can be another PR. (Incidentally, if it would end up being a column per-voxel, it probably makes more sense to output it as a Thanks for the context on the CSV issue. That makes sense and let's definitely go ahead with your plan, there. |
Figured it out by saying it out loud, rubber duck debugging works again |
#572 fixed it |
@effigies @chrisfilo Question: what software do you use to preview the *rst files in docs? I tried retest for ubuntu, but my first impression is disappointing. Again thank you for taking the time to help me through this journey, I believe the code is ready barring documentation formatting and any further suggestions you have. |
python setup.py build_sphinx Generally run it in a docker container, because it's pretty touchy. |
And I'll have a thorough look through (probably Monday). Thanks so much for all the effort you've put into this. |
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.
Alright. Review time.
Looks good! Lots of little comments and a couple organizational issues, but no substantial code changes that I see.
Also:
- Can you add the ICA-AROMA dependency and version to
docs/installation.rst
?
docs/workflows.rst
Outdated
@@ -320,15 +324,29 @@ Confounds estimation | |||
|
|||
from fmriprep.workflows.confounds import init_discover_wf | |||
wf = init_discover_wf(name="discover_wf", | |||
use_aroma=False,ignore_aroma_err=False, |
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.
Space after first comma.
fmriprep/cli/run.py
Outdated
|
||
# ICA_AROMA options | ||
g_aroma = parser.add_argument_group('Specific options for running ICA_AROMA') | ||
g_aroma.add_argument('--use_aroma', action='store_true', default=False, |
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.
Let's switch this to --use-aroma
? This won't change the variable name, so it's a lightweight change. The only other places to update it are in the docs where you talk about the flag.
docs/workflows.rst
Outdated
bold_file_size_gb=3) | ||
|
||
Given a motion-corrected fMRI, a brain mask, MCFLIRT movement parameters and a | ||
segmentation, the `discover_wf` sub-workflow calculates potential | ||
confounds per volume. | ||
Optional (--use_aroma): give a motion corrected fMRI in MNI space, and mask in MNI space, | ||
the `discover_wf` sub-workflow calculates potential motion related | ||
independent components using ICA_AROMA |
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 think this addition can be discarded. The next section explains the change in outputs.
docs/workflows.rst
Outdated
bold_file_size_gb=3) | ||
|
||
Given a motion-corrected fMRI, a brain mask, MCFLIRT movement parameters and a | ||
segmentation, the `discover_wf` sub-workflow calculates potential | ||
confounds per volume. | ||
Optional (--use_aroma): give a motion corrected fMRI in MNI space, and mask in MNI space, | ||
the `discover_wf` sub-workflow calculates potential motion related | ||
independent components using ICA_AROMA | ||
|
||
Calculated confounds include the mean global signal, mean tissue class signal, | ||
tCompCor, aCompCor, Framewise Displacement, 6 motion parameters and DVARS. |
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.
Change to:
... 6 motion parameters, DVARS, and, if the ``--use-aroma`` flag is enabled, the noise
components identified by ICA-AROMA (those to be removed by the "aggressive" denoising strategy).
docs/workflows.rst
Outdated
``fsl_regfilt -i <BIDS SUBJECT>_task-<TASK>_bold_space-<SPACE>_preproc.nii.gz | ||
-f $(cat <BIDS SUBJECT>_task-<TASK>_bold_AROMAnoiseICs.csv) | ||
-d <BIDS SUBJECT>_task-<TASK>_bold_MELODICmix.tsv | ||
-o <BIDS SUBJECT>_task-<TASK>_bold_space-<SPACE>_AromaNonAggressiveDenoised.nii.gz`` |
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'd like to rephrase this whole section a bit just to improve the narrative flow a little. Think of the following as a rough template, and feel free to tweak it further:
*Note*: Confounds for performing *non*-aggressive denoising cannot be generated in FMRIPREP.
If the ``--use-aroma`` flag is passed to FMRIPREP, the noise components and MELODIC mix will
be generated, and non-aggressive denoising may be performed with ``fsl_regfilt``, *e.g.*::
fsl_regfilt -i sub-<subject_label>_task-<task_id>_bold_space-<space>_preproc.nii.gz \
-f $(cat sub-<subject_label>_task-<task_id>_bold_AROMAnoiseICs.csv) \
-d sub-<subject_label>_task-<task_id>_bold_MELODICmix.tsv \
-o sub-<subject_label>_task-<task_id>_bold_space-<space>_AromaNonAggressiveDenoised.nii.gz
It may be worth linking to any online documentation that you think appropriate for fsl_regfilt
, and this can include mailing list threads if they give better context. This is a judgment call; if you don't think it'll be helpful, skip it.
fmriprep/workflows/confounds.py
Outdated
('melodic_mix', 'melodic_mix')]), | ||
# TODO change melodic report to reflect noise and non-noise components | ||
(melodic, outputnode, | ||
[('out_report', 'out_report')]), |
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.
A lot of these connections can be reduced to one or two lines. I find this extremely hard to read as is.
fmriprep/workflows/epi.py
Outdated
debug, output_grid_ref, layout=None): | ||
fmap_bspline, fmap_demean, debug, output_grid_ref, | ||
use_aroma, ignore_aroma_err, | ||
use_syn, force_syn, layout=None): |
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.
Just to reduce the diff to a single addition, rather than 2-/4+.
fmap_bspline, fmap_demean, use_syn, force_syn,
use_aroma, ignore_aroma_err,
debug, output_grid_ref, layout=None):
fmriprep/workflows/epi.py
Outdated
@@ -142,11 +150,11 @@ def init_func_preproc_wf(bold_file, ignore, freesurfer, | |||
('subject_id', 'inputnode.subject_id'), | |||
('fs_2_t1_transform', 'inputnode.fs_2_t1_transform') | |||
]), | |||
(inputnode, discover_wf, [('t1_tpms', 'inputnode.t1_tpms')]), | |||
(inputnode, discover_wf, [('t1_tpms', 'inputnode.t1_tpms'), | |||
]), |
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.
Let's revert this newline.
fmriprep/workflows/epi.py
Outdated
(discover_wf, func_derivatives_wf, [ | ||
('outputnode.aroma_noise_ics', 'inputnode.aroma_noise_ics'), | ||
('outputnode.melodic_mix', 'inputnode.melodic_mix')]), | ||
]) |
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.
Our current pattern is to pass anything that needs to go to derivatives to outputnode
, and then pass the outputnode
fields to func_derivatives_wf.inputnode
. The reasoning is basically that if it's a derivative, it's something another workflow should reasonably expect access to.
fmriprep/workflows/epi.py
Outdated
workflow.connect([ | ||
(inputnode, ds_ica_aroma_report, [('source_file', 'source_file'), | ||
('ica_aroma_report', 'in_file')]), | ||
]) |
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 close brace should be aligned with the start of the tuple. e.g.
if x:
wf.connect([
(node1, node2, [(...)]),
])
.circle/tests.sh
Outdated
@@ -43,7 +43,7 @@ case ${CIRCLE_NODE_INDEX} in | |||
| tee $HOME/docs/builddocs.log | |||
cat $HOME/docs/builddocs.log && if grep -q "ERROR" $HOME/docs/builddocs.log; then false; else true; fi | |||
fmriprep-docker -i poldracklab/fmriprep:latest --help | |||
fmriprep-docker -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds005/scratch $HOME/data/ds005 $HOME/ds005/out participant --output-space fsaverage5 --debug --write-graph | |||
fmriprep-docker -i poldracklab/fmriprep:latest --config $HOME/nipype.cfg -w $HOME/ds005/scratch $HOME/data/ds005 $HOME/ds005/out participant --debug --write-graph --use_aroma --ignore-aroma-denoising-errors |
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.
Need to update --use-aroma
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.
oops, thanks!
@jdkent Thanks so much for spending a month on this. This was a huge contribution. |
Thank you! It's great to see this merged!
…On Jun 28, 2017 1:08 PM, "Chris Markiewicz" ***@***.***> wrote:
@jdkent <https://github.com/jdkent> Thanks so much for spending a month
on this. This was a huge contribution.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#539 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp1XK3Oi2t8l3NfWEQZSXcpHQRVR1ks5sIrKxgaJpZM4NoPBt>
.
|
@effigies @chrisfilo This was an amazing learning experience for me and I can't emphasize enough how thankful I am for you two taking the time to guide me through this contribution. I plan to keep contributing to fmriprep and projects like it (hopefully I'll need less help and be able to help others as I learn more about nipype and python). |
This is my attempt at adding ICA_AROMA functionality to fmriprep. Almost 100% certain it doesn't work as is.
I have ICA_AROMA completing it's own transforms since it appears the template space could be something something different than ICA AROMA is expecting, in addition, ICA_AROMA requires a affine fsl matrix and fnirt fsl warp to pass in so ANTs is off the table (afaik).
I may continue editing/working on this, but first I wanted feedback if this is a worthwhile endeavour or if it's better to hi-jack the original code from ICA-AROMA to make it flexibly fit in the fmriprep workflows (as was done with compcor).
Thanks!
James