[FIX/REF] Switch to new MRtrix3 labelconvert CL tool
#2441
Conversation
|
@jguillon - could you please merge with master, make specs, commit any changes relevant to these interfaces, and push? |
Codecov Report
@@ Coverage Diff @@
## master #2441 +/- ##
==========================================
+ Coverage 66.66% 66.66% +<.01%
==========================================
Files 328 328
Lines 42529 42557 +28
Branches 5278 5281 +3
==========================================
+ Hits 28350 28371 +21
- Misses 13500 13505 +5
- Partials 679 681 +2
Continue to review full report at Codecov.
|
|
@jguillon - you don't need to include the actual freesurfer color LUT, since the testing is only to ensure a filename exists. you can keep the file, but empty out the contents. that will reduce the size of the repo. |
file contents not required for testing
|
@jguillon - i just emptied it out directly. will wait for at least the travis tests to pass and merge. |
|
@satra You may have already been planning this, but it would be good to merge as a squashed commit, so the contents aren't sitting in the history. (Only a few kilobytes, but still. Nipype is already a heavy clone.) |
|
@effigies - yes i was planning on that for this one. |
|
Quick question before merging: Is the old |
|
If, in general, |
|
nipype is fully compatible with SPM5/8 and FreeSurfer 5.X. we take care of this through versioning of interfaces/packages, and introducing fields that are dependent on |
|
Amazing! Then, let's keep the "old" |
|
@jguillon - there are a few ways of doing whole interfaces:
|
|
@jguillon Have you had a chance to look at this further? Let us know if you have any more questions. |
|
Hi again! Sorry I didn't have much time these last days. I think I'll go with the simple solution of keeping the old interface of As a side question, don't you ever check (and force) for the version of SPM when instantiating a SPM12-specific interface like this one: nipype/nipype/interfaces/spm/preprocess.py Line 785 in db26703 or did I miss something? Didn't test at all, just a curiosity. |
|
Just restoring the old one and keeping your new one alongside is definitely easiest. That works for me. (If we want to add in deprecating code, we can push to your branch before merging.) |
| 'segmentation of the base of the spine where the streamlines' | ||
| ' terminate, so that this can become a node in the connection' | ||
| ' matrix.') | ||
| nthreads = traits.Int( |
There was a problem hiding this comment.
This should be num_threads, to help nipype automatically do resource management.
| >>> labels.inputs.in_file = 'aparc+aseg.nii' | ||
| >>> labels.inputs.in_config = 'mrtrix3_labelconfig.txt' | ||
| >>> labels.inputs.in_lut = 'FreeSurferColorLUT.txt' | ||
| >>> labels.cmdline # doctest: +ELLIPSIS |
There was a problem hiding this comment.
No ellipsis in the cmdline, so I think we can drop the doctest directive here.
|
|
||
| if not isdefined(self.inputs.in_config): | ||
| from distutils.spawn import find_executable | ||
| path = find_executable(self._cmd) |
There was a problem hiding this comment.
We're looking at dropping the distutils dependency (#2348). Can you use nipype.utils.filemanip.which instead?
Changes old MRtrix3
labelconfigtolabelconvert.http://mrtrix.readthedocs.io/en/latest/reference/commands/labelconvert.html
Changes proposed in this pull request
LabelConvert