ENH: Adds interfaces for MRtrix utils shconv and sh2amp #3280

tclose · 2020-12-02T10:24:31Z

Summary

This PR adds the predicted signal output option of MRtrix's dwi2tensor two interfaces to mrtrix3.utils: SHConv and SH2Amp, which wrap the MRtrix3 (>v3.0.0) commands shconv and sh2amp respectively. Together these can be useful for calculating the residual signal after fitting tensor or ODF models.

List of changes proposed in this PR (pull-request)

Adds predicted_signal output to FitTensor interface
Adds mrtrix3.SHConv interface
Adds mrtrix3.SH2Amp interface

Acknowledgment

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

added predicted signal to estimate output spec

… mrtrixbase

effigies

Overall looks good. Some small comments.

effigies · 2020-12-02T17:19:17Z

nipype/interfaces/mrtrix3/reconst.py

@@ -187,6 +203,9 @@ def _list_outputs(self):
            outputs["gm_odf"] = op.abspath(self.inputs.gm_odf)
        if self.inputs.csf_odf != Undefined:
            outputs["csf_odf"] = op.abspath(self.inputs.csf_odf)
+        if self.inputs.predicted_signal != Undefined:


I assume that you're being consistent with the rest of this function. If you feel the urge, you can fix up the others, but I think we should do this the standard way.

Suggested change

if self.inputs.predicted_signal != Undefined:

if isdefined(self.inputs.predicted_signal):

On this interface, I remembered that 'predicted_signal' is only a valid option when using the 'msmt_csd' algorithm. Is there a way to specify this in the input spec?

effigies · 2020-12-02T17:31:36Z

nipype/interfaces/mrtrix3/utils.py

+    >>> sh.inputs.in_file = 'odf.mif'
+    >>> sh.inputs.response = 'response.txt'


These files should exist in https://github.com/nipy/nipype/tree/master/nipype/testing/data. If there isn't already one with a name that works for this tool, feel free to create an empty file with that name.

effigies · 2020-12-02T17:32:00Z

nipype/interfaces/mrtrix3/utils.py

+    >>> sh = mrt.SHConv()
+    >>> sh.inputs.in_file = 'odf.mif'
+    >>> sh.inputs.response = 'response.txt'
+    >>> sh.cmdline                               # doctest: +ELLIPSIS


No ... appears in the output line.

Suggested change

>>> sh.cmdline # doctest: +ELLIPSIS

>>> sh.cmdline

nipype/interfaces/mrtrix3/utils.py

effigies · 2020-12-02T17:32:46Z

nipype/interfaces/mrtrix3/utils.py

+    >>> sh = mrt.SH2Amp()
+    >>> sh.inputs.in_file = 'sh.mif'
+    >>> sh.inputs.directions = 'grads.txt'
+    >>> sh.cmdline                               # doctest: +ELLIPSIS


Suggested change

>>> sh.cmdline # doctest: +ELLIPSIS

>>> sh.cmdline

effigies · 2020-12-02T17:47:41Z

Also can you run black on the files you've touched and make specs?

for correct algorithm when using predicted_signal

tclose · 2020-12-02T22:28:46Z

Ok, I think I have made all the changes you requested now

codecov · 2020-12-02T23:33:14Z

Codecov Report

Merging #3280 (e096c19) into master (5d2e224) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3280      +/-   ##
==========================================
+ Coverage   65.07%   65.08%   +0.01%     
==========================================
  Files         307      307              
  Lines       40323    40362      +39     
  Branches     5326     5329       +3     
==========================================
+ Hits        26240    26270      +30     
- Misses      13010    13019       +9     
  Partials     1073     1073

