[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

jguillon · 2017-03-23T18:15:45Z

Added:

VDM Calculation pre-substracted phase case (SIEMENS machines)

Todo:

Support VDM Application
Support different magnitude and phase file types

Added: - VDM Calculation pre-substracted phase case (SIEMENS machines) Todo: - Support VDM Application - Support different magnitude and phase file types

oesteban

My two cents, however I'm not very familiar with SPM. Someone more knowledgeable should look at this

oesteban · 2017-04-29T08:58:02Z

nipype/interfaces/spm/preprocess.py

+class FieldMap(SPMCommand):
+    """Use spm to calculate fieldmap vdm.
+
+    http://www.fil.ion.ucl.ac.uk/spm/doc/manual.pdf#page=19


Please fix link, this leads to page 16 (Slice timing correction)

oesteban · 2017-04-29T08:58:15Z

nipype/interfaces/spm/preprocess.py

+
+
+class FieldMap(SPMCommand):
+    """Use spm to calculate fieldmap vdm.


what "vdm" means? (should be in the documentation, so please expand the acronym in this docstring)

VDM stands for Voxel Displacement Map. Docstring will be modified in the next commit.

oesteban · 2017-04-29T09:00:33Z

nipype/interfaces/spm/preprocess.py

+        field='subj.defaults.defaultsval.uflags.ws',
+        desc='weighted smoothing');
+    # Brain mask defaults parameters
+    template = traits.File(copyfile=False, exists=True,


please use File from nipype.interfaces.base, not from traits.

oesteban · 2017-04-29T09:00:51Z

nipype/interfaces/spm/preprocess.py

+        field='subj.defaults.defaultsval.mflags.reg',
+        desc='regularization value used in the segmentation');
+    # EPI unwarping for quality check
+    epi = traits.File(copyfile=False, exists=True, mandatory=True,


File from base

oesteban · 2017-04-29T09:02:24Z

nipype/interfaces/spm/preprocess.py

+    matchvdm = traits.Bool(True, usedefault=True,
+        field='subj.matchvdm',
+        desc='match VDM to EPI');
+    sessname = traits.String('_run-', usedefault=True,


please, modify the top from ..base import ... to add Str.

Then use it here directly: sessname = Str(...)

oesteban · 2017-04-29T09:02:36Z

nipype/interfaces/spm/preprocess.py

+    writeunwarped = traits.Bool(False, usedefault=True,
+        field='subj.writeunwarped',
+        desc='write unwarped EPI');
+    anat = traits.File(copyfile=False, exists=True,


File from base

oesteban · 2017-04-29T09:05:40Z

nipype/interfaces/spm/preprocess.py

+    magnitude = File(mandatory=True, exists=True, copyfile=False,
+        field='subj.data.presubphasemag.magnitude',
+        desc='presubstracted magnitude file')
+    et = traits.List(traits.Float(), minlen=2, maxlen=2, mandatory=True,


maybe:

et = traits.Tuple(traits.Float, traits.Float, mandatory=True, ...)

?

oesteban · 2017-04-29T09:09:00Z

nipype/interfaces/spm/preprocess.py

+    def _format_arg(self, opt, spec, val):
+        """Convert input to appropriate format for spm
+        """
+        if opt == 'phase' or opt == 'magnitude' or opt == 'anat':


if opt in ['phase', 'magnitude', 'anat']:

oesteban · 2017-04-29T09:11:04Z

nipype/interfaces/spm/preprocess.py

+        """
+        if opt == 'phase' or opt == 'magnitude' or opt == 'anat':
+            return scans_for_fname(filename_to_list(val))
+        if opt == 'epi' or opt == 'magnitude':


if opt in ['epi', 'magnitude']:

However, I don't see any difference with including these two options with the previous condition, finally, it always falls into scans_for_fname(filename_to_list(val)) anyways

I must have done this for a reason that I can't remember anymore... I will merge these if statements in the next commit.

codecov-io · 2018-02-07T14:51:39Z

Codecov Report

Merging #1905 into master will increase coverage by <.01%.
The diff coverage is 80%.

@@            Coverage Diff            @@
##           master   #1905      +/-   ##
=========================================
+ Coverage   67.49%   67.5%   +<.01%     
=========================================
  Files        1225    1226       +1     
  Lines       60313   60375      +62     
  Branches     8649    8656       +7     
=========================================
+ Hits        40709   40756      +47     
- Misses      18488   18502      +14     
- Partials     1116    1117       +1

Flag	Coverage Δ
#smoketests	`51.32% <78.43%> (+0.05%)`	⬆️
#unittests	`65.47% <78.46%> (ø)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/tests/test_auto_DataGrabber.py	`78.57% <ø> (ø)`	⬆️
...ipype/interfaces/tests/test_auto_SSHDataGrabber.py	`78.57% <ø> (ø)`	⬆️
nipype/interfaces/spm/__init__.py	`100% <100%> (ø)`	⬆️
nipype/interfaces/spm/base.py	`83.83% <50%> (-0.23%)`	⬇️
nipype/interfaces/spm/preprocess.py	`57.27% <79.16%> (+1.21%)`	⬆️
nipype/interfaces/spm/tests/test_auto_FieldMap.py	`85.71% <85.71%> (ø)`
nipype/utils/profiler.py	`43.1% <0%> (-1.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 628bdd5...a4994ef. Read the comment docs.

All the requested changes have been included, I'll try to review again but I don't want my potential review to block this PR

effigies · 2018-02-14T13:51:59Z

nipype/interfaces/spm/preprocess.py

+    >>> from nipype.interfaces.spm import FieldMap
+    >>> fm = FieldMap()
+    >>> fm.inputs.phase = 'phasediff.nii'
+    >>> fm.inputs.magnitude = 'magnitude1.nii'


phasediff.nii and magnitude1.nii don't exist in nipype/testing/data. Either add them or use existing files (such as phase.nii and magnitude.nii).

@effigies

as suggested by @effigies

effigies

A few comments. One to get your doctest fixed, and a number because I felt that your inputspec might benefit from clearer names.

I marked a few with specific suggestions, but in general you may want to consider using more verbose names, rather than copying the field names.

effigies · 2018-02-15T01:15:16Z

nipype/interfaces/spm/preprocess.py

+    >>> fm = FieldMap()
+    >>> fm.inputs.phase = 'phase.nii'
+    >>> fm.inputs.magnitude = 'magnitude.nii'
+    >>> fm.inputs.et = [5.19, 7.65]


This should be a tuple, not a list.

effigies · 2018-02-15T01:22:39Z

nipype/interfaces/spm/preprocess.py

+            outputs['vdm'] = []
+            for phase in filename_to_list(self.inputs.phase):
+                outputs['vdm'].append(fname_presuffix(phase, prefix='vdm5_sc'))
+            outputs['vdm'] = list_to_filename(outputs['vdm'])


I'm a bit confused by this. self.inputs.phase is a single file, so this could just be:

if self.inputs.jobtype == 'calculatevdm': outputs['vdm'] = fname_presuffix(self.inputs.phase, prefix='vdm5_sc')

Or am I missing something?

You're not, I just adapted another more complicated wrapper's piece of code without thinking about making it clearer. Thanks for noticing!

effigies · 2018-02-15T01:24:03Z

nipype/interfaces/spm/preprocess.py

+        """
+        einputs = super(FieldMap, self)._parse_inputs()
+        jobtype = self.inputs.jobtype
+        return [{'%s' % (jobtype): einputs[0]}]


What's the advantage of this over the following?

return [{self.inputs.jobtype: einputs[0]}]

I like better your style, but I originally used the syntax of another wrapper (typically Coregister).

effigies · 2018-02-15T01:28:50Z

nipype/interfaces/spm/preprocess.py

+    magnitude = File(mandatory=True, exists=True, copyfile=False,
+        field='subj.data.presubphasemag.magnitude',
+        desc='presubstracted magnitude file')
+    et = traits.Tuple(traits.Float, traits.Float, mandatory=True,


I see this is matches the final part of the field name, so it may be that SPM users will find this intuitive, but you may want to consider echo_times or similar for clarity.

effigies · 2018-02-15T01:30:29Z

nipype/interfaces/spm/preprocess.py

+class FieldMapInputSpec(SPMCommandInputSpec):
+    jobtype = traits.Enum('calculatevdm', 'applyvdm', usedefault=True,
+        desc='one of: calculatevdm, applyvdm')
+    phase = File(mandatory=True, exists=True, copyfile=False,


We often use the suffix _file to indicate inputs that should be files. So consider making this phase_file. Similarly below with magnitude_file.

Not a big deal, if you think that would be disorienting to SPM users.

effigies · 2018-02-15T01:31:26Z

nipype/interfaces/spm/preprocess.py

+    epifm = traits.Bool(False, usedefault=True,
+        field='subj.defaults.defaultsval.epifm',
+        desc='epi-based field map');
+    ajm = traits.Bool(False, usedefault=True,


Here, again, I would consider jacobian_modulation or similar.

effigies · 2018-02-15T01:32:20Z

nipype/interfaces/spm/preprocess.py

+    template = File(copyfile=False, exists=True,
+        field='subj.defaults.defaultsval.mflags.template',
+        desc='template image for brain masking');
+    fwhm = traits.Range(low=0, value=5, usedefault=True,


You've got fwhm twice. Perhaps this one should be mask_fwhm, and the other unwarp_fwhm?

effigies · 2018-02-15T16:37:28Z

Also, please merge master again, to update the tests.

effigies · 2018-02-16T20:26:28Z

nipype/interfaces/spm/preprocess.py

+    >>> fm.inputs.echo_times = (5.19, 7.65)
+    >>> fm.inputs.blip_direction = 1
+    >>> fm.inputs.total_readout_time = 15.6
+    >>> fm.inputs.epi_file = 'bold.nii'


Sorry for another round, but, this needs to be 'epi.nii', or some other file in nipype/testing/data

No worries! It's on me! Btw, I went through the CONTRIBUTING.md file, but did not see any paragraph explaining how to run tests before submitting a PR. Maybe it would have save you some reviews of my code and others'.

Ah, that should probably be more clear. I just run

PYTHONPATH="." py.test --doctest-modules nipype

That tests all of nipype. You can specify a specific file you want to test, as well:

PYTHONPATH="." py.test --doctest-modules nipype/interfaces/spm/preprocess.py

effigies

LGTM.

fabioboh · 2019-02-11T09:09:26Z

I am working on implementing the apply vdm feature.

fabioboh · 2022-08-18T08:34:06Z

I will soon make a pull request so the apply VDM also work on multiple frames (e.g. fMRI images). This is a tiny change of my previous pull request and I am actually sorry it was not already included in the previous one.

Add SPM Fieldmap Tool wrapper

d1cee47

Added: - VDM Calculation pre-substracted phase case (SIEMENS machines) Todo: - Support VDM Application - Support different magnitude and phase file types

oesteban previously requested changes Apr 29, 2017

View reviewed changes

satra added the 2 - Working <= 5 label Sep 11, 2017

Add first remarks from PR

0fd77fc

Merge branch 'master' into enh/spm-fieldmap-wrapper

302be56

effigies reviewed Feb 14, 2018

View reviewed changes

Fix doctest filenames

39715f2

as suggested by @effigies

effigies reviewed Feb 15, 2018

View reviewed changes

jguillon added 2 commits February 16, 2018 15:56

Fix doctest and make vars more explicit

c397662

Merge branch 'master' into enh/spm-fieldmap-wrapper

88fee4a

effigies reviewed Feb 16, 2018

View reviewed changes

satra mentioned this pull request Feb 16, 2018

update testing docs #2445

Closed

jguillon added 2 commits February 19, 2018 12:04

Merge remote-tracking branch 'nipy/master' into enh/spm-fieldmap-wrapper

dd7569f

Fix my doctest and add test_auto file

a4994ef

effigies approved these changes Feb 19, 2018

View reviewed changes

effigies added this to the 1.0.1 milestone Feb 20, 2018

effigies merged commit 6e07273 into nipy:master Feb 20, 2018

jguillon deleted the enh/spm-fieldmap-wrapper branch February 11, 2019 12:09

fabioboh mentioned this pull request Feb 11, 2019

ENH/WIP: added Apply VDM functionality to FieldMap SPM interface #2879

Closed

fabioboh mentioned this pull request Oct 19, 2021

ENH: added Apply VDM functionality to FieldMap SPM interface #3394

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

jguillon commented Mar 23, 2017

oesteban left a comment

oesteban Apr 29, 2017

oesteban Apr 29, 2017

jguillon Jul 7, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

oesteban Apr 29, 2017

jguillon Jul 7, 2017

codecov-io commented Feb 7, 2018 •

edited

Loading

effigies Feb 14, 2018

effigies left a comment

effigies Feb 15, 2018

effigies Feb 15, 2018

jguillon Feb 15, 2018

effigies Feb 15, 2018

jguillon Feb 15, 2018

effigies Feb 15, 2018

effigies Feb 15, 2018

effigies Feb 15, 2018

effigies Feb 15, 2018

effigies commented Feb 15, 2018

effigies Feb 16, 2018

jguillon Feb 16, 2018

effigies Feb 16, 2018

effigies left a comment

fabioboh commented Feb 11, 2019

fabioboh commented Aug 18, 2022



		class FieldMap(SPMCommand):
		"""Use spm to calculate fieldmap vdm.

[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

Conversation

jguillon commented Mar 23, 2017

oesteban 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

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

codecov-io commented Feb 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

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 Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

fabioboh commented Feb 11, 2019

fabioboh commented Aug 18, 2022

codecov-io commented Feb 7, 2018 •

edited

Loading