[ENH] Extended MRtrix3 interface #2338

matteomancini · 2017-12-12T16:25:30Z

Changes proposed in this pull request (#2299):

Added interfaces to mrconvert, dwiextract and mrmath (for operations on single images) not previously included in the mrtrix3 interfaces;
Updated interfaces to 5ttgen, dwi2response and dwi2fod taking into account the new syntax in the latest releases of MRtrix3 and offering the chance to use multi-tissue constrained spherical deconvolution.

This reverts commit bb9f5b4.

…into enh-mrtrix3

satra · 2017-12-12T17:44:19Z

nipype/interfaces/niftyfit/asl.py

@@ -147,7 +147,7 @@ class FitAsl(NiftyFitCommand):
    >>> from nipype.interfaces import niftyfit
    >>> node = niftyfit.FitAsl()
    >>> node.inputs.source_file = 'asl.nii.gz'
-    >>> node.cmdline
+    >>> node.cmdline  


there are still a few weird things like this. that added space is unnecessary.

satra · 2017-12-12T17:44:26Z

nipype/pipeline/engine/tests/test_utils.py

@@ -23,7 +23,7 @@ def test_identitynode_removal(tmpdir):
    def test_function(arg1, arg2, arg3):
        import numpy as np
        return (np.array(arg1) + arg2 + arg3).tolist()
-
+    


satra · 2017-12-12T17:45:14Z

there are some weird ones still. could you please run: make trailing-spaces and commit and push.

matteomancini · 2017-12-13T09:05:01Z

I removed the last trailing spaces, now the changed files in my branch compared to master are only the ones relevant to the pull request: master...matteomancini:enh-mrtrix3

mgxd

Thank you for this large update to our MRtrix3 interfaces! I see a lot of inputs removed/renamed, so this will be a big change to our api.

If the new MRtrix3 api has completely changed these interfaces, it may be better to leave the old interfaces as is and make new class for the latest version, similar to how some of our freesurfer interfaces are setup after FS6 was released. Thanks again for the contribution!

mgxd · 2017-12-19T15:52:28Z

nipype/interfaces/mrtrix3/preprocess.py

-                        desc='specify one or more dw gradient shells')
+    algorithm = traits.Enum('msmt_5tt','dhollander','tournier','tax', argstr='%s', position=-6,
+        mandatory=True, desc='response estimation algorithm (multi-tissue)')
+    dwi_file = File(exists=True, argstr='%s', position=-5,


I'm in favor of this remaining in_file to keep some consistency across nipype interfaces

It makes sense. I will go through the PR and refers to the input diffusion data as in_file.

mgxd · 2017-12-19T15:55:57Z

nipype/interfaces/mrtrix3/preprocess.py

@@ -16,87 +16,47 @@
 import os.path as op

 from ..base import (CommandLineInputSpec, CommandLine, traits, TraitedSpec,
-                    File, isdefined)
+                    File, isdefined, Undefined)
 from .base import MRTrix3BaseInputSpec, MRTrix3Base


 class ResponseSDInputSpec(MRTrix3BaseInputSpec):


Do these changes remove backward compatibility to previous versions of dwi2response?

