Skip to content

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Jan 8, 2020

Proof of concept for #3149

This is a very rough draft of how such an interface would look like. It only tests automatic templated names filling (glob patterns should be fairly easy to add, and regexes too).

Some wrinkles around the corners need to be ironed out. E.g., implementing a keep_ext metadata for outputs, and raising an error if two inputs are set in the template and extensions are different. Similarly, raise error if not all the arguments were replaced.

But in general, it seems doable.

BTW the pattern matching and replacing part of this PR could also be useful to accept the name_template metadata in BaseInterfaces (currently, it is only allowed in command line interfaces).

WDYT @satra, @mgxd, @djarecka, @mgxd ?

EDIT: The new AutoOutputInterface is basically a copy of the current BaseInterface, but removing the _list_outputs and aggregate_outputs, and adding this: https://github.com/nipy/nipype/pull/3150/files#diff-1353462138ab420e9b6ce7f8a19ab284R407-R456

@codecov
Copy link

codecov bot commented Jan 8, 2020

Codecov Report

❌ Patch coverage is 57.86164% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.93%. Comparing base (5d2fe1d) to head (75d12f4).
⚠️ Report is 1384 commits behind head on master.

Files with missing lines Patch % Lines
nipype/interfaces/base/experimental.py 57.86% 51 Missing and 16 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
- Coverage   67.62%   63.93%   -3.70%     
==========================================
  Files         299      298       -1     
  Lines       39487    39556      +69     
  Branches     5220     5240      +20     
==========================================
- Hits        26703    25290    -1413     
- Misses      12073    13207    +1134     
- Partials      711     1059     +348     
Flag Coverage Δ
smoketests ?
unittests 63.91% <57.86%> (-0.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@effigies
Copy link
Member

effigies commented Jan 8, 2020

This seems reasonable on first glance. I think problems are likely to be found while trying to work through a few examples.

I assume that we'll still support the old style, though? Otherwise a lot of interfaces will need to be rewritten mostly from scratch.

@satra
Copy link
Member

satra commented Jan 8, 2020

@oesteban - this looks reasonable.

Copy link
Contributor Author

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I assume that we'll still support the old style, though?

Yes, that would be the idea. Although, if this turns out easier than we thought we could think of a new config compatibility_mode that loads the legacy interfaces when on and these new when off.

Otherwise a lot of interfaces will need to be rewritten mostly from scratch.

For aggregate_outputs not those many:

$ git grep aggregate_outputs
...
nipype/interfaces/afni/preprocess.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/afni/preprocess.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/afni/utils.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/afni/utils.py:        outputs = super(Autobox, self).aggregate_outputs(runtime, needed_outputs)
nipype/interfaces/afni/utils.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/ants/registration.py:    >>> reg4b.aggregate_outputs()  # doctest: +SKIP
nipype/interfaces/ants/registration.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/base/core.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/base/core.py:    This class does not implement aggregate_outputs, input_spec or
nipype/interfaces/base/core.py:            outputs = self.aggregate_outputs(runtime)
nipype/interfaces/base/core.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/base/tests/test_core.py:        nif.aggregate_outputs()
nipype/interfaces/freesurfer/preprocess.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/freesurfer/preprocess.py:        return super(SegmentCC, self).aggregate_outputs(runtime, needed_outputs)
nipype/interfaces/freesurfer/utils.py:        # aggregate_outputs.  Let's try to parse stderr and raise a
nipype/interfaces/freesurfer/utils.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/fsl/model.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/fsl/preprocess.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/fsl/preprocess.py:        outputs = super(FLIRT, self).aggregate_outputs(
nipype/interfaces/fsl/utils.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/niftyseg/label_fusion.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/interfaces/spm/model.py:    def aggregate_outputs(self, runtime=None):
nipype/interfaces/spm/model.py:    def aggregate_outputs(self, runtime=None, needed_outputs=None):
nipype/pipeline/engine/nodes.py:            aggouts = self._interface.aggregate_outputs(

Most of these redefine aggregate_outputs to parse standard out/err.

The use of _list_outputs is more pervasive and will probably require some kind of high-level parsing (which we would need anyways in the migration to pydra if we want to make interfaces have less weight and boilerplate). I'm worried this part could be a lot of work.

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