FIX: Remove 'reg_term'/'regularisation' from dwi2tensor interface #2731

ihnorton · 2018-10-10T21:38:13Z

Summary

remove regularisation (reg_term) option from MRtrix3 dwi2tensor interface.

This option was removed as part of refactoring in:

MRtrix3/mrtrix3@9b886a94289a

Fixes

dwi2tensor: [ERROR] unknown option "-regularisation"

Acknowledgment

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

codecov-io · 2018-10-11T01:47:57Z

Codecov Report

Merging #2731 into master will decrease coverage by 3.42%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##           master    #2731      +/-   ##
==========================================
- Coverage   67.45%   64.02%   -3.43%     
==========================================
  Files         340      338       -2     
  Lines       43155    43119      -36     
  Branches     5351     5350       -1     
==========================================
- Hits        29111    27609    -1502     
- Misses      13346    14443    +1097     
- Partials      698     1067     +369

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.02% <47.05%> (-0.81%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/reconst.py	`79.59% <ø> (ø)`	⬆️
nipype/interfaces/mrtrix3/base.py	`56.36% <47.05%> (-6.14%)`	⬇️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/freesurfer/base.py	`49.59% <0%> (-30.9%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35.39% <0%> (-29.21%)`	⬇️
nipype/interfaces/spm/base.py	`58.41% <0%> (-29.05%)`	⬇️
nipype/utils/provenance.py	`55.73% <0%> (-28.99%)`	⬇️
nipype/interfaces/fsl/model.py	`55.26% <0%> (-25.35%)`	⬇️
... and 43 more

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 4f2933e...98faffa. Read the comment docs.

effigies · 2018-10-11T13:29:14Z

Since this is still valid for older versions of mrtrix3, we shouldn't remove it altogether. You can make the input spec depend on the version of the library. See, for example, BBRegister:

nipype/nipype/interfaces/freesurfer/preprocess.py

Lines 1336 to 1428 in ffd8cf8

    
           class BBRegisterInputSpec(FSTraitedSpec): 
        
               subject_id = traits.Str( 
        
                   argstr='--s %s', desc='freesurfer subject id', mandatory=True) 
        
               source_file = File( 
        
                   argstr='--mov %s', 
        
                   desc='source file to be registered', 
        
                   mandatory=True, 
        
                   copyfile=False) 
        
               init = traits.Enum( 
        
                   'spm', 
        
                   'fsl', 
        
                   'header', 
        
                   argstr='--init-%s', 
        
                   mandatory=True, 
        
                   xor=['init_reg_file'], 
        
                   desc='initialize registration spm, fsl, header') 
        
               init_reg_file = File( 
        
                   exists=True, 
        
                   argstr='--init-reg %s', 
        
                   desc='existing registration file', 
        
                   xor=['init'], 
        
                   mandatory=True) 
        
               contrast_type = traits.Enum( 
        
                   't1', 
        
                   't2', 
        
                   'bold', 
        
                   'dti', 
        
                   argstr='--%s', 
        
                   desc='contrast type of image', 
        
                   mandatory=True) 
        
               intermediate_file = File( 
        
                   exists=True, 
        
                   argstr="--int %s", 
        
                   desc="Intermediate image, e.g. in case of partial FOV") 
        
               reg_frame = traits.Int( 
        
                   argstr="--frame %d", 
        
                   xor=["reg_middle_frame"], 
        
                   desc="0-based frame index for 4D source file") 
        
               reg_middle_frame = traits.Bool( 
        
                   argstr="--mid-frame", 
        
                   xor=["reg_frame"], 
        
                   desc="Register middle frame of 4D source file") 
        
               out_reg_file = File( 
        
                   argstr='--reg %s', desc='output registration file', genfile=True) 
        
               spm_nifti = traits.Bool( 
        
                   argstr="--spm-nii", 
        
                   desc="force use of nifti rather than analyze with SPM") 
        
               epi_mask = traits.Bool( 
        
                   argstr="--epi-mask", desc="mask out B0 regions in stages 1 and 2") 
        
               dof = traits.Enum( 
        
                   6, 9, 12, argstr='--%d', desc='number of transform degrees of freedom') 
        
               fsldof = traits.Int( 
        
                   argstr='--fsl-dof %d', 
        
                   desc='degrees of freedom for initial registration (FSL)') 
        
               out_fsl_file = traits.Either( 
        
                   traits.Bool, 
        
                   File, 
        
                   argstr="--fslmat %s", 
        
                   desc="write the transformation matrix in FSL FLIRT format") 
        
               out_lta_file = traits.Either( 
        
                   traits.Bool, 
        
                   File, 
        
                   argstr="--lta %s", 
        
                   min_ver='5.2.0', 
        
                   desc="write the transformation matrix in LTA format") 
        
               registered_file = traits.Either( 
        
                   traits.Bool, 
        
                   File, 
        
                   argstr='--o %s', 
        
                   desc='output warped sourcefile either True or filename') 
        
               init_cost_file = traits.Either( 
        
                   traits.Bool, 
        
                   File, 
        
                   argstr='--initcost %s', 
        
                   desc='output initial registration cost file') 
        
           class BBRegisterInputSpec6(BBRegisterInputSpec): 
        
               init = traits.Enum( 
        
                   'coreg', 
        
                   'rr', 
        
                   'spm', 
        
                   'fsl', 
        
                   'header', 
        
                   'best', 
        
                   argstr='--init-%s', 
        
                   xor=['init_reg_file'], 
        
                   desc='initialize registration with mri_coreg, spm, fsl, or header') 
        
               init_reg_file = File( 
        
                   exists=True, 
        
                   argstr='--init-reg %s', 
        
                   desc='existing registration file', 
        
                   xor=['init'])

nipype/nipype/interfaces/freesurfer/preprocess.py

Lines 1462 to 1465 in ffd8cf8

    
           if LooseVersion('0.0.0') < Info.looseversion() < LooseVersion("6.0.0"): 
        
               input_spec = BBRegisterInputSpec 
        
           else: 
        
               input_spec = BBRegisterInputSpec6

In this case, you'd probably want to go the other way around, with the newer version as the superclass and the older as the subclass.

Let me know if you have any questions.

satra · 2018-10-11T13:55:37Z

doesn't max_ver metadata on the trait take care of this? or does mrtrix not provide versioning info?

effigies · 2018-10-11T14:52:34Z

Ah, I'd forgotten about that one. My approach wouldn't work without version info, either.

ihnorton · 2018-10-11T15:35:45Z

I added version info to the interface (changes pushed), which seems to work.
But max_ver doesn't do what the docs seem to imply -- I get an error:

    (name, self.__class__.__name__, version, max_ver))
