nipy / nipype

Workflows and interfaces for neuroimaging packages
https://nipype.readthedocs.org/en/latest/
Other
746 stars 530 forks source link

cmp package in /nipype/workflows/dmri/mrtrix/group_connectivity.py #1106

Open jpellman opened 9 years ago

jpellman commented 9 years ago

Hi,

In one of the dMRI workflows I noticed the following set of lines:

https://github.com/nipy/nipype/blob/732cef3f0259fec92f9dd15205c7c209e2606499/nipype/workflows/dmri/mrtrix/group_connectivity.py#L10-L15

However, 'cmp' never seems to actually be used. Is this code legacy, or is there something that I'm missing? I know that cmp is optional (as per https://neurostars.org/p/2960/ ) so I've been ignoring the warning, but it seems entirely unnecessary if group_connectivity.py isn't using cmp at all.

swederik commented 9 years ago

Hi,

Yeah you can ignore the warning. Here's the long story:

The check for CMP was written because the connectivity pipeline depends on the connectome mapper's parcellation (see https://github.com/nipy/nipype/blob/732cef3f0259fec92f9dd15205c7c209e2606499/nipype/interfaces/cmtk/parcellation.py#L306) and it was stable at the time.

Since then, however, the original CMP API was broken by a number of recent commits (https://github.com/LTS5/cmp/commits/master) making it extremely annoying to depend upon (you had to checkout an old commit to get it to work: https://groups.google.com/forum/#!searchin/nipy-user/cmp/nipy-user/BGscJb4-Ne4/KUn4x0-UEU4J).

Now the CMP developers have abandoned the original repository in favour of a version based on Nipype (https://github.com/LTS5/cmp_nipype), and this is what new users keep tending to install when they want to use Nipype to make connectomes (see e.g. https://neurostars.org/p/2992/). The weird thing is that it depends on Nipype (and the cmtk.Parcellate interface, see https://github.com/LTS5/cmp_nipype/blob/3452142c064694966471b88b0ec903a9531dd35c/cmp/stages/parcellation/parcellation.py#L91), which in turn depends on the old cmp package. I haven't actually tested cmp_nipype, so I don't know what you need to do to set it up.

The easiest solution is to remove the references to cmp in Nipype completely (they're only really in the Parcellate interface). It's only there because the original package includes the parcellation scheme information (i.e. region names for the Lausanne 2008 atlas). Other than that there is the use of "runCmd", which is just a thin unnecessary wrapper used for calling freesurfer commands with subprocess (https://github.com/LTS5/cmp/blob/93094ce227bda9064512290dd505a7ba75cf7072/cmp/logme.py#L70).

The best option is to rewrite the Parcellate interface as a workflow of Freesurfer commands. That would allow all the steps inside the interface to run in parallel and allow them to be stopped / started without losing progress. It's something I think @chrisfilo and/or @JohnGriffiths and I discussed a long time ago, but it's a thankless and time consuming job (especially because testing your implementation works will take hours).

oesteban commented 9 years ago

Since it is a warning raised by a workflow demonstrating MRTrix, and new MRTrix3 version provides tools for the connectome extraction, cmp is not a requirement anymore.

What do you think about including this issue in #1126, and I update the workflow to use the new version 3 of MRTrix and I replace connectome computation of cmp?

cc @chrisfilo

djarecka commented 6 years ago

@oesteban - are you still planning to change MRTrix to MRTrix3 and remove the warning?

oesteban commented 6 years ago

Sure, let's put it in 0.14.1

effigies commented 6 years ago

@oesteban Any interest in trying to squeeze this into 1.1?