Skip to content
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

Fix typo in spm.Normalize12 process #3599

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Fix typo in spm.Normalize12 process #3599

merged 2 commits into from
Sep 1, 2023

Conversation

servoz
Copy link
Collaborator

@servoz servoz commented Sep 1, 2023

Please consider this PR that will be fast to review:

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"].

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.

	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
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (03a2363) 63.17% compared to head (63689e4) 63.18%.

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     
Files Changed Coverage Δ
nipype/interfaces/spm/preprocess.py 49.52% <0.00%> (+0.18%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member

effigies commented Sep 1, 2023

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.

@servoz
Copy link
Collaborator Author

servoz commented Sep 1, 2023

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.

@servoz
Copy link
Collaborator Author

servoz commented Sep 1, 2023

Done!

@effigies effigies merged commit 83d52f9 into nipy:master Sep 1, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants