FIX: DTITK Interface #2514
FIX: DTITK Interface #2514
Conversation
… instead of being mandatory
…ated xfm to a boolean instead of integer
…n in that directory through another mechanism.
|
Here's a mixin you can put in class DTITKRenameMixin(object):
def __init__(self, *args, **kwargs):
classes = [cls.__name__ for cls in self.__class__.mro()]
dep_name = classes[0]
rename_idx = classes.index('DTITKRenameMixin')
new_name = classes[rename_idx + 1]
warnings.warn('The {} interface has been renamed to {}\n'
'Please see the documentation for DTI-TK '
'interfaces, as some inputs have been '
'added or renamed for clarity.'
''.format(dep_name, new_name),
DeprecationWarning)
super(DTITKRenameMixin, self).__init__(*args, **kwargs)And then for classes that have been renamed, we can just have a big sequence of dummy classes like so:
class RigidTask(DTITKRenameMixin, Rigid):
pass
...
class diffeoScalarVolTask(DTITKRenameMixin, DiffeoScalarVol):
pass
class TVAdjustVoxSpTask(DTITKRenameMixin, TVAdjustVoxSp):
pass
...
class BinThreshTask(DTITKRenameMixin, BinThresh):
pass@satra @mgxd @djarecka I suspect you might have preferences for how deprecations are handled. Feel free to suggest any modifications (e.g. do we want to set a version that these will be removed by?). |
| return | ||
| return fname_presuffix(os.path.basename( | ||
| self.inputs.in_df).split('.')[0], | ||
| suffix='_affdf.df.nii') |
There was a problem hiding this comment.
So you originally had (a10da5d): name_template="%s_aff.df", keep_extension=True
I think we can achieve that pattern as follows:
return fname_presuffix(os.path.basename(self.inputs.in_df),
suffix='_aff.df', keep_ext=True)There was a problem hiding this comment.
In that case, fname_presuffix still keeps the .df, so we end up with an extra extension (filename.df_aff.df.nii). I wanted to make it match the other warps .df.nii extension with something like _affdf prepended so the user knows this warp includes both the affine and diffeomorphic transforms together (filename_affdf.df.nii or filename_affdf.df.nii.gz). Is the extra extension okay or would it be better to match the other warp files that come from just the diffeo transform?
There was a problem hiding this comment.
Oh, I see. I misunderstood the situation (and I'm now no longer sure that fixing #2506 will let us do what you want). There are two perspectives here, IMO.
-
File names are semantically meaningful, and as long as we're going to impose a convention within the DTI-TK interfaces, we should respect it.
-
Nipype is often used to bring tools together that have zero respect for each others' conventions, so attempting to manipulate names semantically will fail in some cases. Naming should largely be handled at the
DataSinklevel.
I think we can sort of split the difference here:
path, base, ext = split_filename(self.inputs.in_df)
suffix = '_affdf'
if base.endswith('.df'):
suffix += '.df'
base = base[:-3]
return fname_presuffix(base, suffix=suffix + ext, keep_ext=False)This way if you get a file named x.df.nii, you'll get x_affdf.df.nii, but if you just get x.nii, you'll get x_affdf.nii.
Did I get your convention right? And does this seem like a reasonable way to achieve that?
There was a problem hiding this comment.
Yup. Thanks! I think it's a good idea to match the diffeomorphic naming convention.
nipype/interfaces/dtitk/utils.py
Outdated
| if name == "out_file": | ||
| return self._list_outputs()["out_file"] | ||
| return None | ||
| class BinThreshTASK(DTITKRenameMixin, BinThresh): |
|
@effigies can I test the automatic naming in a workflow before this gets merged? |
|
Sounds good. Just say when you're ready. |
|
Thanks, @effigies . Something isn't right and I'm trying to figure out what it is... in the report.rst for nodes with multiple outputs that are named automatically, the transform is not getting logged as an output ("out_file_xfm" is undefined). If I look at the .json, "needed_outputs" lists only "out_file" ("out_file_xfm" is missing)... I think this is making it delete the transform from the directory since it isn't listed in "needed outputs" (the transforms are missing from the node directory). Do I need to do something with "aggregate_outputs" ? |
|
You can update the configuration to set If |
…rect directory (previous node) to the current node directory
|
Thanks for the suggestion, @effigies. That was helpful in debugging. I think I figured out what's been causing problems and how to fix it. It seems like the outputs aren't moving between node directories correctly in registration nodes. Let me know if you think this is a good fix. DTI-TK saves the outputs in the same directory as the inputs for the functions that don't take an output path (e.g. Rigid registration), so I addressed this by setting copyfile=False for both fixed and moving images (which means that the execution inputs are paths to the input files in the node working directory instead of the previous node's directory). Then, the outputs (image and rigid transform) get put in the node working directory instead of the input node directory (which is what we want). We still have an issue, however, in that there is no input for the initialization transform (to provide the Affine node with the Rigid node's output transform), so it needs to be copied to the Affine node working directory. I addressed this by making the boolean flag instructing the use of a precomputed transform to initialize the registration (use_in_transform) into a file that takes in the .aff transform file. It adds the transform as a string, which triggers using the transform just like the "1" flag would, and leaves it off it it isn't set (thus not triggering "use transform". Then I can set copyfile=True to ensure that the transform makes it into the new directory even though it isn't specified as an input to the command line. Cases: Rigid.outputs.out_file_xfm connected to Affine.inputs.initialize_xfm One thing to be addressed with this approach is that we need to enforce the DTITK format or it's a silent failure (if it can't open the .aff transform file, it just skips it and still performs the registration with a different initialization strategy) |
|
Yes, using If that's correct, I think we still want to be able to use initialize_xfm = traits.Either(File(exists=True, copyfile=False), "1",
desc="Initialize with DTITK-FORMAT transform "
"('1' to initialize with existing transform)",
position=5, argstr="%s")I'm not sure whether And then, to check for nipype/nipype/interfaces/fsl/epi.py Lines 1112 to 1116 in bc6df5b You can modify the check to look for a specific string, if there may be innocuous |
|
Yes; that is my understanding of the issue. Your suggestion works for running the interface alone, but in the pipeline it doesn't put the symlink to .aff in the working directory. It works if you put copyfile=False outside the File(), but that breaks if you set it to 1. Do you know why copyfile=False would behave differently inside of copyfile=False as argument to copyfile=False as argument to |
|
Oh, so if you specify the location, it uses that location as both input and output? I get it now. Out of curiosity, is it still a symlink after running? If so, I suspect it's overwriting the file back in the rigid node, in which case you'll want And I guess let's drop the |
|
It still saves the .aff to the node working directory (because the moving file gets symlinked and that is the path used in the execution input, which is the basis for the output I don't see the symlinks after running (I think they're deleted after the node processes), but I can infer that they're there during processing from the 'Execution Inputs' section of the If we drop |
|
So just regarding symlinks and Specifically, I'm trying to verify whether a new file is being written or if it's writing to the old file through the symlink. To illustrate the idea: $ echo A > a
$ cat a
A
$ ln -s a b
$ echo B > b
$ cat a
BBasically, if this is what's happening, we need to switch to Sorry if this was all clear to you and you were trying to make a different point, by the way.
So at this point we're balancing which is more useful to a user: checking whether the file exists or letting them type |
|
I think the intended behavior is to overwrite the previous .aff file behind the scenes to cut down on processing, but I don't think that happens backward through the symlink in our case because the "execution" paths are all to the working directory so the I agree that setting
I made that change, and added simple workflows for performing affine and diffeomorphic registration to make sure a user has a functioning pipeline example that adheres to dtitk's requirements (e.g. array size dimension zero must be a power of 2). Does that look good? |
|
This looks great. If you're all set, I'm happy to merge this. |
Fixes #2462
Changes proposed in this pull request
Closes #2463.