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

Use out_prefix instead of "w" in spm.Normalize12 #3600

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Use out_prefix instead of "w" in spm.Normalize12 #3600

merged 2 commits into from
Sep 11, 2023

Conversation

servoz
Copy link
Collaborator

@servoz servoz commented Sep 4, 2023

Please consider this PR that will be fast to review:

I think there are two solutions.
1- Remove the out_prefix parameter and the prefix remains always at "w"
or
2- use the out_prefix parameter instead of the hard-coded "w" value.

I think the second solution is the right one.
I propose a slight modification to this effect.

Currently, for the Normalize12 process in spm, the user can define the out_prefix parameter, but this is not used to generate output, as it is hard-coded to 'w' in Normalize12._list_outputs().
@effigies
Copy link
Member

This seems reasonable, but does it work? I don't see where the updated file names would be passed to SPM to tell it to write to those files.

@servoz
Copy link
Collaborator Author

servoz commented Sep 11, 2023

Yes it is working.
Using this little change in nipype.interfaces.spm.Normalize12:

$ python
Python 3.11.4 (main, Jun  7 2023, 00:00:00) [GCC 12.3.1 20230508 (Red Hat 12.3.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import nipype.interfaces.spm as spm
>>> import os
>>> matlab_cmd = '/data/softs/SPM12standalone/spm12_r7771_Linux_R2019b/spm12/run_spm12.sh /data/softs/MATLAB_Runtime/v97 script'
>>> spm.SPMCommand.set_mlab_paths(matlab_cmd=matlab_cmd, use_mcr=True)
>>> norm12 = spm.Normalize12()
>>> norm12.inputs.jobtype = 'estwrite'
>>> norm12.inputs.image_to_align = '/home/Desktop/testNipypeNormalize12/anat.nii'
>>> norm12.inputs.out_prefix = 'test'
>>> os.listdir('/home/Desktop/testNipypeNormalize12')
['anat.nii', 'ref']
>>> norm12.run()
<nipype.interfaces.base.support.InterfaceResult object at 0x7f537d58fc10>
>>> os.listdir('/home/Desktop/testNipypeNormalize12')
['y_anat.nii', 'anat.nii', 'ref', 'testanat.nii']

I don't see where the updated file names would be passed to SPM to tell it to write to those files.

I confess I didn't look into the nipype machinery to see exactly where the updated filenames would be passed to SPM to tell it to write to those files. ... However, concretely, with the small modification I propose, it works directly with nipype or in our environment (populse).

@effigies
Copy link
Member

Sounds good. Could you go ahead and run black on this file? My guess is we have a line length issue.

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 50.00% and no project coverage change.

Comparison is base (83d52f9) 63.18% compared to head (b9cac5e) 63.18%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3600   +/-   ##
=======================================
  Coverage   63.18%   63.18%           
=======================================
  Files         308      308           
  Lines       40809    40809           
  Branches     5652     5652           
=======================================
  Hits        25784    25784           
  Misses      14013    14013           
  Partials     1012     1012           
Files Changed Coverage Δ
nipype/interfaces/spm/preprocess.py 49.52% <50.00%> (ø)

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

@servoz
Copy link
Collaborator Author

servoz commented Sep 11, 2023

Could you go ahead and run black on this file?

Done!

@effigies
Copy link
Member

Thanks!

@effigies effigies merged commit 661dd1d into nipy:master Sep 11, 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