Yes, they do since the current version of MRtrix provides different APIs from the ones implemented at the moment in the Nipype interface (#2299).

mgxd · 2017-12-19T15:56:30Z

nipype/interfaces/mrtrix3/preprocess.py

-        if isdefined(self.inputs.out_sf):
-            outputs['out_sf'] = op.abspath(self.inputs.out_sf)
+        outputs['wm_file'] = op.abspath(self.inputs.wm_file)
+        if self.inputs.gm_file!=Undefined:


self.inputs.gm_file != Undefined

I'll take care of the missing spaces.

mgxd · 2017-12-19T15:56:44Z

nipype/interfaces/mrtrix3/preprocess.py

+        outputs['wm_file'] = op.abspath(self.inputs.wm_file)
+        if self.inputs.gm_file!=Undefined:
+            outputs['gm_file'] = op.abspath(self.inputs.gm_file)
+        if self.inputs.csf_file!=Undefined:


same as above

mgxd · 2017-12-19T15:57:14Z

nipype/interfaces/mrtrix3/reconst.py

-                               ' image'))
+    algorithm = traits.Enum('csd','msmt_csd', argstr='%s', position=-8,
+        mandatory=True, desc='FOD algorithm')
+    dwi_file = File(exists=True, argstr='%s', position=-7,


in_file for consistency

mgxd · 2017-12-19T15:59:09Z

nipype/interfaces/mrtrix3/reconst.py

+    wm_odf = File('wm.mif', argstr='%s', position=-5, usedefault=True,
+        mandatory=True, desc='output WM ODF')
+    gm_txt = File(argstr='%s', position=-4, desc='GM response text file')
+    gm_odf = File('gm.mif', argstr='%s', position=-3, desc='output GM ODF')


usedefault=True?

The new interfaces for estimating the SD responses are now able to estimate different responses for different tissues in case of multi-tissue CSD. However, the related command can be used to estimate also SD responses without taking into account tissues and therefore only related to white matter:
https://github.com/matteomancini/nipype/blob/e92e11296ccd4e1894ef835d431977bc67834955/nipype/interfaces/mrtrix3/preprocess.py#L23-L25
Setting usedefault=True only for the output related to the white matter response allows to correctly use the command both with canonical and multi-tissues algorithms.

mgxd · 2017-12-19T15:59:19Z

nipype/interfaces/mrtrix3/reconst.py

+    gm_txt = File(argstr='%s', position=-4, desc='GM response text file')
+    gm_odf = File('gm.mif', argstr='%s', position=-3, desc='output GM ODF')
+    csf_txt = File(argstr='%s', position=-2, desc='CSF response text file')
+    csf_odf = File('csf.mif', argstr='%s', position=-1, desc='output CSF ODF')


same as above

mgxd · 2017-12-19T16:00:35Z

nipype/interfaces/mrtrix3/reconst.py

@@ -185,5 +132,12 @@ class EstimateFOD(MRTrix3Base):

    def _list_outputs(self):
        outputs = self.output_spec().get()
-        outputs['out_file'] = op.abspath(self.inputs.out_file)
+        outputs['wm_odf'] = op.abspath(self.inputs.wm_odf)
+        if self.inputs.gm_odf!=Undefined:


space before/after !=

matteomancini · 2017-12-19T16:20:31Z

The issue with the new interfaces was highlighted by @oesteban in the previous PR (#2299): I agreed with him that since the MRtrix API has changed, then the interfaces needed be updated and not replaced keeping the old ones (as I did initially). At the moment, an updated installation of MRtrix won't work with some of the current interfaces since they do not take into account the new structure of certain commandline tools (i.e. cmd algorithm inputs, as in dwi2response). I'm not sure of what the Nipype general policy is when the interfaced tool changes the API as in this case. If you let me know what is the best option (updating the API as suggested by @oesteban ; adding new interfaces for the updated API ; implementing the interfaces for the old API as new interfaces), I will promptly make the changes, together with the other minor issues highlighted.

mgxd · 2017-12-19T16:36:00Z

@matteomancini as someone who rarely uses the MRtrix3 software, I am onboard with just updating the interfaces to the current (and hopefully stable) API. My only concern was for users who are using the version currently support by nipype - this is where having user statistics that can track which interfaces are being used most frequently would come in handy. Since @oesteban originally added support for this package? and is pushing to update, I don't have a problem with that

matteomancini · 2017-12-19T16:43:02Z

I don't have a problem either in case of adding interfaces to the old API, but I'm interested also for the future in knowing which is the best policy.
This case should be different from the transition from FreeSurfer 5.x to 6: I have seen that in a similar way Nipype still maintains for backward compatibility MRtrix and MRtrix3 interfaces. Instead, in this case the latest version of MRtrix has changed the API, with the result that some of the current MRtrix3 interfaces do not work with the current MRtrix. Of course, just updating the interface will not allow using the API from previous releases (some of them available in the MRtrix interface), but in that case probably it makes sense to use the previous Nipype version, or not?

matteomancini · 2018-01-04T11:45:51Z

Since there was no further reply, I wonder if then it is the case to proceed with updating the interface to the latest API as proposed in #2299 .

mgxd · 2018-01-05T20:47:37Z

@matteomancini happy to merge after the autotest conflict is fixed

matteomancini · 2018-01-08T10:18:13Z

@mgxd I looked at the comparison and I can't the conflict, all I see is that the the three checks were successful and that the branch has no conflicts with the base. I looked both on this page (the PR) and on the comparison. Let me know if you still see the conflict or if I just need to merge master and repush. Thank you.

mgxd · 2018-01-08T15:20:21Z

it looks like nipype/interfaces/afni/tests/test_auto_TCatSubBrick was moved to nipype/interfaces/mrtrix3/tests/test_auto_MRMath.py - both should exist

matteomancini · 2018-01-08T16:51:14Z

I merged master, then ran make check-before-commit and now nipype/interfaces/afni/tests/test_auto_TCatSubBrick is there.

effigies · 2018-01-08T16:54:50Z

.zenodo.json

+    {
+      "affiliation": "University College London",
+      "name": "Mancini, Matteo",
+      "orcid": "0000-0001-7194-4568"


I'm not sure what the general ordering policy is, but I'm pretty sure Satra should be the last author

Apologies, I thought the file was in some chronological order of contributions.

Looks like @mgxd (or someone) reorders it later anyway, so fixing it won't save anybody time. #2338 (comment). Sorry for the noise.

mgxd · 2018-01-08T17:02:18Z

@effigies the zenodo is fine for now - it will be rearranged (by lines contributed) on release anyways

matteomancini added 8 commits November 22, 2017 09:55

Extended MRtrix3 interface

bb9f5b4

Merge branch 'master' into enh-mrtrix3

4227937

Revert "Extended MRtrix3 interface"

c15ba6b

This reverts commit bb9f5b4.

Updated MRtrix3 interfaces

248e589

Merge branch 'enh-mrtrix3' of https://github.com/matteomancini/nipype …

88a1734

…into enh-mrtrix3

Merge remote-tracking branch 'upstream/master' into enh-mrtrix3

0cd54bc

Conflicts with master resolved

54d058e

Fixed tests

b135506

matteomancini mentioned this pull request Dec 12, 2017

ENH: Extended MRtrix3 interface #2299

Closed

satra reviewed Dec 12, 2017

View reviewed changes

matteomancini added 3 commits December 13, 2017 03:52

Removed trailing spaces

bdb340d

Removed last trailing spaces

1e1dcc0

Removed last trailing spaces

e92e112

mgxd reviewed Dec 19, 2017

View reviewed changes

mgxd added this to the 0.14.1 milestone Dec 19, 2017

matteomancini added 2 commits December 19, 2017 11:54

Minor fixes (style)

c3ccafe

Minor fixes (tests)

1197307

matteomancini added 2 commits January 8, 2018 11:32

Merge remote-tracking branch 'upstream/master' into enh-mrtrix3

6401e97

Minor fix (test)

c31d7cd

effigies reviewed Jan 8, 2018

View reviewed changes

mgxd merged commit cfcd8f6 into nipy:master Jan 8, 2018

matteomancini deleted the enh-mrtrix3 branch January 9, 2018 11:49

effigies modified the milestones: 0.14.1, 1.0 Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Extended MRtrix3 interface #2338

[ENH] Extended MRtrix3 interface #2338

matteomancini commented Dec 12, 2017 •

edited

Loading

satra Dec 12, 2017

satra Dec 12, 2017

satra commented Dec 12, 2017

matteomancini commented Dec 13, 2017 •

edited

Loading

mgxd left a comment

mgxd Dec 19, 2017

matteomancini Dec 19, 2017

mgxd Dec 19, 2017

matteomancini Dec 19, 2017

mgxd Dec 19, 2017

matteomancini Dec 19, 2017

mgxd Dec 19, 2017

mgxd Dec 19, 2017

mgxd Dec 19, 2017

matteomancini Dec 19, 2017

mgxd Dec 19, 2017

mgxd Dec 19, 2017

matteomancini commented Dec 19, 2017

mgxd commented Dec 19, 2017

matteomancini commented Dec 19, 2017

matteomancini commented Jan 4, 2018

mgxd commented Jan 5, 2018

matteomancini commented Jan 8, 2018

mgxd commented Jan 8, 2018

matteomancini commented Jan 8, 2018 •

edited

Loading

effigies Jan 8, 2018

matteomancini Jan 8, 2018

effigies Jan 8, 2018

mgxd commented Jan 8, 2018

[ENH] Extended MRtrix3 interface #2338

[ENH] Extended MRtrix3 interface #2338

Conversation

matteomancini commented Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satra commented Dec 12, 2017

matteomancini commented Dec 13, 2017 • edited Loading

mgxd 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

Choose a reason for hiding this comment

matteomancini commented Dec 19, 2017

mgxd commented Dec 19, 2017

matteomancini commented Dec 19, 2017

matteomancini commented Jan 4, 2018

mgxd commented Jan 5, 2018

matteomancini commented Jan 8, 2018

mgxd commented Jan 8, 2018

matteomancini commented Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgxd commented Jan 8, 2018

matteomancini commented Dec 12, 2017 •

edited

Loading

matteomancini commented Dec 13, 2017 •

edited

Loading

matteomancini commented Jan 8, 2018 •

edited

Loading