[ENH] Add MRIsCombine to FreeSurfer utils. #1948
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1948 +/- ##
=========================================
+ Coverage 72.5% 72.5% +<.01%
=========================================
Files 1063 1064 +1
Lines 54189 54219 +30
Branches 7820 7825 +5
=========================================
+ Hits 39288 39311 +23
- Misses 13682 13688 +6
- Partials 1219 1220 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1948 +/- ##
==========================================
- Coverage 72.5% 72.49% -0.02%
==========================================
Files 1063 1065 +2
Lines 54209 54268 +59
Branches 7825 7836 +11
==========================================
+ Hits 39305 39341 +36
- Misses 13682 13704 +22
- Partials 1222 1223 +1
Continue to review full report at Codecov.
|
| desc='File to be combined with in_file2') | ||
| in_file2 = File(exists=True, mandatory=True, position=2, | ||
| argstr='%s', | ||
| desc='File to be combined with in_file1') |
There was a problem hiding this comment.
Do you specifically want in_file1 and in_file2 ports? This could be made a list like so:
in_files = List(File(Exists=True), maxlen=2, minlen=2,
mandatory=True, position=1, argstr='--combinesurfs %s', desc="...")(I think. I'd check this syntax.)
There was a problem hiding this comment.
I didn't realize traits.List allowed for maxlen and minlen. I definitely prefer it your way. Just tested it and it works, so I'll commit it in a second.
| if any(self.inputs.out_file.startswith(pre) for pre in ['lh.', 'rh.']): | ||
| outputs['out_file'] = self.inputs.out_file | ||
| else: | ||
| outputs['out_file'] = 'lh.' + self.inputs.out_file |
There was a problem hiding this comment.
For your doctest example, you're going to get lh.out.stl. Is that what mris_convert produces? And if so, is that the same whether you pass lh.pial or rh.pial as in_file1?
There was a problem hiding this comment.
From what I've seen:
mris_convert --combinesurfs lh.pial rh.pial out.stlcreates lh.out.stlmris_convert --combinesurfs rh.pial lh.pial out.stlcreates lh.out.stlmris_convert --combinesurfs rh.pial rh.smoothwm out.stlcreates lh.out.stlmris_convert --combinesurfs lh.pial rh.pial rh.out.stlcreates rh.out.stl
So it doesn't seem to matter what order you pass in the files or even if both files are from the right hemisphere. As long as the output file doesn't start with lh. or rh., it will prepend lh. to the output filename.
There was a problem hiding this comment.
Haha. That's awful. Can you add a short comment explaining that this is a FreeSurfer behavior you're accommodating?
There was a problem hiding this comment.
Added an explanation to the docstring for _list_outputs in a7bafda. Should be clear now.
|
Actually, I'm going to be obnoxious. That Also, can you see if |
|
Putting in the path information worked. It wrote out the file given, with no changes. MRIMarchingCubes deals with this by using Rather than putting the path in the argstr, is there a way to reformat it in something like Whatever the fix is, it'll probably need to be implemented in other FreeSurfer functions- especially MRIMarchingCubes. |
|
Yeah, I wouldn't do it in the
def _get_filecopy_info(self):
super(MRIsCombine, self)._get_filecopy_info()
if isdefined(self.inputs.out_file):
self.inputs.out_file = os.path.abspath(self.inputs.out_file)It's not elegant, but it is the main pre-execution hook that doesn't also get called when running an interface directly. We should probably decide on a single hack, and apply it to all the interfaces, or better yet, update the |
|
I don't know how pervasive the renaming behavior is, but I think it probably only applies to functions that work on surface files. What about a new class inheriting from |
|
I took a look at the different ways different interfaces are handling the same issue.
To check the import nipype.interfaces.freesurfer as fs
jacobian = fs.Jacobian()
jacobian.inputs.in_origsurf = 'lh.pial'
jacobian.inputs.in_mappedsurf = 'lh.pial'
jacobian.inputs.out_file = 'outfile.pial'
res = jacobian.run()Checking the inputs and outputs: jacobian.cmdline
'mris_jacobian lh.pial lh.pial outfile.pial'
res.inputs['out_file']
'outfile.pial'
res.outputs['out_file']
'/home/data/nbc/sub-01/surf/outfile.pial'But the file that is actually created is still |
|
Cool. Thanks for rounding up these cases.
I believe these were added as part of the I agree that this probably isn't an appropriate solution to interfaces that aren't used in
Yes, it's basically what you did, except the naming behavior of
Yeah, this is definitely a bug. It works under the assumption that nobody actually changes
These happen before it runs, so I think it should actually be fine, especially if they're generating absolute paths (which I'm pretty sure they should). But again, this correct behavior is predicated on users not specifying I like the class FSSurfaceCommand(FSCommand):
...
def _get_filecopy_info(self):
self._normalize_filenames()
return super(FSSurfaceCommand, self)._get_filecopy_info()
def _normalize_filenames(self):
"""Filename normalization routine to perform only when run in Node context"""
passThen I'd rewrite class MRIsExpand(FSSurfaceCommand):
_cmd = 'mris_expand'
input_spec = MRIsExpandInputSpec
output_spec = MRIsExpandOutputSpec
def _list_outputs(self):
outputs = self._outputs().get()
outputs['out_file'] = self._associated_file(self.inputs.in_file,
self.inputs.out_name)
return outputs
def _normalize_filenames(self):
in_file = self.inputs.in_file
pial = self.inputs.pial
if not isdefined(pial):
pial = 'pial'
self.inputs.pial = self._associated_file(in_file, pial)
if isdefined(self.inputs.thickness) and self.inputs.thickness:
thickness_name = self.inputs.thickness_name
if not isdefined(thickness_name):
thickness_name = 'thickness'
self.inputs.thickness_name = self._associated_file(in_file,
thickness_name)
self.inputs.sphere = self._associated_file(in_file, self.inputs.sphere)
super(MRIsExpand, self)._normalize_filenames()WDYT? |
|
Should class FSSurfaceCommand(FSCommand):
...
def _get_filecopy_info(self):
self._normalize_filenames()
return super(FSSurfaceCommand, self)._get_filecopy_info()
def _normalize_filenames(self):
"""Filename normalization routine to perform only when run in Node context"""
if isdefined(self.inputs.out_file):
self.inputs.out_file = os.path.abspath(self.inputs.out_file) If |
|
This comment gets into the weeds a bit, so tl;dr:
To address your comment in detail:
I wouldn't do that, because there's no guarantee that all
No, because
But sometimes we (or at least I) want it to add the hemisphere prefix, and I'll signal that by using an Consider the following cases for
The right column is determined by how the underlying program treats the inputs. The bottom row is inflexible because the user gave path information. So our only real question is how to treat the top left corner. If we always do
Which, in this context, is probably not what the user wanted. Now in your case, I think you want to actually adjust the behavior to be less surprising, like:
This seems completely reasonable. The |
|
Thanks for the detailed reply. I'll make the minimal changes and leave the full refactoring for later/someone with more knowledge. I think I just have one question left. When you say that import nipype.interfaces.freesurfer as fs
mris = fs.MRIsCombine()
mris.inputs.in_files = ['lh.pial', 'rh.pial']
mris.inputs.out_file = 'out.stl'
mris.cmdlineshould give you |
|
The former. |
nipype/interfaces/freesurfer/base.py
Outdated
| By including the full path in the filename, we can also avoid this behavior. | ||
| """ | ||
| def __init__(self, **inputs): | ||
| super(FSSurfaceCommand, self).__init__(**inputs) |
There was a problem hiding this comment.
No need for an __init__ if all it calls is super.
There was a problem hiding this comment.
So go straight to _get_filecopy_info?
nipype/interfaces/freesurfer/base.py
Outdated
| def _get_filecopy_info(self): | ||
| """Filename normalization routine to perform only when run in Node | ||
| context | ||
| """ |
There was a problem hiding this comment.
Comment belongs in _normalize_filenames.
nipype/interfaces/freesurfer/base.py
Outdated
| """ | ||
| path, base = os.path.split(out_name) | ||
| if path == '': | ||
| _, in_file = os.path.split(in_file) |
There was a problem hiding this comment.
This should be path, in_file = .... The logic here is "if out_file doesn't have path information, use in_file's path".
There was a problem hiding this comment.
Sorry that's my mistake. With mris_convert --combinesurfs (MRIsCombine) it outputs to the working directory, but I didn't realize that that wasn't how regular calls to mris_convert worked. Will fix.
| """ | ||
| path, base = os.path.split(out_name) | ||
| if path == '': | ||
| _, in_file = os.path.split(in_file) |
There was a problem hiding this comment.
Using _ here instead of replacing path means that mris_convert outputs to the working directory by default. Is this correct?
There was a problem hiding this comment.
With MRIsCombine, it outputs to the working directory if path information isn't provided. So I think that's what we want.
There was a problem hiding this comment.
Sounds good. You can just remove this line, then, since in_file isn't used at all otherwise. Actually, the whoe thing could be shortened to:
path, base = os.path.split(out_name)
if path == '' and base[:3] not in ('lh.', 'rh.'):
base = 'lh.' + base
return os.path.abspath(os.path.join(path, base))And if you prefer, you could just do this directly in _list_outputs, rather than override _associated_file. (That's up to you, though.)
|
|
||
| For complete details, see the `mris_convert Documentation. | ||
| <https://surfer.nmr.mgh.harvard.edu/fswiki/mris_convert>`_ | ||
|
|
There was a problem hiding this comment.
Here might be a good place to add a comment about outputs (so it shows up in the docs):
If given an out_file that does not begin with 'lh.' or 'rh.', mris_convert will prepend 'lh.'
to the file name.
To avoid this behavior, consider setting out_file = './<filename>', or leaving out_file blank.
|
@satra would you mind having a look at this one? Here's another case where we need to do a little pre-run fiddling to get FreeSurfer commands to run nicely in a Node, so I've suggested injecting a hook for |
In freesurfer/base/FSSurfaceCommand: - Remove unnecessary __init__ - Move _get_filecopy_info docstring to _normalize_filenames - Add to _associated_files docstring - Output files with no path info go to in_file’s directory, not working directory. In freesurfer/utils/MRIsCombine: - Add to MRIsCombine dosctring
|
LGTM. Assuming tests pass, and there are no objections, I'll merge tonight or in the morning. Thanks for your patience. |
| base = 'lh.' + base | ||
| return os.path.abspath(os.path.join(path, base)) | ||
|
|
||
| def _normalize_filenames(self): |
There was a problem hiding this comment.
it says this function is used in the Node context, but where is it used inside Node?
There was a problem hiding this comment.
MRIsCombine uses mris_convert to combine two surface files.