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

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

Merged
merged 8 commits into from Oct 26, 2018

Conversation

Projects
None yet
2 participants
@jdkent
Copy link
Contributor

jdkent commented Oct 20, 2018

Changes proposed in this pull request

ref #1136
ref #1300

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...

@@ -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')

This comment has been minimized.

@effigies

effigies Oct 22, 2018

Collaborator

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

This comment has been minimized.

@jdkent

jdkent Oct 23, 2018

Author Contributor

nice, thanks!

@jdkent 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 some commits Oct 23, 2018

[skip ds210][skip ds054][skip tests] switch n_volumes* to skip_vols a…
…nd use nonsteady calculation from bold_reference_wf (review by effigies)

@jdkent jdkent force-pushed the jdkent:remove_init_vol_aroma branch from ae48390 to 11ea1c2 Oct 23, 2018

@jdkent

This comment has been minimized.

Copy link
Contributor Author

jdkent commented Oct 23, 2018

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

@effigies
Copy link
Collaborator

effigies left a comment

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.

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

return workflow


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

This comment has been minimized.

@effigies

effigies Oct 23, 2018

Collaborator

Over-indented.

# 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 comment has been minimized.

@effigies

effigies Oct 23, 2018

Collaborator

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
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

This comment has been minimized.

@effigies

effigies Oct 23, 2018

Collaborator

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.

import nibabel as nb
from nipype.utils.filemanip import fname_presuffix

# load the bold file and get the 4d matrix

This comment has been minimized.

@effigies

effigies Oct 23, 2018

Collaborator

Before doing anything else, I would check:

if skip_vols == 0:
    return bold_file
import nibabel as nb
from nipype.utils.filemanip import fname_presuffix

# load the data

This comment has been minimized.

@effigies

effigies Oct 23, 2018

Collaborator
if skip_vols == 0:
    return bold_cut_file

@effigies effigies modified the milestone: 1.1.0 Oct 24, 2018

@effigies

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 24, 2018

@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?

@jdkent

This comment has been minimized.

Copy link
Contributor Author

jdkent commented Oct 24, 2018

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).

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.

This comment has been minimized.

@effigies

effigies Oct 24, 2018

Collaborator

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

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 24, 2018

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 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

This comment has been minimized.

Copy link
Collaborator

effigies commented Oct 26, 2018

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

@effigies effigies merged commit aa56aaa into poldracklab:master Oct 26, 2018

11 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/circleci: ds005 Your tests passed on CircleCI!
Details
ci/circleci: ds054 Your tests passed on CircleCI!
Details
ci/circleci: ds210 Your tests passed on CircleCI!
Details
ci/circleci: get_data Your tests passed on CircleCI!
Details
ci/circleci: get_regression_data Your tests passed on CircleCI!
Details
ci/circleci: test_pytest Your tests passed on CircleCI!
Details
ci/circleci: update_cache Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jdkent

This comment has been minimized.

Copy link
Contributor Author

jdkent commented Oct 26, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment