ENH: Add FSL `eddy_quad` interface #2825

richford · 2018-12-13T18:45:22Z

Summary

This PR adds an interface to eddy_quad, one of two tools in the EDDY QC framework.
See the EDDY QC documentation for further details. Thanks to @akeshavan for help with my first nipype PR.

List of changes proposed in this PR (pull-request)

Add EddyQuadInputSpec class
Add EddyQuadOutputSpec class
Add EddyQuad class with doctest

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

richford · 2018-12-13T20:40:20Z

Hmm, the travis PR checks failed, but only for Python 3.4 because xdist could not be loaded. Not sure why this is happening. I don't think that anything I changed in the PR is causing this. Any help would be appreciated.

effigies

Thanks for the contribution! Overall, this looks great.

I have a few suggestions/comments/questions.

And don't worry about the Python 3.4 tests. I made an issue in #2826.

nipype/interfaces/fsl/epi.py

effigies · 2018-12-13T18:52:17Z

nipype/interfaces/fsl/epi.py

+            os.path.join(out_dir, 'vdm.png')
+        )
+
+        if os.path.exists(vdm):


These checks are a little bit worrisome. Is there no way you can predict whether it should exist from the inputs?

Thanks. In 8bac064, I inspect the qc.json file to see if those files should exist.

effigies · 2018-12-13T18:53:11Z

nipype/interfaces/fsl/epi.py

+        )
+
+        outputs['out_avg_b0_png'] = glob(os.path.abspath(
+            os.path.join(out_dir, 'avg_b0_pe*.png')


Is the use of glob here a time saver, or is the number of outputs unknown until after it's been run? If the former, it would be better to actually predict the outputs. If the latter, then this is fine.

Good call. In 8bac064, I now inspect the qc.json which has flags that indicate the presence of certain output files and the number of expected files for each type.

effigies · 2018-12-13T18:55:23Z

nipype/interfaces/fsl/epi.py

+    base_name = traits.Str(
+        'eddy_corrected',
+        argstr='%s',
+        desc="Basename (including path) specified when running EDDY",


As a non-EDDY user, this means little to me. Will the intent here be obvious to EDDY users?

I improved the description in 8bac064. I think it should be clear to EDDY users. But honestly, I'm no EDDY expert myself.

effigies · 2018-12-13T19:10:57Z

nipype/interfaces/fsl/epi.py

+              "file is present")
+    )
+    output_dir = traits.Str(
+        'eddy_corrected.qc',


Suggested change

'eddy_corrected.qc',

name_template='%s.qc',

name_source=['base_name'],

Thanks. I think I resolved this in 8bac064. Is that correct?

nipype/interfaces/fsl/epi.py

- Remove redundant `__init__` method in `EddyQuad` class. - Use `os.path.abspath()` earlier in order to remove from later statements. - Improve `EddyQuad`'s `base_name` input description. - Remove unnecessary `mandatory=False` params. - Rename `slspec` to `slice_spec`. - Use default for `EddyQuad`'s `base_name` input. - Use a name template for `EddyQuad`'s `output_dir` input.

codecov-io · 2018-12-14T22:30:44Z

Codecov Report

Merging #2825 into master will decrease coverage by 0.01%.
The diff coverage is 54.34%.

@@            Coverage Diff             @@
##           master    #2825      +/-   ##
==========================================
- Coverage   67.39%   67.38%   -0.02%     
==========================================
  Files         341      341              
  Lines       43428    43474      +46     
  Branches     5396     5401       +5     
==========================================
+ Hits        29270    29294      +24     
- Misses      13461    13474      +13     
- Partials      697      706       +9

Flag	Coverage Δ
#smoketests	`50.47% <54.34%> (ø)`	⬆️
#unittests	`64.79% <54.34%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/fsl/__init__.py	`100% <ø> (ø)`	⬆️
nipype/interfaces/fsl/epi.py	`63.22% <54.34%> (-0.98%)`	⬇️
nipype/testing/utils.py	`89.65% <0%> (-1.73%)`	⬇️
nipype/interfaces/dynamic_slicer.py	`17.47% <0%> (ø)`	⬆️
nipype/interfaces/nipy/preprocess.py	`45.79% <0%> (ø)`	⬆️
nipype/interfaces/io.py	`53.99% <0%> (ø)`	⬆️
nipype/interfaces/freesurfer/preprocess.py	`66.11% <0%> (ø)`	⬆️

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 f2a21fd...41d82ad. Read the comment docs.

richford · 2018-12-14T22:44:57Z

Thanks for your review @effigies. I responded inline to your reviews and closed the ones that I felt were trivially resolved, leaving the more involved ones to you to review and close if you agree that I've addressed them. Thanks again for your help!

effigies

Thanks for making the changes. Sorry to do this to you, but I've got another round of comments, and some of them are to undo some of what you've done... Thanks for your patience.

effigies · 2018-12-22T02:46:56Z

.zenodo.json

+      "affiliation": "University of Washington",
+      "name": "Richie-Halford, Adam",
+      "orcid": "0000-0001-9276-9084"
+    },


Suggested change

},

}

