Skip to content

Conversation

satra
Copy link
Member

@satra satra commented Oct 9, 2013

core-devs please review. (unfortunately a bunch of PEP-8 fixes are also there).

This patch is to demonstrate a plan to refactor filename generation and thereby simplify interface creation.

@bpinsard, @inati - comments much appreciated

in this commit take a look at the InvWarp class in utils.py.

inverse_warp = File(argstr='--out=%s', name_source=['warp'],
                        hash_files=False, name_template='%s_inverse',
...

to use this, the interface developer should:

  1. add metadata to inputspec:
    name_source: indicates which field to draw a name from, this field must be the name of a File
    name_template (optional): if you want to override the default _generated suffix
    keep_extension (optional): if you want the extension from the input to be kept.
  2. add functionality to your class or base class, to allow changing extensions specific to package or interface
def self._overload_extension(self, value):
     return value #do whatever you want here with the name
  1. in the outputspec make sure the name matches that of the input spec

@bpinsard
Copy link
Contributor

so if I get it right, this would change from being able to specify the prefix/suffix in the interface specification and instantiation to only the former?

@satra
Copy link
Member Author

satra commented Oct 10, 2013

the sequence of operations are:

in parse_args:

            if spec.genfile or spec.name_source:
                value = self._filename_from_source(name)
                if not isdefined(value):
                    value = self._gen_filename(name)

in filename_from_source

    def _filename_from_source(self, name):
        trait_spec = self.inputs.trait(name)
        retval = getattr(self.inputs, name)
        if not isdefined(retval):
            if not trait_spec.name_source:
                return retval
            name_template = trait_spec.name_template
            if not name_template:
                name_template = "%s_generated"
            if isinstance(trait_spec.name_source, list):
                for ns in trait_spec.name_source:
                    if isdefined(getattr(self.inputs, ns)):
                        name_source = ns
                        break
            else:
                name_source = trait_spec.name_source
            _, base, _ = split_filename(getattr(self.inputs, name_source))
            retval = name_template % base
            _, _, ext = split_filename(retval)
            if trait_spec.keep_extension and ext:
                return retval
            return self._overload_extension(retval)
        return retval

and then later on in list outputs:

    def _list_outputs(self):
        metadata = dict(name_source=lambda t: t is not None)
        out_names = self.inputs.traits(**metadata).keys()
        if out_names:
            outputs = self.output_spec().get()
            for name in out_names:
                outputs[name] = \
                    os.path.abspath(self._filename_from_source(name))
            return outputs

so from an interface developer perspective i think in most cases you could get by with simply the inputspec and outputspec declarations and in some cases you might want to implement _overload_extension. for example, if an interface really wants abspath then

def _overload_extension(self, value):
    return os.path.abspath(value)

or in the case of FSL, it can change the extension to the expected default.

@satra
Copy link
Member Author

satra commented Oct 13, 2013

@chrisfilo added docs - also modified another interface (FLIRT) to match with doc example.

* master:
  fix: ridiculous mistakes
  fix: make sure mandatory=False items are displayed
@chrisgorgo chrisgorgo merged commit e20d221 into nipy:master Oct 15, 2013
@bpinsard
Copy link
Contributor

The only concern I have is for backward compatibility with pipeline that I am running regularly:
previously I was able to set generated name in pipelines using for example:

calc = afni.Calc(out_file='%s_mycomputation')

but now if I merge this pr, all the nodes and descendants will have to rerun because nipype expects %s_calc.ext file for instance.
the only option not to rerun all is to tweak each interface generated file that I have set previously doing:

calc.inputs.traits()['out_file'].name_template='%s_mycomputation'

I know this feature was not yet really stable but I do wonder if we could keep also detecting %s in the value for filename to be generated.

@satra satra deleted the fix/invwarp branch October 30, 2017 15:13
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.

3 participants