Exception: Trait reg_term (FitTensor) (version 0.3.15 > required 0.3.13)

ihnorton · 2018-10-11T15:40:16Z

Removing usedefault fixes the problem. I take it that needs to be made mutually-exclusive with max_ver somewhere...

satra · 2018-10-11T15:45:45Z

indeed. essentially its saying that if the environment conflicts with the use, raise an exception.

we should think about cleaner ways to handle versioning in nipype 2.0.

ihnorton · 2018-10-11T16:23:21Z

Alright, updated to do it Chris's way.

effigies

Can you also re-run make specs?

effigies · 2018-10-11T18:37:49Z

nipype/interfaces/mrtrix3/base.py

+class Info(PackageInfo):
+    version_cmd = which('mrconvert')
+    if version_cmd:
+        version_cmd = version_cmd + ' --version'


If mrconvert is in your PATH, then you can simplify this as just:

version_cmd = 'mrconvert --version'

effigies · 2018-10-11T18:44:57Z

nipype/interfaces/mrtrix3/base.py

+        if ver is None:
+            return LooseVersion('0.0.0')
+
+        return cls.version())


I assume you want:

return LooseVersion(ver)

Or shorter:

@classmethod def looseversion(cls): return LooseVersion(cls.version() or '0.0.0')

effigies

Actually, I think we can't use the auto-test if you're going to do the dual specs. I commented with the "correct" way to handle this.

Although I hate to have wasted your time, it might be simpler to revert to a single spec and remove the default value. usedefault was only set to true in #2533.

nipype/interfaces/mrtrix3/tests/test_auto_FitTensor.py

effigies

Great, thanks! Just a couple minor cleanups left.

effigies · 2018-10-11T20:32:57Z