effigies · 2018-12-22T02:47:46Z

nipype/interfaces/fsl/epi.py

+class EddyQuadOutputSpec(TraitedSpec):
+    out_qc_json = File(
+        exists=True,
+        mandatory=True,


Suggested change

mandatory=True,

effigies · 2018-12-22T02:47:53Z

nipype/interfaces/fsl/epi.py

+
+    out_qc_pdf = File(
+        exists=True,
+        mandatory=True,


Suggested change

mandatory=True,

effigies · 2018-12-22T02:48:36Z

nipype/interfaces/fsl/epi.py

+    )
+
+    out_avg_b_png = traits.List(
+        File(


Suggested change

File(

File(exists=True),

effigies · 2018-12-22T02:50:58Z

nipype/interfaces/fsl/epi.py

+            exists=True,
+            mandatory=True,
+            desc=("Image showing mid-sagittal, -coronal and -axial slices of "
+                  "each averaged b-shell volume.")


desc should be a parameter to List, not File, and you can drop mandatory. I've suggested removing it from the other output spec files... I don't think it's enforced, but if it is, it will cause problems if you have the remove_unnecessary_outputs config option set, and no node takes those files as inputs.

effigies · 2018-12-22T02:51:36Z

nipype/interfaces/fsl/epi.py

+            mandatory=False,
+            desc=("Image showing mid-sagittal, -coronal and -axial slices of "
+                  "each b-shell CNR volume. Generated when CNR maps are "
+                  "available.")


effigies · 2018-12-22T02:52:14Z

nipype/interfaces/fsl/epi.py

+
+    out_vdm_png = File(
+        exists=True,
+        mandatory=False,


Suggested change

mandatory=False,

effigies · 2018-12-22T02:52:22Z

nipype/interfaces/fsl/epi.py

+
+    out_residuals = File(
+        exists=True,
+        mandatory=False,


Suggested change

mandatory=False,

effigies · 2018-12-22T02:52:28Z

nipype/interfaces/fsl/epi.py

+
+    out_clean_volumes = File(
+        exists=True,
+        mandatory=False,


Suggested change

mandatory=False,

effigies · 2018-12-22T02:57:22Z

nipype/interfaces/fsl/epi.py

+        outputs['out_qc_pdf'] = os.path.join(out_dir, 'qc.pdf')
+
+        with open(outputs['out_qc_json']) as fp:
+            qc = json.load(fp)


Ah, we'd prefer glob to this. The reason is that _list_outputs() should be runnable without having already run the interface. With glob you'll just get an empty list, while attempting to read a JSON file will actually fail.

If there's no way to predict the outputs entirely without running, that's fine. I was just suggesting that, if you could, that's a little nicer. But we should only depend on self.inputs to determine the expected outputs. And I can't remember whether you used sorted(glob(...)) before, but I would recommend sorting the list so it's consistent across runs. (glob does not have to return sorted lists.)

effigies · 2019-01-16T21:54:25Z

Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release.

richford · 2019-01-16T22:26:58Z

Yes. Sorry for the delay and thanks for the nudge. I'll try to get this done in the next week.

richford · 2019-01-18T23:00:42Z

@effigies I think this is ready for review. I changed back to glob and tried to rely on self.inputs as much as I could. I also added a fallback check for the output_dir after @akeshavan and I found that self.inputs.output_dir was undefined when it used the default even though it passed the correct value to the FSL CLI.

effigies

Awesome. Looks good. A few minor touchups and suggestions, and I think this is good to go. Feel free to push back if I'm getting too nit-picky, by the way.

nipype/interfaces/fsl/epi.py

effigies · 2019-01-21T02:57:08Z

nipype/interfaces/fsl/epi.py

+        # If the output directory isn't defined, the interface seems to use
+        # the default but not set its value in `self.inputs.output_dir`
+        if not isdefined(self.inputs.output_dir):
+            out_dir = os.path.abspath(os.path.basename(self.inputs.base_name) + '.qc.nii.gz')


The default output is a directory named eddy_corrected.qc.nii.gz/? This looks weird, so just want to check there isn't a typo.

@akeshavan, we tested this on your system. I think this is correct but it is very weird. Could you confirm? (I don't want to take the hours needed to build the docker image on my laptop. 😏)

effigies · 2019-01-21T15:11:07Z

Oh, can you please merge master to fix the tests?

effigies · 2019-01-21T15:11:53Z

@akeshavan Feel free to merge if you're happy with this.

- Remove redundant `__init__` method in `EddyQuad` class. - Use `os.path.abspath()` earlier in order to remove from later statements. - Improve `EddyQuad`'s `base_name` input description. - Remove unnecessary `mandatory=False` params. - Rename `slspec` to `slice_spec`. - Use default for `EddyQuad`'s `base_name` input. - Use a name template for `EddyQuad`'s `output_dir` input.

…tputs()

Co-Authored-By: richford <richford@users.noreply.github.com>

richford · 2019-01-21T15:22:38Z

Oh, can you please merge master to fix the tests?

Just rebased onto master (with a force push for the change in history).

effigies · 2019-01-22T14:31:50Z

I think we can take your word on the default output_dir. Please open a new PR if you discover it should be something else.

@effigies

* origin/master: (63 commits) Update nipype/interfaces/dipy/tracks.py Update nipype/interfaces/dipy/reconstruction.py MNT: Install numpy!=1.16.0 from conda in Docker Add FSL auto test remake specs Update nipype/interfaces/io.py Remove return type named tuple Update nipype/info.py STY: Whitespace, line length Remove out_ prefix from EddyQuad outputs Apply minor edits from code review Use os.path.basename for the fallback output_dir in EddyQuad._list_outputs() Add output_dir check to EddyQuad._list_outputs() Remove redundant out_avg_b_png lines Add glob stuff back in Edit in response to @effigies comments on PR nipy#2825 Fix tests for EddyQuad in interfaces/fsl/epi.py Add name to .zenodo.json Add doctest for EddyQuad fix: made the outputdir be mandatory and use the default val ...

@feilong

1.1.8 (January 28, 2019) * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846) * FIX: Resolve LinAlgError during SVD (nipy#2838) * ENH: Add interfaces wrapping DIPY worflows (nipy#2830) * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737) * ENH: Add FSL `eddy_quad` interface (nipy#2825) * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823) * ENH: Support for BIDS event files (nipy#2845) * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785) * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850) * DOC: Add S3DataGrabber example (nipy#2849) * DOC: Skip conftest module in API generation (nipy#2852) * DOC: Hyperlink DOIs to preferred resolver (nipy#2833) * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862) * MAINT: Drop pytest-xdist requirement, minimum pytest version (nipy#2856) * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855) * tag '1.1.8': (79 commits) MNT: Add @feilong to .zenodo, update ordering MNT: Update .mailmap MNT: Update .zenodo ordering Accept invitation as Zenodo release co-author (see nipy#2864) MAINT: Update .mailmap BF: allowing bids_event_file as alternate input MNT: Update .zenodo ordering MNT: Version 1.1.8 DOC: 1.1.8 changelog Update nipype/interfaces/dipy/tracks.py Update nipype/interfaces/dipy/reconstruction.py MNT: Install numpy!=1.16.0 from conda in Docker Add FSL auto test remake specs Update nipype/interfaces/io.py Remove return type named tuple Update nipype/info.py STY: Whitespace, line length Remove out_ prefix from EddyQuad outputs Apply minor edits from code review ...

effigies reviewed Dec 13, 2018

View reviewed changes

effigies added this to the 1.1.8 milestone Dec 14, 2018

effigies reviewed Dec 22, 2018

View reviewed changes

effigies approved these changes Jan 21, 2019

View reviewed changes

Adam Richie-Halford and others added 13 commits January 21, 2019 07:20

Add EddyQuad interface

e35cbf8

fix: completed outputspec for EddyQuad

9e4d5e8

fix: made the outputdir be mandatory and use the default val

292d5fb

Add doctest for EddyQuad

8bfc9fa

Add name to .zenodo.json

48482fe

Fix tests for EddyQuad in interfaces/fsl/epi.py

4c19775

Add glob stuff back in

4a41f5e

Remove redundant out_avg_b_png lines

d01bf40

Add output_dir check to EddyQuad._list_outputs()

f6b49f5

Use os.path.basename for the fallback output_dir in EddyQuad._list_ou…

14027c5

…tputs()

Apply minor edits from code review

51b7f45

Co-Authored-By: richford <richford@users.noreply.github.com>

Remove out_ prefix from EddyQuad outputs

41d82ad

richford force-pushed the fsl-eddy-quad-interface branch from 2fe91b9 to 41d82ad Compare January 21, 2019 15:21

effigies merged commit 6b74fd2 into nipy:master Jan 22, 2019

josephmje mentioned this pull request Mar 18, 2019

Nipype syntax change nipy/dmriprep#67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add FSL `eddy_quad` interface #2825

ENH: Add FSL `eddy_quad` interface #2825

richford commented Dec 13, 2018 •

edited

Loading

richford commented Dec 13, 2018

effigies left a comment

effigies Dec 13, 2018

richford Dec 14, 2018

effigies Dec 13, 2018

richford Dec 14, 2018

effigies Dec 13, 2018

richford Dec 14, 2018

effigies Dec 13, 2018

richford Dec 14, 2018

codecov-io commented Dec 14, 2018 •

edited

Loading

richford commented Dec 14, 2018

effigies left a comment

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies Dec 22, 2018

effigies commented Jan 16, 2019

richford commented Jan 16, 2019

richford commented Jan 18, 2019

effigies left a comment

effigies Jan 21, 2019

richford Jan 21, 2019

effigies commented Jan 21, 2019

effigies commented Jan 21, 2019

richford commented Jan 21, 2019

effigies commented Jan 22, 2019

	'eddy_corrected.qc',
	name_template='%s.qc',
	name_source=['base_name'],

ENH: Add FSL eddy_quad interface #2825

ENH: Add FSL eddy_quad interface #2825

Conversation

richford commented Dec 13, 2018 • edited Loading

Summary

List of changes proposed in this PR (pull-request)

Acknowledgment

richford commented Dec 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 14, 2018 • edited Loading

Codecov Report

richford commented Dec 14, 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

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 Jan 16, 2019

richford commented Jan 16, 2019

richford commented Jan 18, 2019

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

effigies commented Jan 21, 2019

effigies commented Jan 21, 2019

richford commented Jan 21, 2019

effigies commented Jan 22, 2019

ENH: Add FSL `eddy_quad` interface #2825

ENH: Add FSL `eddy_quad` interface #2825

richford commented Dec 13, 2018 •

edited

Loading

codecov-io commented Dec 14, 2018 •

edited

Loading