Skip to content

[ENH] Respect SliceEncodingDirection metadata #1350

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

Merged
merged 7 commits into from
Nov 1, 2018
Merged

Conversation

fliem
Copy link
Contributor

@fliem fliem commented Oct 25, 2018

Changes proposed in this pull request

closes #1345
adds slice encoding direction to stc

depends on nipype PR#2753

Documentation that should be reviewed

Not sure...

@welcome
Copy link

welcome bot commented Oct 25, 2018

Thanks for opening this pull request! We have detected this is the first time for you to contribute to fMRIPrep. Please check out our contributing guidelines.
We invite you to list yourself as a fMRIPrep contributor, so if your name is not already mentioned, please modify the .zenodo.json file with your data right above Russ' entry. Example:

{
   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
},
{
   "name": "Poldrack, Russell A.",
   "affiliation": "Department of Psychology, Stanford University",
   "orcid": "0000-0001-6755-0259"
},

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

@fliem
Copy link
Contributor Author

fliem commented Oct 27, 2018

@effigies thanks for merging the nipype PR. I can't figure out how to get the newest nipype version into fmriprep. Via niworkflows?

@effigies
Copy link
Member

Once the new nipype is released, we'll incorporate the fix here. We're waiting on one fix in nipype before release (scheduled for Monday anyway).

@effigies
Copy link
Member

Although, you can pin to the latest nipype commit in requirements.txt, for testing purposes:

git+https://github.com/nipy/nipype.git@d83b06d7f8ddb76d4e1033c149eecc966eddb514#egg=nipype

@effigies
Copy link
Member

If you update the nipype dependency to >=1.1.4, I think this should be nearly ready.

https://github.com/poldracklab/fmriprep/blob/92786d1456c4098197f6a61e46fde7275f891777/fmriprep/__about__.py#L87-L108

@fliem
Copy link
Contributor Author

fliem commented Nov 1, 2018

Thanks. I am still running some tests. I'll get back to you when they are done.

@@ -71,6 +71,8 @@ def init_bold_stc_wf(metadata, name='bold_stc_wf'):
tr='{}s'.format(metadata["RepetitionTime"]),
slice_timing=metadata['SliceTiming']),
name='slice_timing_correction')
if 'SliceEncodingDirection' in metadata:
slice_timing_correction.inputs.slice_encoding_direction = metadata['SliceEncodingDirection']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could be as clear (and not hit line length limits) by just adding slice_encoding_direction=metadata.get('SliceEncodingDirection', 'k') to the TShift() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I have tested the previous version and this looks good

  • same list order, different direction gives different results (as expected)
  • flipped list, flipped direction gives same results (as expected)
  • no direction vs "k" gives same results (as expected)

@effigies
Copy link
Member

effigies commented Nov 1, 2018

LGTM. I merged master in, as there've been some updates to the tests and CI config. If this tests clean, I'll merge right away.

@effigies effigies changed the title ENH: added SliceEncodingDirection to stc [ENH] Respect SliceEncodingDirection metadata Nov 1, 2018
@effigies
Copy link
Member

effigies commented Nov 1, 2018

Thanks!

@effigies effigies merged commit 12df369 into nipreps:master Nov 1, 2018
@fliem fliem deleted the fix-stc branch November 2, 2018 13:38
@effigies effigies added this to the 1.2.2 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STC ignoring SliceEncodingDirection
3 participants