[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990
Conversation
Change max_sh to InputMultiObject, so that one can pass a list of lmax values when using a multishell model.
|
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: AssertionErrorThis 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 But first, did you check whether it was okay to keep it as Once you're satisfied that the Second, it might be useful for users to see the use of a multiple nipype/nipype/interfaces/mrtrix3/reconst.py Lines 141 to 157 in 0c1a4cd Thanks again for the contribution, and feel free to ask questions if anything's unclear. |
|
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/>*
*lucinda.sisk@yale.edu <camila.caballero@yale.edu>*
|
|
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 git@github.com:lucindasisk/nipype.git
cd nipypeFrom 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
git commit -m 'make specs'Then you can push back to this PR: git push patch-1 |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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/>*
*lucinda.sisk@yale.edu <camila.caballero@yale.edu>*
|
|
Hi @lucindasisk, sorry I gave you the wrong advice on That should be all we need to finish up this one. |
There was a problem hiding this comment.
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.
|
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. |
|
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/>*
*lucinda.sisk@yale.edu <camila.caballero@yale.edu>*
|
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
|
Thanks! And please feel free to add yourself to the |
|
Thanks very much! |
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)
Acknowledgment