-
Notifications
You must be signed in to change notification settings - Fork 536
[ENH] Update mrtrix reconst.py EstimateFOD max_sh to be able to accept list #2990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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: 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
Thanks again for the contribution, and feel free to ask questions if anything's unclear.
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 to fix the test:
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 @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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will merge when tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 .zenodo.json file. This will list you as a contributor on our Zenodo releases.
Thanks very much!
Uh oh!
There was an error while loading. Please reload this page.
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