-
Notifications
You must be signed in to change notification settings - Fork 536
Fix typo in spm.Normalize12 process #3599
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
Concerning Normalise12: 1- deformation_file and image_to_align are mutually exclusive. So we can't use the mandatory=True option for these two traits, as one of them will always be Undefined. 2- the self.inputs.jobtype == "estimate" statement is useless because the jobtype trait can only take values in ["est", "write", "estwrite"].
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@ ## master #3599 +/- ## ======================================= Coverage 63.17% 63.18% ======================================= Files 308 308 Lines 40813 40809 -4 Branches 5654 5652 -2 ======================================= Hits 25784 25784 + Misses 14016 14013 -3 + Partials 1013 1012 -1
☔ View full report in Codecov by Sentry. |
mandatory=True is fine for mutually exclusive options. It means there will be an error if neither is defined. Have you ever had a problem?
But the other fix looks good.
We never use nipype directly, we use it in the populse_mia application.
In this application, there's a method that checks whether an input trait with the mandatory=True option has a defined value. If it doesn't, this indicates that a mandatory parameter has not been filled in, which blocks the calculation...
Ok, I've just run nipype directly, with deformation_file as Udefined and I can't see any issue ...
$ python
Python 3.9.9 (main, Nov 19 2021, 00:00:00)
[GCC 10.3.1 20210422 (Red Hat 10.3.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import nipype.interfaces.spm as spm
>>> norm12 = spm.Normalize12()
>>> norm12.inputs.jobtype = 'est'
>>> norm12.inputs.image_to_align = '/home/SPM_Matlab_tests_normalize/directlyInnipype/anat.nii'
>>> norm12.run()
<nipype.interfaces.base.support.InterfaceResult object at 0x7f1d771570a0>
Well, it's on our side that we need to change the way we check if there's a missing mandatory parameter!
So I'm going to cancel the modification for the mandatory trait option, and keep only the unnecessary part.
...ion_file in spm process Normalize12
Done!
Uh oh!
There was an error while loading. Please reload this page.
Please consider this PR that will be fast to review:
1-
deformation_fileandimage_to_alignare mutually exclusive. So we can't use themandatory=Trueoption for these two traits, as one of them will always be Undefined.2- the
self.inputs.jobtype == "estimate"statement is useless because thejobtypetrait can only take values in ["est", "write", "estwrite"].For point 2- it's not very important, it doesn't stop the run, it's just useless.
For point 1-, it's critical because the condition for one or the other can never be met, and this blocks the use of this process in our application, which uses nipype.