-
Notifications
You must be signed in to change notification settings - Fork 533
[FIX] fix afni.allineate interface #2502
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
Codecov Report
@@ Coverage Diff @@
## master #2502 +/- ##
==========================================
+ Coverage 66.87% 66.87% +<.01%
==========================================
Files 327 327
Lines 42448 42439 -9
Branches 5266 5263 -3
==========================================
- Hits 28387 28382 -5
- Misses 13362 13365 +3
+ Partials 699 692 -7
Continue to review full report at Codecov.
|
Suggested changes to nipy#2502
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.
I'm tagging the changes I suggested in vanandrew#1 here.
I also made a couple other changes I saw while looking into this. This interface can be substantially reduced.
nipype/interfaces/afni/preprocess.py
Outdated
@@ -219,6 +219,7 @@ class AllineateInputSpec(AFNICommandInputSpec): | |||
desc='output file from 3dAllineate', | |||
argstr='-prefix %s', | |||
genfile=True, | |||
hash_files=False, |
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.
Good catch.
nipype/interfaces/afni/preprocess.py
Outdated
@@ -219,6 +219,7 @@ class AllineateInputSpec(AFNICommandInputSpec): | |||
desc='output file from 3dAllineate', | |||
argstr='-prefix %s', | |||
genfile=True, |
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.
We can replace genfile
with the newer name_template
.
- genfile=True,
+. name_template='%s_allineate',
+. name_source='in_file',
In the PR I sent you, I've resolved the issue with it not respecting the xor
tag.
nipype/interfaces/afni/preprocess.py
Outdated
|
||
if self.inputs.out_weight_file: | ||
if isdefined(self.inputs.out_weight_file): |
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.
Undefined
evaluates to False
, so this isn't necessary, here.
nipype/interfaces/afni/preprocess.py
Outdated
@@ -519,7 +525,7 @@ def _list_outputs(self): | |||
|
|||
def _gen_filename(self, name): | |||
if name == 'out_file': | |||
return self._list_outputs()[name] | |||
return self._gen_outfilename() | |||
return None |
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.
We should be able to remove this method entirely, if we're switching to name_template
/name_source
.
Fast merge. 😃 |
>>> allineate.inputs.reference = 'structural.nii' | ||
>>> allineate.inputs.nwarp_fixmot = ['X', 'Y'] | ||
>>> allineate.cmdline | ||
'3dAllineate -source functional.nii -nwarp_fixmotX -nwarp_fixmotY -prefix functional_allineate -base structural.nii' |
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.
A question: Is -prefix functional_allineate
the correct default, or should it be functional_allineate.nii
?
If it should have the extension, we should add keep_extension=True
to the out_file
trait spec.
@tsalo @salma1601 You might also be good people to chime in here.
# Do not generate filename when excluded by other inputs | ||
if trait_spec.xor and any(isdefined(getattr(self.inputs, field)) | ||
for field in trait_spec.xor): | ||
return retval |
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.
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.
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.
Ah, right. I had it kind of backwards, but a similar fix to this should resolve #2506. I'll merge and propose a quick PR for that one.
Ah, sorry. Forgot to |
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.
Thanks. Will merge in 24 hours unless someone spots something and makes noise.
Suggested changes to nipy/nipype#2502
Fixes #2499, #2216
Changes proposed in this pull request
-See linked issue #2499