[FIX] Mrtrix3 usedefault issue #3004
Conversation
There was a problem hiding this comment.
This seems like it's going to break for any user that has been getting gm/csf_odf files for free up to now. Unless it's the case that this has been unusable up to now. so we won't be breaking things for anybody. I haven't used MRTrix, so I can't really tell.
Does it make sense to split the CSD algorithm into its own interface? Or perhaps we can figure out the cases under which csf_odf and gm_odf inputs should be ignored, and disable them.
@lucindasisk Would you care to look at this and give your thoughts? I assume you've been using this, given your recent edit.
And @oesteban, it looks like you added this interface in the first place. Your thoughts would be appreciated.
Suggested a couple cleanups related to default values.
|
@effigies thank you for the suggestions, I will implement them later today. The issue is that in the current form the interface is broken for anyone who tries to use the If you look at the step before the FOD computation, the response estimation, is implemented in Since it is just a matter of defining additional output names for one of the two algorithms, I think that two different interfaces are quite redundant and the result would be even more disruptive than the change here proposed. If you have any idea on how to automatically set |
Right. This is the problem. I agree that it's not a heavy fix for an actively maintained workflow, but the concern is for workflows that are not actively maintained and don't depend on a specific version of nipype. On the whole, we try not to break backwards compatibility under the current setup, unless it's clear that the feature never worked for anybody. Here it's apparently worked for people using msmt_csd. (In the future of nipype, interfaces will themselves be versioned, and design decisions can be made with at least a bit less regard for workflows in the wild.) My suggestion for an alternative interface could be done with very little code: class EstimateCSDInputSpec(EstimateFODInputSpec):
algorithm = traits.Enum('csd', ...)
gm_odf = Undefined
csf_odf = Undefined
class EstimateCSD(EstimateFOD):
input_spec = EstimateCSDInputSpecI haven't tested this, in particular whether you can get away with the To redirect CSD users to this interface, I'd update the documentation of the original to indicate that There's actually another alternative, which is that you can add an def _format_arg(self, name, trait_spec, value):
if name in ("gm_odf", "csf_odf") and self.inputs.algorithm == "csd":
return None
super(EstimateFOD, self)._format_arg(name, trait_spec, value)And you'd also need to update def _list_outputs(self):
outputs = self.output_spec().get()
outputs['wm_odf'] = op.abspath(self.inputs.wm_odf)
- if self.inputs.gm_odf != Undefined:
+ if self.inputs.gm_odf != Undefined and self.inputs.algorithm != 'csd':
outputs['gm_odf'] = op.abspath(self.inputs.gm_odf)
- if self.inputs.csf_odf != Undefined:
+ if self.inputs.csf_odf != Undefined and self.inputs.algorithm != 'csd':
outputs['csf_odf'] = op.abspath(self.inputs.csf_odf)
return outputsI don't know which is more appealing to you, but either would be fine with me. I think the former is a bit more explicit, but the latter would probably be less work. In either case, it would be good to update the doctests to show the command lines produced for both algorithms. |
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
==========================================
- Coverage 67.49% 66.96% -0.54%
==========================================
Files 344 343 -1
Lines 44038 44020 -18
Branches 5554 5551 -3
==========================================
- Hits 29723 29477 -246
- Misses 13571 13788 +217
- Partials 744 755 +11
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
==========================================
- Coverage 67.49% 66.96% -0.54%
==========================================
Files 344 343 -1
Lines 44038 44020 -18
Branches 5554 5551 -3
==========================================
- Hits 29723 29477 -246
- Misses 13571 13788 +217
- Partials 744 755 +11
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
==========================================
- Coverage 67.49% 66.96% -0.54%
==========================================
Files 344 343 -1
Lines 44038 44020 -18
Branches 5554 5551 -3
==========================================
- Hits 29723 29477 -246
- Misses 13571 13788 +217
- Partials 744 755 +11
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
==========================================
- Coverage 67.49% 66.96% -0.54%
==========================================
Files 344 343 -1
Lines 44038 44020 -18
Branches 5554 5551 -3
==========================================
- Hits 29723 29477 -246
- Misses 13571 13788 +217
- Partials 744 755 +11
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
==========================================
- Coverage 67.49% 66.96% -0.54%
==========================================
Files 344 343 -1
Lines 44038 44020 -18
Branches 5554 5551 -3
==========================================
- Hits 29723 29477 -246
- Misses 13571 13788 +217
- Partials 744 755 +11
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3004 +/- ##
=======================================
Coverage 67.22% 67.22%
=======================================
Files 343 343
Lines 44823 44823
Branches 5609 5609
=======================================
Hits 30131 30131
Misses 13936 13936
Partials 756 756
Continue to review full report at Codecov.
|
I can see your point here, but the issue is that at the moment for keeping backwards compatibility the interface to the csd algorithm is broken. I don't know if there are actual numbers on which of the algorithms is more used, but if people agree to separate the two algorithms (and if it doesn't break pipelines based on the csd algorithm), I'm happy to implement that. |
|
Right, I'm not saying we should preserve backwards compatibility for the broken part, just for the parts that are already working. If |
|
Hi @matteomancini, we're going to be aiming for a release in about a week. Please let me know if I can be any help. |
|
Hi @effigies, |
|
Hi @matteomancini, sorry to do this to you, but we just merged some pretty huge changes. Could you re-apply these changes on the latest Please let me know if you need any help or clarifications. |
|
Hi @effigies thank you for the message. I have been swamped in the last days, but this is still on my to-do list. So should I pull the latest changes, then run make specs and push to the branch on my repo clone? |
|
Hi @matteomancini. Looks like your response got drowned in a flood of notifications... Anyway, I would do the following: git fetch upstream
# Merge, but pause for fixes
git merge upstream/master --no-commit
# Get this branch's version of the changed file
git checkout be919a5 nipype/interfaces/mrtrix3/reconst.py
black nipype/interfaces/mrtrix3/reconst.py
git add nipype/interfaces/mrtrix3/reconst.py
# Inspect the differences to make sure nothing else was reverted.
git diff --cached upstream/master
# Assuming everything's good
git commit
git pushIf there are other issues, you can use If you need to install |
|
@matteomancini I added a |
Summary
Fixes #2822 .
List of changes proposed in this PR (pull-request)
useDefault=Falseforgm_odfandcsf_odfin order to avoid issues with thecsdalgorithm;useDefault=Truefor the-lmaxargument in order to avoid issues in case of multi-shell data.Acknowledgment
I acknowledge that this contribution will be available under the Apache 2 license.