FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect

[GalKepler]

Resolves #3247 .

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

[effigies]

I think this may be a change in CLI...

This line shows:

  app.warn('Use of -fsl option in dwibiascorrect script is discouraged due to its strong dependence ' + \
           'on brain masking (specifically its inability to correct voxels outside of this mask).' + \
           'Use of the -ants option is recommended for quantitative DWI analyses.')

If this has changed, then we need to adjust our behavior based on the version of MRtrix3 installed.

[effigies]

If I'm reading it correctly, MRtrix3/mrtrix3#1449 changed from -ants and -fsl flags to ants and fsl subcommands, which means this behavior is new as of MRtrix3 3.0. @Lestropie is that correct?

Given that at least some MRtrix3 commands are Python, I wonder if there's a cleaner way to invoke these through a function API, rather than subprocesses. Though that also may have changed.

[GalKepler]

@effigies I think you're right, but I wonder whether to create a version-specific inputspec, or simply change the relevant trait in accordance with the user's Mrtrix3's version?
I've also posted a message in Mrtrix3's forum to try and verify the exact version in which the change occurred.

[Lestropie]

MRtrix3/mrtrix3#1449 changed from -ants and -fsl flags to ants and fsl subcommands, which means this behavior is new as of MRtrix3 3.0.

Correct; they changed from command-line options to using the MRtrix3 algorithm submodule (essentially subcommands), as this is more consistent with other MRtrix3 Python scripts. This also means that the algorithm name needs to be the second command-line argument, whereas previously the command-line option was not positional.

Given that at least some MRtrix3 commands are Python, I wonder if there's a cleaner way to invoke these through a function API, rather than subprocesses. Though that also may have changed.

Given the degree to which the MRtrix3 Python API is custom and very much nonstandard, I suspect not; at least not without essentially duplicating the functionality of the dwibiascorrect wrapper script. The majority of MRtrix3 is C++ terminal commands, so when Python scripts started to get added they were designed around the same philosophy.

I've also posted a message in Mrtrix3's forum to try and verify the exact version in which the change occurred.

The change is from 3.0_RC3 to 3.0.0. There's a couple of other tags (3.0_RC3_latest, 3.0_RC3_last, 3.0_RC4) that were not actually intended for public use. The relevant change is present in 3.0_RC4; but I'd advise avoiding that tag anyway, it was somewhat erroneous to include that tag at all on the main repo.

[Lestropie]

Also MRtrix3/mrtrix3#2171 may have contributed to the confusion; sorry about that.

[effigies]

Okay, then I think we want to add:

class DWIBiasCorrect(MRTrix3Base):
    ...
    def _format_arg(self, name, trait_spec, value):
        if name in ('use_ants', 'use_fsl'):
            ver = self.version
            # Changed in version 3.0, after release candidates
            if ver is not None and (ver[0] < '3' or ver.startswith("3.0_RC")):
                return f"-{trait_spec.argstr}"
        super()._format_arg(name, trait_spec, value)

And we need to update the doctest to say dwibiascorrect ants dwi.mif dwi_biascorr.mif.

[effigies]

@GalBenZvi Do you have time to finish this one off? I think we're probably due for a release soon, and it would be nice to have this in.

[GalKepler]

@effigies I'm sorry, I was under the impression that it was finished, could you maybe explain to me what do you want me to do? Obviously I would love to contribute.

[effigies]

Sure, my previous review had a number of suggestions. If that doesn't seem actionable, I can send a PR against your branch with my suggested changes.

[GalKepler]

Sure, I'll do my best to get to it as soon as possible. Sorry for the delay.

[codecov]

Codecov Report

Merging #3248 (68b3003) into master (3976524) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3248      +/-   ##
==========================================
- Coverage   65.02%   64.98%   -0.04%     
==========================================
  Files         302      302              
  Lines       39941    39947       +6     
  Branches     5281     5283       +2     
==========================================
- Hits        25973    25961      -12     
- Misses      12902    12914      +12     
- Partials     1066     1072       +6     

Flag	Coverage Δ
unittests	64.91% <66.66%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/preprocess.py	85.29% <66.66%> (-1.17%)	⬇️
nipype/pipeline/plugins/legacymultiproc.py	65.68% <0.00%> (-5.40%)	⬇️
nipype/pipeline/plugins/base.py	58.08% <0.00%> (-1.65%)	⬇️
nipype/interfaces/mrtrix3/base.py	59.64% <0.00%> (+1.75%)	⬆️

---

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 3976524...68b3003. Read the comment docs.

[effigies]

Okay, this looks all set. Thanks for this addition!