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

[RTM] slice timing correction #415

Merged
merged 18 commits into from Mar 30, 2017

Conversation

Projects
None yet
3 participants
@chrisfilo
Contributor

chrisfilo commented Mar 28, 2017

  • Adds slice time correction if metadata preset (can be disabled, closes #27)
  • Refactors the basic workflow to allow for different workflows per each bold file depending on metadata
  • Removes redundant avscale (it was producing the same results as mcflirts' .par file)

This implementation uses a two step approach using AFNI. There were big discrepancies in estimated motion between nipy and FSL so I decided to avoid 4dRealign until this was figured out.

@oesteban

Just had a very quick look on the changes related to the motion parameters. I will need more time to understand the whole thing 👍

Show outdated Hide outdated fmriprep/info.py
Show outdated Hide outdated fmriprep/workflows/base.py
@@ -39,7 +38,7 @@ def discover_wf(settings, name="ConfoundDiscoverer"):
dvars.interface.estimated_memory_gb = settings[
"biggest_epi_file_size_gb"] * 3
# Frame displacement
frame_displace = pe.Node(confounds.FramewiseDisplacement(parameter_source="FSL"),
frame_displace = pe.Node(confounds.FramewiseDisplacement(parameter_source="SPM"),

This comment has been minimized.

@oesteban

oesteban Mar 28, 2017

Contributor

Just to confirm: this parameter source is compatible with ITK matrices right?

@oesteban

oesteban Mar 28, 2017

Contributor

Just to confirm: this parameter source is compatible with ITK matrices right?

This comment has been minimized.

@oesteban

oesteban Mar 28, 2017

Contributor

Actually FSL? Are we sure angles are in radians?

@oesteban

oesteban Mar 28, 2017

Contributor

Actually FSL? Are we sure angles are in radians?

This comment has been minimized.

@chrisfilo

chrisfilo Mar 28, 2017

Contributor

This interface is never fed anything that used to be ITK matrices - so the answer is no.

"FSL" and "SPM" differ only in order of parameters - it should not influence the FD calculations, but since we are conforming all motion parameters to SPM style I did this for consistency.

@chrisfilo

chrisfilo Mar 28, 2017

Contributor

This interface is never fed anything that used to be ITK matrices - so the answer is no.

"FSL" and "SPM" differ only in order of parameters - it should not influence the FD calculations, but since we are conforming all motion parameters to SPM style I did this for consistency.

@chrisfilo

This comment has been minimized.

Show comment
Hide comment
@chrisfilo

chrisfilo Mar 28, 2017

Contributor

I'm still planning on adding some comments and do some refactoring before this is ready for review.

I also need to verify everything is ok with the motion parameters.

Contributor

chrisfilo commented Mar 28, 2017

I'm still planning on adding some comments and do some refactoring before this is ready for review.

I also need to verify everything is ok with the motion parameters.

@chrisfilo

This comment has been minimized.

Show comment
Hide comment
@chrisfilo

chrisfilo Mar 28, 2017

Contributor

The motion parameters are roughly similar, but not identical. Not sure how much difference I should expect judging how different this procedure is. This is FD

image

Contributor

chrisfilo commented Mar 28, 2017

The motion parameters are roughly similar, but not identical. Not sure how much difference I should expect judging how different this procedure is. This is FD

image

@chrisfilo chrisfilo changed the title from [WIP] slice timing correction to [RTM] slice timing correction Mar 30, 2017

@effigies

This all looks reasonable.

@@ -248,7 +374,7 @@ def apply_fs_transform(fs_2_t1_transform, bbreg_transform):
(inputnode, ds_t1w, [(('name_source', _first), 'source_file')]),
(inputnode, ds_t1w_mask,
[(('name_source', _first), 'source_file')]),
(merge, ds_t1w, [('merged_file', 'in_file')]),
(outputnode, ds_t1w, [('epi_t1', 'in_file')]),

This comment has been minimized.

@effigies

effigies Mar 30, 2017

Collaborator

This seems to be a null change. Why use outputnode as the source? Just to move the DerivativesDataSink to the bottom of the graph?

@effigies

effigies Mar 30, 2017

Collaborator

This seems to be a null change. Why use outputnode as the source? Just to move the DerivativesDataSink to the bottom of the graph?

@chrisfilo

This comment has been minimized.

Show comment
Hide comment
@chrisfilo

chrisfilo Mar 30, 2017

Contributor
Contributor

chrisfilo commented Mar 30, 2017

@effigies

This comment has been minimized.

Show comment
Hide comment
@effigies

effigies Mar 30, 2017

Collaborator

Fair.

Actually, I guess one more question: Is there a good report we can add to verify STC function?

Collaborator

effigies commented Mar 30, 2017

Fair.

Actually, I guess one more question: Is there a good report we can add to verify STC function?

@chrisfilo

This comment has been minimized.

Show comment
Hide comment
@chrisfilo

chrisfilo Mar 30, 2017

Contributor
Contributor

chrisfilo commented Mar 30, 2017

@chrisfilo chrisfilo merged commit 3ccda66 into poldracklab:master Mar 30, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment