-
Notifications
You must be signed in to change notification settings - Fork 533
ENH: AFNI (3d)LocalBistat interface #2590
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
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 couple issues with the input spec, and some suggestions on formatting.
nipype/interfaces/afni/utils.py
Outdated
@@ -1338,6 +1338,124 @@ def _list_outputs(self): | |||
return outputs | |||
|
|||
|
|||
class LocalBistatInputSpec(AFNICommandInputSpec): | |||
in_files = InputMultiPath( |
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.
This should just be traits.List
, not InputMultiPath
.
And out of curiosity, is it more likely that people will want to pass in a list of two files or want separate names for each file (e.g. in_file1
, in_file2
)?
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.
may be separate names are more suitable, you are right
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.
Is the plan to split this into two traits, or leave it as 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.
sorry I forgot this, I am doing it
nipype/interfaces/afni/utils.py
Outdated
'jointent', 'hellinger', 'crU', 'crM', 'crA', 'L2slope', | ||
'L1slope', 'num', 'ALL'] | ||
stat = traits.Either( | ||
traits.Enum(*_stat_names), traits.List(traits.Enum(_stat_names)), |
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.
Does this not work?
stat = traits.MultiPath(traits.Enum(_stat_names), ...)
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 am not sure I understand: traits does not seem to have a `MultiPath
AttributeError: 'module' object has no attribute 'MultiPath'`
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.
Oh, sorry. I meant InputMultiPath
, not traits.MultiPath
. I don't know if I copy-pasted or actually wrote that out.
nipype/interfaces/afni/utils.py
Outdated
'\'SPHERE\', \'RHDD\' (rhombic dodecahedron), \'TOHD\' ' | ||
'(truncated octahedron) with a given radius in mm or ' | ||
'\'RECT\' (rectangular block) with dimensions to specify in mm.', | ||
argstr='-nbhd %s') |
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.
How about:
argstr='-nbhd %s(%s)'
Related to a comment below.
nipype/interfaces/afni/utils.py
Outdated
"'{0}({1})'".format(region_name, ','.join(region_size))) | ||
else: | ||
return spec.argstr % ( | ||
"'{0}({1})'".format(region_name, region_size)) |
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.
With the argstr='-nbhd %s(%s)'
as suggested above, you could make this whole block:
if name == 'neighborhood' and value[0] == 'RECT':
value = ('RECT', '%s,%s,%s' % value[1])
(Letting value
fall through into the super
call.)
desc='File name of an image to use as a weight. Only applies to ' | ||
'\'pearson\' statistics.', | ||
argstr='-weight %s', | ||
xor=['automask']) |
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.
You have two mask_file
traits.
nipype/interfaces/afni/utils.py
Outdated
'will have its statistic(s) computed as zero (0).', | ||
argstr='-mask %s') | ||
automask = traits.Bool( | ||
desc='Compute the mask as in program 3dAutomask.') |
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.
Should there be an argstr
here?
nipype/interfaces/afni/utils.py
Outdated
if isinstance(value, (str, bytes)): | ||
return spec.argstr % value | ||
else: | ||
return ' '.join([spec.argstr % v for v in value]) |
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 pretty sure you can drop this if name == 'stat'
block and get the same behavior.
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 pretty sure you can drop this if name == 'stat' block and get the same behavior.
this doesn't seem to work: it doesn't repeat the option '-stat' before each value
bistat.inputs.stat = ['pearson', 'ALL']
gives -stat pearson ALL
instead of -stat pearson -stat ALL
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.
@salma1601 - if you change the argstr
above to '-stat %s...'
it will do what you are looking for.
Codecov Report
@@ Coverage Diff @@
## master #2590 +/- ##
=========================================
+ Coverage 67.57% 67.6% +0.02%
=========================================
Files 335 336 +1
Lines 42630 42699 +69
Branches 5270 5278 +8
=========================================
+ Hits 28807 28865 +58
- Misses 13150 13157 +7
- Partials 673 677 +4
Continue to review full report at Codecov.
|
Hi @salma1601, not to rush you, but just a heads up that we should try to get this in by Friday (EDT) if we want it in 1.0.4. |
nipype/interfaces/afni/utils.py
Outdated
' map that size.' | ||
' * ALL = all of the above, in that order' | ||
'More than one option can be used.', | ||
argstr='-stat %s') |
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.
try '-stat %s...'
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.
yes it works, thanks !
try add interface for
3dLocalBistat