Flag	Coverage Δ
unittests	`64.80% <66.66%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/__init__.py	`100.00% <ø> (ø)`
nipype/interfaces/mrtrix3/reconst.py	`75.00% <38.46%> (-6.49%)`	⬇️
nipype/interfaces/mrtrix3/utils.py	`83.00% <79.31%> (-0.61%)`	⬇️
nipype/__init__.py	`53.19% <0.00%> (+6.38%)`	⬆️

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 5d2e224...e096c19. Read the comment docs.

effigies

Regarding msmt_csd, please see #3176 and #3004.

I think what we want to do is leave EstimateFOD alone and improve ConstrainedSphericalDeconvolution, since that one is capable of doing something other than msmt_csd. This would encourage people to move to the more "correct" interface to get the full feature set.

Some small suggestions, but otherwise this is looking good.

nipype/interfaces/mrtrix3/reconst.py

nipype/interfaces/mrtrix3/utils.py

effigies · 2021-04-30T21:56:14Z

Hey @tclose just a reminder that there are unresolved suggestions here.

Co-authored-by: Chris Markiewicz <[email protected]>

tclose · 2021-05-06T01:25:01Z

Sorry, I think I was on leave when they came through and they slipped of the radar.

effigies

Just about there, @tclose. Can you merge master, as well, to get any bug fixes from there?

nipype/interfaces/mrtrix3/utils.py

effigies · 2021-05-27T12:05:25Z

I'm going to try to make a release in the next couple days. If you want to try to finish this off, I'd love to include it!

Co-authored-by: Chris Markiewicz <[email protected]>

tclose · 2021-06-11T10:18:17Z

Sorry to miss your release, I'm just getting back into this now. Hopefully it passes the checks now (I'm off to bed:).

On a very tangential note, I have created a new Pydra task repository for Dcm2niix at tclose/pydra-dcm2niix, which is nearly ready to go I think. Would it be good to migrate it into the nipype org, and if so, which is the best way to do this?

effigies · 2021-06-15T20:50:02Z

Oh, hey Tom. We didn't actually release due to some failing checks, so I'll try to get this in.

Re pydra-dcm2niix, would you mind opening an issue on https://github.com/nipype/pydra-tasks-template/issues? We don't really have a process for transitioning projects into nipype control, so this would be a good one to work through manually. At some point, it would be nice to have something like a conda-forge/feedstocks setup. (cc @satra I think this is what we discussed?)

tclose · 2021-06-16T00:56:30Z

Ok great, so it is all good to go now?

effigies · 2021-06-16T01:05:56Z

Looks like it! In it goes.

tclose added 4 commits December 2, 2020 21:20

added predicted_signal option to tensor

1f3d251

added predicted signal to estimate output spec

added SHConv and SH2Amp interfaces to mrtrix utils

a9a8904

added name to authors list

ac7490c

touched up docs and made commands inherit from commandline instead of…

974ca38

… mrtrixbase

effigies reviewed Dec 2, 2020

View reviewed changes

tclose added 4 commits December 3, 2020 09:16

changed isdefined checks of EstimateFOD and check

ccd25c6

for correct algorithm when using predicted_signal

fixed up doctest strings and added dummy files

f245ba4

run black over touched files

c81259b

ran make specs

ab96460

fixed up doctest failure

cd4bbc6

effigies reviewed Dec 6, 2020

View reviewed changes

tclose and others added 9 commits May 6, 2021 11:13

Update nipype/interfaces/mrtrix3/reconst.py

35a762f

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

7e670ff

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

8671983

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

308436d

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

f4eefeb

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

70e3bcf

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

a6e440d

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

5266a7c

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

c5d7ffe

Co-authored-by: Chris Markiewicz <[email protected]>

effigies mentioned this pull request May 26, 2021

REL: 1.6.1 #3336

Merged

14 tasks

effigies reviewed May 26, 2021

View reviewed changes

nipype/interfaces/mrtrix3/utils.py Outdated Show resolved Hide resolved

nipype/interfaces/mrtrix3/utils.py Outdated Show resolved Hide resolved

nipype/interfaces/mrtrix3/utils.py Outdated Show resolved Hide resolved

nipype/interfaces/mrtrix3/utils.py Outdated Show resolved Hide resolved

tclose and others added 2 commits June 11, 2021 20:10

Update nipype/interfaces/mrtrix3/utils.py

fd8cc40

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

76ccb77

Co-authored-by: Chris Markiewicz <[email protected]>

tclose and others added 3 commits June 11, 2021 20:10

Update nipype/interfaces/mrtrix3/utils.py

af8e01f

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/utils.py

f698f4b

Co-authored-by: Chris Markiewicz <[email protected]>

Merge branch 'master' of github.com:nipy/nipype into predicted_signal

5e5ffbf

TEST: make specs

e096c19

effigies merged commit 935d209 into nipy:master Jun 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Adds interfaces for MRtrix utils shconv and sh2amp #3280

ENH: Adds interfaces for MRtrix utils shconv and sh2amp #3280

tclose commented Dec 2, 2020

effigies left a comment

effigies Dec 2, 2020

tclose Dec 2, 2020

effigies Dec 2, 2020

effigies Dec 2, 2020

effigies Dec 2, 2020

effigies commented Dec 2, 2020

tclose commented Dec 2, 2020

codecov bot commented Dec 2, 2020 •

edited

Loading

effigies left a comment

effigies commented Apr 30, 2021

tclose commented May 6, 2021

effigies left a comment

effigies commented May 27, 2021

tclose commented Jun 11, 2021

effigies commented Jun 15, 2021

tclose commented Jun 16, 2021

effigies commented Jun 16, 2021

	if self.inputs.predicted_signal != Undefined:
	if isdefined(self.inputs.predicted_signal):

		>>> sh.inputs.in_file = 'odf.mif'
		>>> sh.inputs.response = 'response.txt'

ENH: Adds interfaces for MRtrix utils shconv and sh2amp #3280

ENH: Adds interfaces for MRtrix utils shconv and sh2amp #3280

Conversation

tclose commented Dec 2, 2020

Summary

List of changes proposed in this PR (pull-request)

Acknowledgment

effigies left a comment

Choose a reason for hiding this comment

effigies Dec 2, 2020

Choose a reason for hiding this comment

tclose Dec 2, 2020

Choose a reason for hiding this comment

effigies Dec 2, 2020

Choose a reason for hiding this comment

effigies Dec 2, 2020

Choose a reason for hiding this comment

effigies Dec 2, 2020

Choose a reason for hiding this comment

effigies commented Dec 2, 2020

tclose commented Dec 2, 2020

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

effigies left a comment

Choose a reason for hiding this comment

effigies commented Apr 30, 2021

tclose commented May 6, 2021

effigies left a comment

Choose a reason for hiding this comment

effigies commented May 27, 2021

tclose commented Jun 11, 2021

effigies commented Jun 15, 2021

tclose commented Jun 16, 2021

effigies commented Jun 16, 2021

codecov bot commented Dec 2, 2020 •

edited

Loading