Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Interfaces with automated population of outputs #3150

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

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

Merging #3150 into master will decrease coverage by 3.76%.
The diff coverage is 45.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
- Coverage   67.62%   63.86%   -3.77%     
==========================================
  Files         299      298       -1     
  Lines       39487    39623     +136     
  Branches     5220     5268      +48     
==========================================
- Hits        26703    25304    -1399     
- Misses      12073    13252    +1179     
- Partials      711     1067     +356
Flag Coverage Δ
#smoketests ?
#unittests 63.86% <45.16%> (-1.01%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base/experimental.py 45.16% <45.16%> (ø)
nipype/interfaces/nilearn.py 38.98% <0%> (-57.63%) ⬇️
nipype/workflows/__init__.py 47.05% <0%> (-52.95%) ⬇️
nipype/utils/spm_docs.py 20% <0%> (-48%) ⬇️
nipype/interfaces/freesurfer/base.py 45.68% <0%> (-32.76%) ⬇️
nipype/utils/logger.py 58.46% <0%> (-30.77%) ⬇️
nipype/algorithms/rapidart.py 34.42% <0%> (-29.68%) ⬇️
nipype/interfaces/spm/base.py 57.66% <0%> (-29.34%) ⬇️
nipype/utils/provenance.py 55.16% <0%> (-28.71%) ⬇️
nipype/interfaces/fsl/model.py 55.18% <0%> (-25.4%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d2fe1d...75d12f4. Read the comment docs.

@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