[FIX] Remove non-steady-state volumes prior to ICA-AROMA #1335

jdkent · 2018-10-20T01:59:22Z

Changes proposed in this pull request

This pull request pulls the steady state detector into bold/base.py and uses it's output for both the confounds workflow and the aroma workflow.

Documentation that should be reviewed

I need to update the documentation for ica-aroma, more details to come...

…y state volumes from aroma calculation

… remove_init_vol_aroma

effigies · 2018-10-22T15:31:21Z

fmriprep/workflows/bold/base.py

@@ -424,6 +425,9 @@ def init_func_preproc_wf(bold_file, ignore, freesurfer,
                                             omp_nthreads=omp_nthreads,
                                             use_compression=False)

+    # get non steady-state volumes
+    non_steady_state = pe.Node(nac.NonSteadyStateDetector(), name='non_steady_state')


You should already be able to get this from bold_reference_wf.outputnode.skip_vols.

nice, thanks!

…nd use nonsteady calculation from bold_reference_wf (review by effigies)

jdkent · 2018-10-23T02:37:42Z

If I wanted to write tests for the semi-private functions, where should I put them? Should I test them?

effigies

This overall looks good. I left some comments on these functions.

I think testing is a good idea. The rule for tests is: if the function you're testing is in a/b/c.py, the test should be in a/b/tests/test_c.py. So let's put them in fmriprep/workflows/bold/tests/test_confounds.py.

effigies · 2018-10-23T01:55:27Z

fmriprep/workflows/bold/confounds.py

        (ica_aroma, ds_report_ica_aroma, [('out_report', 'in_file')]),
    ])

    return workflow


+def _remove_volumes(bold_file, skip_vols):
+        import nibabel as nb


Over-indented.

effigies · 2018-10-23T02:22:01Z

fmriprep/workflows/bold/confounds.py

+        # save the resulting bold file
+        out = fname_presuffix(bold_file, suffix='_cut')
+        bold_img.__class__(bold_data_cut, bold_img.affine, bold_img.header).to_filename(out)
+        return out


This could be much shorter:

import nibabel as nb from nipype.utils.filemanip import fname_presuffix out = fname_presuffix(bold_file, suffix='_cut') bold_img = nb.load(bold_file) bold_img.__class__(bold_img.dataobj[..., skip_vols:], bold_img.affine, bold_img.header).to_filename(out) return out

Note that the data shape will be set automatically, so there's no need to manually update it. And using the dataobj should save as much memory as you can, though I don't think there's much waste here, assuming skip_vols is small.

Or, with nibabel>=2.3:

import nibabel as nb from nipype.utils.filemanip import fname_presuffix out = fname_presuffix(bold_file, suffix='_cut') nb.load(bold_file).slicer[..., skip_vols:].to_filename(out) return out

effigies · 2018-10-23T02:39:41Z

fmriprep/workflows/bold/confounds.py

+    bold_cut_data = bold_cut_img.get_data()
+
+    # assign everything from skip_vols foward to bold_cut_data
+    bold_data[..., skip_vols:] = bold_cut_data


What about:

bold_img = nb.load(bold_file) bold_cut_img = nb.load(bold_cut_file) bold_data = np.concatenate((bold_img.dataobj[..., :skip_vols], bold_cut_img.dataobj), axis=3)

This would reduce the memory usage by the uncompressed size of bold_cut_img.

Or, if we're willing to require nibabel>=2.3:

new_img = nb.concat_images((bold_img.slicer[..., :skip_vols], bold_cut_img), axis=3)

This last has the advantage of checking affines to ensure the images are in the same space.

effigies · 2018-10-23T02:42:26Z

fmriprep/workflows/bold/confounds.py

+        import nibabel as nb
+        from nipype.utils.filemanip import fname_presuffix
+
+        # load the bold file and get the 4d matrix


Before doing anything else, I would check:

if skip_vols == 0: return bold_file

effigies · 2018-10-23T02:43:33Z

fmriprep/workflows/bold/confounds.py

+    import nibabel as nb
+    from nipype.utils.filemanip import fname_presuffix
+
+    # load the data


if skip_vols == 0: return bold_cut_file

effigies · 2018-10-24T03:49:05Z

@jdkent We're trying to update to be largely conformant with BIDS Derivatives RC2 (due out Friday-ish). That will trigger fMRIPrep 1.2.0. Do you want to get this in by then, or do you need more time?

…eview

jdkent · 2018-10-24T17:37:25Z

Hi @effigies, Thanks for the helpful review! I've integrated the changes you suggested in the last commit, but I have not written tests yet. I think I will be able generate tests by friday, but if not, I can make it a separate pull request, and have this one merged as is (if all the tests pass).

