-
Notifications
You must be signed in to change notification settings - Fork 536
ENH: Interfaces with automated population of outputs #3150
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
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
@oesteban - this looks reasonable.
There was a problem hiding this 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.
Uh oh!
There was an error while loading. Please reload this page.
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_templatemetadata in BaseInterfaces (currently, it is only allowed in command line interfaces).WDYT @satra, @mgxd, @djarecka, @mgxd ?
EDIT: The new
AutoOutputInterfaceis basically a copy of the currentBaseInterface, but removing the_list_outputsandaggregate_outputs, and adding this: https://github.com/nipy/nipype/pull/3150/files#diff-1353462138ab420e9b6ce7f8a19ab284R407-R456