Skip to content

ENH: Re-enable spm.Realign to take lists of lists of files #2409

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

Merged
merged 7 commits into from
Feb 5, 2018

Conversation

effigies
Copy link
Member

@effigies effigies commented Jan 26, 2018

Fixes #2406. Essentially reverts #2375, but using InputMultiPath instead of Either.

Changes proposed in this pull request

  • Re-enables spm.Realign to take lists of lists, using nested InputMultiPaths.

@effigies
Copy link
Member Author

Before working on these tests, is there a semantic difference between [['in_fileA.nii', 'in_fileB.nii']] and [['in_fileA.nii'], ['in_fileB.nii']], and which would be the correct interpretation of ['in_fileA.nii', 'in_fileB.nii']?

@effigies
Copy link
Member Author

@mgxd @djarecka I'm undoing your consensus here, so I'd appreciate your input.

@satra Looks like you initially implemented support for a series of 3D files for Realign, so you might understand better than us the best way to go about this.

@djarecka
Copy link
Collaborator

@effigies - i will take a look later today, but i'm still not sure how my changes could be responsible for issue with ver 0.13... but will read more about the issue.

Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure how this would affect version 0.13 - however I would prefer the interface to work as intended and raise a cryptic error message rather than vice-versa. perhaps we should also add an additional realign test that handles a list of lists as input?

@satra
Copy link
Member

satra commented Jan 30, 2018

this was the original formulation:

in_files = InputMultiPath(traits.Either(traits.List(

so as a first pass we could switch back to this.

in terms of tests we would really have to check this against the matlab file that's created, so that would be a little involved.

@effigies
Copy link
Member Author

Okay, I'll just revert #2375 for now, rather than try to be fancy and concise with nested InputMultiPaths.

@satra
Copy link
Member

satra commented Jan 30, 2018

there are a few things here:

  1. the error message that comes from InputMultiPath is correct, but doesn't reflect the ImageFileSPM error, so that should be a fix in InputMultiPath, not the SPM interface.
  2. SPM accepts a list of list of 3d nifti files where each inner list corresponds to a run. while most people use 4D files these days, the support from SPM should not be revoked.
  3. nesting InputMultiPath would be fine, if that works properly. i'm not sure that works fine. someone should check.

@effigies
Copy link
Member Author

  1. Sounds good.
  2. Agreed.
  3. I guess the failing test builds suggest that it's at least not working exactly as before, so it would likely break workflows, even if the behavior is still supported.

@effigies effigies force-pushed the enh/spm_realign_in_list_of_lists branch from aa1d482 to a84c8e8 Compare February 1, 2018 16:26
isinstance(value[0], list)):
or (isinstance(inner_trait.trait_type, traits.List) and
not isinstance(inner_trait.trait_type, InputMultiPath) and
not isinstance(value[0], list)):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify:

  • self.inner_traits() is always True for a traits.List (default value: (traits.Any(),))
  • isinstance(value, list) is always True at this point in the short-circuit
  • bool(value) is always True, or we would have returned Undefined already

These changes aren't really related to the rest of the PR, but I thought I would be modifying code in here at first, so factoring the inner_trait local variable led to this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@effigies what if value = []?

Copy link
Collaborator

@djarecka djarecka Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sorry, it would return undefined in l.331

@effigies
Copy link
Member Author

effigies commented Feb 1, 2018

New error message:

In [1]: from nipype.interfaces import spm

In [2]: realign = spm.Realign()

In [3]: realign.inputs.in_files = ['test.nii.gz', 'test.img']
---------------------------------------------------------------------------
TraitError                                Traceback (most recent call last)
<ipython-input-3-b45a253e48c2> in <module>()
----> 1 realign.inputs.in_files = [['test.nii.gz', 'test.img']]

/data/cjmarkie/Projects/nipype/nipype/interfaces/base/traits_extension.py in validate(self, object, name, value)
    345             newvalue = [value]
    346 
--> 347         value = super(MultiPath, self).validate(object, name, newvalue)
    348         # try:
    349         # except TraitError as excp:

~/.anaconda3/lib/python3.6/site-packages/traits/trait_types.py in validate(self, object, name, value)
   2334                 return value
   2335 
-> 2336             return TraitListObject( self, object, name, value )
   2337 
   2338         self.error( object, name, value )

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in __init__(self, trait, object, name, value)
   2311             except TraitError as excp:
   2312                 excp.set_prefix( 'Each element of the' )
-> 2313                 raise excp
   2314 
   2315         self.len_error( len( value ) )

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in __init__(self, trait, object, name, value)
   2303                 validate = trait.item_trait.handler.validate
   2304                 if validate is not None:
-> 2305                     value = [ validate( object, name, val ) for val in value ]
   2306 
   2307                 list.__setitem__(self, slice(0, 0), value )

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in <listcomp>(.0)
   2303                 validate = trait.item_trait.handler.validate
   2304                 if validate is not None:
-> 2305                     value = [ validate( object, name, val ) for val in value ]
   2306 
   2307                 list.__setitem__(self, slice(0, 0), value )

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in validate(self, object, name, value)
   1981             except TraitError:
   1982                pass
-> 1983         return self.slow_validate( object, name, value )
   1984 
   1985     def slow_validate ( self, object, name, value ):

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in slow_validate(self, object, name, value)
   1989             except TraitError:
   1990                pass
-> 1991         self.error( object, name, value )
   1992 
   1993     def full_info ( self, object, name, value ):

~/.anaconda3/lib/python3.6/site-packages/traits/trait_handlers.py in error(self, object, name, value)
    170         """
    171         raise TraitError( object, name, self.full_info( object, name, value ),
--> 172                           value )
    173 
    174     def full_info ( self, object, name, value ):

TraitError: Each element of the 'in_files' trait of a RealignInputSpec instance must be a list of
items which are an existing, uncompressed file (valid extensions: [.img, .nii, .hdr]) or an existing,
uncompressed file (valid extensions: [.img, .nii, .hdr]), but a value of 'test.nii.gz' <class 'str'> was
specified.

@effigies
Copy link
Member Author

effigies commented Feb 1, 2018

Last change I think makes the grammar a little smoother.

TraitError: Each element of the 'in_files' trait of a RealignInputSpec instance must be an
existing, uncompressed file (valid extensions: [.hdr, .img, .nii]) or a list of items which
are an existing, uncompressed file (valid extensions: [.hdr, .img, .nii]), but a value of
'test.nii.gz' <class 'str'> was specified.

@djarecka
Copy link
Collaborator

djarecka commented Feb 1, 2018

lgtm

@satra satra merged commit 872d749 into nipy:master Feb 5, 2018
@effigies effigies added this to the 1.0.1 milestone Feb 13, 2018
@effigies effigies deleted the enh/spm_realign_in_list_of_lists branch February 27, 2018 20:46
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.

4 participants