effigies · 2018-10-24T17:47:02Z

fmriprep/workflows/bold/confounds.py

-            Do not fail on ICA-AROMA errors
-        aroma_melodic_dim: int or None
-            Set the dimensionality of the Melodic ICA decomposition
-            If None, MELODIC automatically estimates dimensionality.


It looks like you sorted these. I would keep these in the order that they are in the parameter list. If we want to reorder that, we can do that in another PR, but that's an API change, and we should avoid it unless there's a good reason.

effigies · 2018-10-24T17:50:52Z

This looks reasonable to me. I'm okay with making tests a separate PR, if you can't get that in by Friday.

@oesteban WDYT?

effigies · 2018-10-26T13:04:54Z

@jdkent I'm going to go ahead and merge. Please submit tests in a new PR.

jdkent · 2018-10-26T15:25:00Z

Thanks @effigies, I should be able to get those tests in on Monday.

jdkent added 4 commits October 19, 2018 20:54

[skip tests][skip docs][skip ds210][skip ds054] wip: remove non stead…

b3f34a5

…y state volumes from aroma calculation

[skip tests][skip docs][skip ds210][skip ds054] pacify flake8

b4ab300

[skip ds210][skip ds054] update the documentation

921bf56

Merge branch 'master' of https://github.com/poldracklab/fmriprep into…

99f9a23

… remove_init_vol_aroma

effigies reviewed Oct 22, 2018

View reviewed changes

jdkent changed the title ~~[WIP,FIX]: remove non steady-state volumes from aroma calculation~~ [FIX]: remove non steady-state volumes from aroma calculation Oct 22, 2018

jdkent added 2 commits October 22, 2018 21:05

remove whitespace

b9e5ff4

[skip ds210][skip ds054][skip tests] switch n_volumes* to skip_vols a…

11ea1c2

…nd use nonsteady calculation from bold_reference_wf (review by effigies)

jdkent force-pushed the remove_init_vol_aroma branch from ae48390 to 11ea1c2 Compare October 23, 2018 02:06

effigies reviewed Oct 23, 2018

View reviewed changes

effigies modified the milestone: 1.1.0 Oct 24, 2018

[skip ds210][skip ds054][skip tests][skip docs] edits from effigies r…

949b504

…eview

effigies reviewed Oct 24, 2018

View reviewed changes

re-order parameters in docstrings [skip ds210][skip ds054][skip ds005]

a44b9b4

effigies changed the title ~~[FIX]: remove non steady-state volumes from aroma calculation~~ [FIX] Remove non-steady-state volumes prior to ICA-AROMA Oct 26, 2018

effigies merged commit aa56aaa into nipreps:master Oct 26, 2018

jdkent mentioned this pull request Oct 30, 2018

Crash on AROMA if all ICs are classified as motion #1300

Closed

markushs mentioned this pull request Nov 5, 2018

Fmriprep 1.2.1: ica_aroma_wf.rm_nonsteady: skip_vols argument missing #1365

Closed

effigies mentioned this pull request Nov 5, 2018

FIX: Count non-steady-state volumes even if sbref is passed nipreps/niworkflows#258

Merged

jdkent mentioned this pull request Nov 11, 2018

[TEST] Utility functions for skipping/re-inserting non-steady-state volumes #1382

Merged

oesteban mentioned this pull request Nov 15, 2018

fmriprep crashed, seems to be related to ICA aroma #1389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Remove non-steady-state volumes prior to ICA-AROMA #1335

[FIX] Remove non-steady-state volumes prior to ICA-AROMA #1335

jdkent commented Oct 20, 2018

effigies Oct 22, 2018

jdkent Oct 23, 2018

jdkent commented Oct 23, 2018

effigies left a comment

effigies Oct 23, 2018

effigies Oct 23, 2018

effigies Oct 23, 2018

effigies Oct 23, 2018

effigies Oct 23, 2018

effigies commented Oct 24, 2018

jdkent commented Oct 24, 2018

effigies Oct 24, 2018

effigies commented Oct 24, 2018

effigies commented Oct 26, 2018

jdkent commented Oct 26, 2018

[FIX] Remove non-steady-state volumes prior to ICA-AROMA #1335

[FIX] Remove non-steady-state volumes prior to ICA-AROMA #1335

Conversation

jdkent commented Oct 20, 2018

Changes proposed in this pull request

Documentation that should be reviewed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdkent commented Oct 23, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Oct 24, 2018

jdkent commented Oct 24, 2018

Choose a reason for hiding this comment

effigies commented Oct 24, 2018

effigies commented Oct 26, 2018

jdkent commented Oct 26, 2018