[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990

lucindasisk · 2019-08-06T17:58:53Z

Summary

Change max_sh in EstimateFOD to InputMultiObject, so that one can pass a list of lmax values when using a multishell model; followed format of lmax variable from ResponseSD.

Fixes # .

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

Import InputMultiObject from ..base
Change format of EstimateFOD to InputMultiObject so one can input a list of lmax values

Acknowledgment

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

Change max_sh to InputMultiObject, so that one can pass a list of lmax values when using a multishell model.

effigies · 2019-08-07T16:06:23Z

Hi @lucindasisk, thanks for the contribution!

It looks like the tests are failing. If we check one, we see:

        for key, metadata in list(input_map.items()):
            for metakey, value in list(metadata.items()):
>               assert getattr(inputs.traits()[key], metakey) == value
E               AssertionError: assert '-lmax %s' == '-lmax %d'
E                 - -lmax %s
E                 ?        ^
E                 + -lmax %d
E                 ?        ^

/home/travis/build/nipy/nipype/nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py:95: AssertionError

This is an auto-test, which exists to make sure that the the specifications don't change unintentionally, and it's detected that you changed the argstr from '-lmax %d' to '-lmax %s'. These can be updated with make specs when the change is intentional.

But first, did you check whether it was okay to keep it as %d? I'm not positive of whether we need to switch to %s to handle lists.

Once you're satisfied that the argstr is correct, go ahead and run make specs, and that will fix the test failure.

Second, it might be useful for users to see the use of a multiple max_shs. Could you add a second example to the doctest demonstrating the use of multi-shell response? These examples are also tested, so it will make sure the command line looks as expected.

nipype/nipype/interfaces/mrtrix3/reconst.py

Lines 141 to 157 in 0c1a4cd

    
           class EstimateFOD(MRTrix3Base): 
        
               """ 
        
               Estimate fibre orientation distributions from diffusion data using spherical deconvolution 
        
               Example 
        
               ------- 
        
               >>> import nipype.interfaces.mrtrix3 as mrt 
        
               >>> fod = mrt.EstimateFOD() 
        
               >>> fod.inputs.algorithm = 'csd' 
        
               >>> fod.inputs.in_file = 'dwi.mif' 
        
               >>> fod.inputs.wm_txt = 'wm.txt' 
        
               >>> fod.inputs.grad_fsl = ('bvecs', 'bvals') 
        
               >>> fod.cmdline                               # doctest: +ELLIPSIS 
        
               'dwi2fod -fslgrad bvecs bvals -lmax 8 csd dwi.mif wm.txt wm.mif gm.mif csf.mif' 
        
               >>> fod.run()                                 # doctest: +SKIP 
        
               """

Thanks again for the contribution, and feel free to ask questions if anything's unclear.

lucindasisk · 2019-08-12T20:28:32Z

Thank you so much for these comments! I don't believe it is necessary to replace %d with %s. I'll submit another pull request with this reverted. Thanks again!

…

On Wed, Aug 7, 2019 at 12:06 PM Chris Markiewicz ***@***.***> wrote: Hi @lucindasisk <https://github.com/lucindasisk>, thanks for the contribution! It looks like the tests <https://travis-ci.org/nipy/nipype/builds/568486689> are failing. If we check one <https://travis-ci.org/nipy/nipype/jobs/568486692#L7045-L7052>, we see: for key, metadata in list(input_map.items()): for metakey, value in list(metadata.items()):> assert getattr(inputs.traits()[key], metakey) == value E AssertionError: assert '-lmax %s' == '-lmax %d' E - -lmax %s E ? ^ E + -lmax %d E ? ^ /home/travis/build/nipy/nipype/nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py:95: AssertionError This is an auto-test, which exists to make sure that the the specifications don't change *unintentionally*, and it's detected that you changed the argstr from '-lmax %d' to '-lmax %s'. These can be updated with make specs when the change is intentional. But first, did you check whether it was okay to keep it as %d? I'm not positive of whether we need to switch to %s to handle lists. Once you're satisfied that the argstr is correct, go ahead and run make specs, and that will fix the test failure. ------------------------------ Second, it might be useful for users to see the use of a multiple max_shs. Could you add a second example to the doctest demonstrating the use of multi-shell response? These examples are also tested, so it will make sure the command line looks as expected. https://github.com/nipy/nipype/blob/0c1a4cdcf2867523c6dac84ab2ca0745ca5f0d08/nipype/interfaces/mrtrix3/reconst.py#L141-L157 Thanks again for the contribution, and feel free to ask questions if anything's unclear. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2990?email_source=notifications&email_token=AG7ICOLDJNRPBEGILHEXPV3QDLXIJA5CNFSM4IJY26ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3Y45UY#issuecomment-519163603>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG7ICOLCA2TKT6EAFX6JTATQDLXIJANCNFSM4IJY26ZA> .

-- Lucinda Sisk *PhD Student, Neuroscience Division* *CANDLab, Yale University | candlab.yale.edu <http://candlab.yale.edu/>* *[email protected] <[email protected]>*

effigies · 2019-08-13T16:20:14Z

Hi @lucindasisk. We can update this PR, rather than open a new one.

I assume you have your branch of nipype cloned, but if not:

git clone [email protected]:lucindasisk/nipype.git
cd nipype

From here, you can add commits to this branch:

git checkout patch-1
git fetch origin
git checkout origin/master nipype/interfaces/mrtrix3/reconst.py
git commit -m 'Revert argstr to use %d'

You will also need to run make specs to fix the test:

make specs
git commit -m 'make specs'

Then you can push back to this PR:

git push patch-1

codecov · 2019-08-13T16:33:38Z

Codecov Report

Merging #2990 into master will decrease coverage by 0.53%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2990      +/-   ##
==========================================
- Coverage   67.59%   67.06%   -0.54%     
==========================================
  Files         344      343       -1     
  Lines       43796    43778      -18     
  Branches     5476     5473       -3     
==========================================
- Hits        29606    29360     -246     
- Misses      13473    13685     +212     
- Partials      717      733      +16

Flag	Coverage Δ
#smoketests	`50.19% <ø> (ø)`	⬆️
#unittests	`64.24% <100%> (-0.8%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/reconst.py	`79.59% <100%> (ø)`	⬆️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/testing/fixtures.py	`77.33% <0%> (-21.34%)`	⬇️
nipype/interfaces/freesurfer/base.py	`61.01% <0%> (-19.5%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon2.py	`65.9% <0%> (-9.2%)`	⬇️
nipype/interfaces/ants/base.py	`63.82% <0%> (-8.52%)`	⬇️
nipype/workflows/smri/freesurfer/ba_maps.py	`91.66% <0%> (-8.34%)`	⬇️
nipype/workflows/smri/freesurfer/recon.py	`66.66% <0%> (-7.81%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon3.py	`68.4% <0%> (-6.95%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon1.py	`41.66% <0%> (-5.1%)`	⬇️
... and 10 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 01a2772...26bf3af. Read the comment docs.

codecov · 2019-08-13T16:33:43Z

Codecov Report

Merging #2990 into master will decrease coverage by 0.78%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2990      +/-   ##
==========================================
- Coverage   67.59%   66.81%   -0.79%     
==========================================
  Files         344      343       -1     
  Lines       43796    44567     +771     
  Branches     5476     5551      +75     
==========================================
+ Hits        29606    29777     +171     
- Misses      13473    13995     +522     
- Partials      717      795      +78

Flag	Coverage Δ
#smoketests	`50.19% <ø> (ø)`	⬆️
#unittests	`64.16% <100%> (-0.88%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/reconst.py	`79.59% <100%> (ø)`	⬆️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/testing/fixtures.py	`77.33% <0%> (-21.34%)`	⬇️
nipype/interfaces/freesurfer/base.py	`61.01% <0%> (-19.5%)`	⬇️
nipype/utils/nipype2boutiques.py	`52.23% <0%> (-14.68%)`	⬇️
nipype/interfaces/ants/base.py	`61.7% <0%> (-10.64%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon2.py	`65.9% <0%> (-9.2%)`	⬇️
nipype/workflows/smri/freesurfer/ba_maps.py	`91.66% <0%> (-8.34%)`	⬇️
nipype/pipeline/engine/utils.py	`72.14% <0%> (-8%)`	⬇️
nipype/workflows/smri/freesurfer/recon.py	`66.66% <0%> (-7.81%)`	⬇️
... and 13 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 01a2772...3b1b62a. Read the comment docs.

lucindasisk · 2019-08-13T16:33:59Z

Hi Chris, Great, just did that. Sorry for the hassle/being new and thanks for bearing with me! Best, Lucinda

…

On Tue, Aug 13, 2019 at 12:20 PM Chris Markiewicz ***@***.***> wrote: Hi @lucindasisk <https://github.com/lucindasisk>. We can update this PR, rather than open a new one. I assume you have your branch of nipype cloned, but if not: git clone ***@***.***:lucindasisk/nipype.gitcd nipype From here, you can add commits to this branch: git checkout patch-1 git fetch origin git checkout origin/master nipype/interfaces/mrtrix3/reconst.py git commit -m 'Revert argstr to use %d' You will also need to run make specs to fix the test: make specs git commit -m 'make specs' Then you can push back to this PR: git push patch-1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2990?email_source=notifications&email_token=AG7ICOPENUJPRAPCHNWK7V3QELNMHA5CNFSM4IJY26ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4GGBPQ#issuecomment-520904894>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG7ICOOQRKZTVVNYJJDI33DQELNMHANCNFSM4IJY26ZA> .

-- Lucinda Sisk *PhD Student, Neuroscience Division* *CANDLab, Yale University | candlab.yale.edu <http://candlab.yale.edu/>* *[email protected] <[email protected]>*

effigies · 2019-08-16T16:58:54Z

Hi @lucindasisk, sorry I gave you the wrong advice on make specs. Try:

make specs
git add nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py
git commit -m 'TEST: Update EstimateFOD autotest'
git push

That should be all we need to finish up this one.

effigies

LGTM. Will merge when tests pass.

effigies

From the tests:

TypeError: %d format: a number is required, not str

Looks like we do need %s after all. Sorry about all the run-around. You should be able to add these two suggestions to a batch and commit them in one go from your browser. See Applying suggested changes. Otherwise, feel free to make the changes locally (and make specs) and push.

nipype/interfaces/mrtrix3/reconst.py

nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py

effigies · 2019-08-16T21:10:18Z

Hi @lucindasisk. Sorry to bug you again today, but just to let you know I'm targeting Monday for a release, and this would be good to get in if you have time.

lucindasisk · 2019-08-17T01:37:07Z

Of course, no problem! I'll submit the recommended changes shortly. Thanks again for your help!

…

On Fri, Aug 16, 2019 at 5:10 PM Chris Markiewicz ***@***.***> wrote: Hi @lucindasisk <https://github.com/lucindasisk>. Sorry to bug you again today, but just to let you know I'm targeting Monday for a release, and this would be good to get in if you have time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2990?email_source=notifications&email_token=AG7ICOIZO6U5T6A7Z2H77FDQE4JT5A5CNFSM4IJY26ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4PWUPQ#issuecomment-522152510>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG7ICONGZLAL3YYGTEZO7CDQE4JT5ANCNFSM4IJY26ZA> .

-- Lucinda Sisk *PhD Student, Neuroscience Division* *CANDLab, Yale University | candlab.yale.edu <http://candlab.yale.edu/>* *[email protected] <[email protected]>*

Co-Authored-By: Chris Markiewicz <[email protected]>

effigies · 2019-08-17T01:45:00Z

Thanks! And please feel free to add yourself to the .zenodo.json file. This will list you as a contributor on our Zenodo releases.

effigies · 2019-08-17T13:13:06Z

Thanks very much!

Update reconst.py

0c1a4cd

Change max_sh to InputMultiObject, so that one can pass a list of lmax values when using a multishell model.

Revert argstr to use %d

26bf3af

TEST: Update EstimateFOD autotest

847b423

effigies approved these changes Aug 16, 2019

View reviewed changes

effigies mentioned this pull request Aug 16, 2019

Change max_sh variable in reconst.py to accept a list; keep %d option #2997

Closed

1 task

effigies reviewed Aug 16, 2019

View reviewed changes

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

nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py Outdated Show resolved Hide resolved

lucindasisk and others added 2 commits August 16, 2019 21:40

Update nipype/interfaces/mrtrix3/reconst.py - change back to %s

4d2d260

Co-Authored-By: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/tests/test_auto_EstimateFOD.py

3b1b62a

Co-Authored-By: Chris Markiewicz <[email protected]>

effigies merged commit ea2243c into nipy:master Aug 17, 2019

effigies added this to the 1.2.1 milestone Aug 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990

[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990

Uh oh!

lucindasisk commented Aug 6, 2019 •

edited

Loading

Uh oh!

effigies commented Aug 7, 2019

Uh oh!

lucindasisk commented Aug 12, 2019 via email

Uh oh!

effigies commented Aug 13, 2019

Uh oh!

codecov bot commented Aug 13, 2019

Uh oh!

codecov bot commented Aug 13, 2019 •

edited

Loading

Uh oh!

lucindasisk commented Aug 13, 2019 via email

Uh oh!

effigies commented Aug 16, 2019

Uh oh!

effigies left a comment

Uh oh!

effigies left a comment

Uh oh!

Uh oh!

Uh oh!

effigies commented Aug 16, 2019

Uh oh!

lucindasisk commented Aug 17, 2019 via email

Uh oh!

effigies commented Aug 17, 2019

Uh oh!

effigies commented Aug 17, 2019

Uh oh!

Uh oh!

[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990

[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990

Uh oh!

Conversation

lucindasisk commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

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

Acknowledgment

Uh oh!

effigies commented Aug 7, 2019

Uh oh!

lucindasisk commented Aug 12, 2019 via email

Uh oh!

effigies commented Aug 13, 2019

Uh oh!

codecov bot commented Aug 13, 2019

Codecov Report

Uh oh!

codecov bot commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lucindasisk commented Aug 13, 2019 via email

Uh oh!

effigies commented Aug 16, 2019

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

effigies commented Aug 16, 2019

Uh oh!

lucindasisk commented Aug 17, 2019 via email

Uh oh!

effigies commented Aug 17, 2019

Uh oh!

effigies commented Aug 17, 2019

Uh oh!

Uh oh!

lucindasisk commented Aug 6, 2019 •

edited

Loading

codecov bot commented Aug 13, 2019 •

edited

Loading