nipype/interfaces/mrtrix3/reconst.py

 from ..base import traits, TraitedSpec, File, Undefined
-from .base import MRTrix3BaseInputSpec, MRTrix3Base
+from .base import MRTrix3BaseInputSpec, MRTrix3Base, Info


No longer using the Info and LooseVersion imports.

effigies · 2018-10-11T20:34:19Z

nipype/interfaces/mrtrix3/reconst.py

@@ -39,8 +40,9 @@ class FitTensorInputSpec(MRTrix3BaseInputSpec):
        argstr='-method %s',
        desc=('select method used to perform the fitting'))
    reg_term = traits.Float(
-        5.e3, usedefault=True,
+        5.e3,


Could you go ahead and remove the 5.e3? The reason usedefault=True was added was because it looked like a default was intended by the original interface author. If we're not going to apply defaults, we should remove it.

effigies · 2018-10-11T21:15:34Z

Also please merge or rebase against master to fix tests.

- mrtrix3: use looseversion to avoid undefined version - Address review comment and update auto_FitTensor test - mrtrix3.FitTensor: switch back to single spec, usedefault=False

ihnorton force-pushed the rmv_dwi2tensor_reg_term branch from 26ad945 to 67cfe04 Compare October 11, 2018 16:23

effigies reviewed Oct 11, 2018

View reviewed changes

nipype/interfaces/mrtrix3/tests/test_auto_FitTensor.py Show resolved Hide resolved

effigies reviewed Oct 11, 2018

View reviewed changes

effigies added this to the 1.1.4 milestone Oct 11, 2018

Make mrtrix3 FitTensor reg_term conditional on version

286eecf

- mrtrix3: use looseversion to avoid undefined version - Address review comment and update auto_FitTensor test - mrtrix3.FitTensor: switch back to single spec, usedefault=False

ihnorton force-pushed the rmv_dwi2tensor_reg_term branch from 264a8b6 to 286eecf Compare October 12, 2018 02:19

Fix doctest

98faffa

effigies merged commit 3ec8065 into nipy:master Oct 12, 2018

ihnorton deleted the rmv_dwi2tensor_reg_term branch October 12, 2018 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Remove 'reg_term'/'regularisation' from dwi2tensor interface #2731

FIX: Remove 'reg_term'/'regularisation' from dwi2tensor interface #2731

ihnorton commented Oct 10, 2018 •

edited

Loading

codecov-io commented Oct 11, 2018 •

edited

Loading

effigies commented Oct 11, 2018 •

edited

Loading

satra commented Oct 11, 2018

effigies commented Oct 11, 2018

ihnorton commented Oct 11, 2018

ihnorton commented Oct 11, 2018 •

edited

Loading

satra commented Oct 11, 2018

ihnorton commented Oct 11, 2018

effigies left a comment

effigies Oct 11, 2018

effigies Oct 11, 2018

effigies left a comment

effigies left a comment

effigies Oct 11, 2018

effigies Oct 11, 2018

effigies commented Oct 11, 2018

FIX: Remove 'reg_term'/'regularisation' from dwi2tensor interface #2731

FIX: Remove 'reg_term'/'regularisation' from dwi2tensor interface #2731

Conversation

ihnorton commented Oct 10, 2018 • edited Loading

Summary

Acknowledgment

codecov-io commented Oct 11, 2018 • edited Loading

Codecov Report

effigies commented Oct 11, 2018 • edited Loading

satra commented Oct 11, 2018

effigies commented Oct 11, 2018

ihnorton commented Oct 11, 2018

ihnorton commented Oct 11, 2018 • edited Loading

satra commented Oct 11, 2018

ihnorton commented Oct 11, 2018

effigies left a comment

Choose a reason for hiding this comment

effigies Oct 11, 2018

Choose a reason for hiding this comment

effigies Oct 11, 2018

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

effigies Oct 11, 2018

Choose a reason for hiding this comment

effigies Oct 11, 2018

Choose a reason for hiding this comment

effigies commented Oct 11, 2018

ihnorton commented Oct 10, 2018 •

edited

Loading

codecov-io commented Oct 11, 2018 •

edited

Loading

effigies commented Oct 11, 2018 •

edited

Loading

ihnorton commented Oct 11, 2018 •

edited

Loading