@satra: some of these functions require parameters to be set on the command line (no defaults), but we provide default values in the input spec (I matched defaults to the example from the DTITK tutorial), fulfilling this requirement. I am redoing the input specs such that usedefault=True and mandatory=False. In general, does that sound like a good idea, or is it better to make the user specify?

if it's a common default value across use cases then setting a default value is fine. however, if it's a required argument, mandatory should always be set to True even if a default value is used. also mandatory=False should not be used.

this flag (useInTrans) determines whether or not a previously calculated affine is used to initialize the registration. If it is set to anything, the affine file in the working directory with the ${prefix}.aff will be used. Otherwise, the center of mass is used to initialize. If it's a traits.Bool, setting it to false is still tripping the flag (executes as true) so I don't want to do that because it would be misleading. According to the tutorial, it should be either set to 1 or not set at all.

My thought is to have the field be traits.Int with default 1, but not specify to usedefault for any case (even the affine) so that the user must specify useInTrans=1 to initialize with an affine. Otherwise, it's undefined and the center of mass is used to initialize. Does that sound good?

If you're not going to usedefault=True, setting a default value doesn't seem to have much point. And it sounds like perhaps this makes more sense as a traits.Bool with argstr="1". That would place "1" in position 7 if True, and omit if False or unspecified. Does that seem reasonable?

Yes. Thank you, @effigies .

Since a rigid should be calculated before an affine, I'm setting the usedefault=True to be the case in the affine interface, but not the rigid. Does that make sense or do you think both should require the user to request that the precalculated xfm be used as the initialization?

For the sake of flexibility, I probably would not set usedefault here (it becomes more difficult to un-set, so if there's ever a case you might not want the default, best to avoid), but you could use the docstring to show/describe how you expect people will want to use it.

Getting rid of TVAdjustOrigin because it is redundant (this calls the same command as TVAdjustVoxelspace)

As long as everything that TVAdjustOrigin does can be done with TVAdjustVoxelspace, that sounds fine to me.

Yes. It calls the command TVAdjustOrigin with a specific set of flags, so it is redundant as an interface.

I'm going to set the default of this to be (0,0,0) and set usedefault=True because that is recommended by the tutorial. If a target image is provided, this will be ignored anyway so I think it's fine to do. Let me know if you disagree.

I'm personally disinclined to add arguments that will be ignored, since that could be confusing to someone attempting to understand or debug a pipeline. You could set it to xor=['in_target'], to make clear that these are mutually exclusive options.

Also, I think you probably want to use numeric data type here, rather than a string:

origin = traits.Tuple(traits.Float, traits.Float, traits.Float, mandatory=True,
                      xor=['in_target'], position=4, argstr='-origin (%g,%g,%g)')

Similarly, in_target should list origin in its xor parameter. (Mandatory is okay in mutually exclusive cases.)

There are multiple parameters that will be ignored if 'in_target' is provided. You can either provide a target image to match parameters to (origin & voxel size) or give parameters individually, but you do not need to provide both. E.g. one could just want to change the voxel size but not the origin, so mandatory would be problematic in that case, right?

How does this look (link, below)? The way I have it, there are defaults with a note in the description that they would be superseded by in_target.

https://github.com/kesshijordan/nipype/blob/satra_fix_specs/nipype/interfaces/dtitk/utils.py

Ah, yes, mandatory isn't necessary, then. I was going off of how this is currently written. My understanding now is: If target file is specified, array size, voxel size, and origin are defined by the properties of the target file and any explicit specifications are ignored; if it is not specified, then the properties of the input file are preserved, except where specified explicitly. So essentially none of these inputs is mandatory except in_file.

It's also unclear to me whether the positions are relevant. Typically, flagged arguments aren't required to be in a specific order. For the sake of simplicity, I'll assume they can be removed.

So, if that understanding is correct, then I might rewrite the input spec as follows (a couple comments within):

class TVResampleInputSpec(CommandLineInputSpec):
    in_file = File(desc="image to resample", exists=True,
                   mandatory=True, argstr="-in %s")
    out_file = traits.Str(desc='output path',
                          name_source="in_file", name_template="%s_resampled",
                          keep_extension=True, argstr="-out %s")
    target_file = File(desc='specs read from the target volume',
                       argstr="-target %s", xor=['array_size', 'voxel_size', 'origin'])
    align = traits.Str('center', argstr="-align %s")
    # This is missing an argstr. I'm not sure what it's supposed to be
    # Would also add a desc to make the options a little clearer
    interpolation = traits.Enum('LEI', 'EI')
    # Note the switch to ints and %d
    array_size = traits.Tuple((traits.Int(), traits.Int(), traits.Int()),
                           desc='resampled array size', xor=['target_file'],
                           argstr="-size %d %d %d")
    # Using %g will almost always give better command strings than %f
    voxel_size = traits.Tuple((traits.Float(), traits.Float(), traits.Float()),
                         desc='resampled voxel size', xor=['target_file'],
                         argstr="-vsize %g %g %g")
    origin = traits.Tuple((traits.Float(), traits.Float(), traits.Float()),
                          desc='xyz origin', xor=['target_file'],
                          argstr='-origin %g %g %g')

(Since you've done some renaming of these input fields, I've taken the liberty of renaming to names that I feel are a little more descriptive. Feel free to reject these changes if you prefer the ones you had.)

Note the xor arguments for target_file, array_size, voxel_size, and origin. That ensures that users aren't mistakenly passing parameters that will be ignored. (Again, I'm assuming that array_size should be included in this group. You can just remove its xor and remove it from target_file's xor list, if I'm wrong.)

Thanks, @effigies ! This is very helpful. The xor is a good idea to make sure the user is aware their inputs are ignored. I've fixed your other notes, here, and will apply them to other interfaces.

Could you add a node.cmdline with the expected command line, here?

Similarly, this would be good to add to any of the docstrings that are currently missing it.

If you're not going to usedefault=True, setting a default value doesn't seem to have much point. And it sounds like perhaps this makes more sense as a traits.Bool with argstr="1". That would place "1" in position 7 if True, and omit if False or unspecified. Does that seem reasonable?

You can remove these prefatory lines. Tests are now run in that directory through another mechanism. (You may need to merge master to make it work.)

Similarly at the tops of other files.

As long as everything that TVAdjustOrigin does can be done with TVAdjustVoxelspace, that sounds fine to me.

I'm personally disinclined to add arguments that will be ignored, since that could be confusing to someone attempting to understand or debug a pipeline. You could set it to xor=['in_target'], to make clear that these are mutually exclusive options.

Also, I think you probably want to use numeric data type here, rather than a string:

origin = traits.Tuple(traits.Float, traits.Float, traits.Float, mandatory=True,
                      xor=['in_target'], position=4, argstr='-origin (%g,%g,%g)')

Similarly, in_target should list origin in its xor parameter. (Mandatory is okay in mutually exclusive cases.)