Skip to content

ENH: Add SimpleInterface #2220

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

Merged
merged 3 commits into from
Oct 9, 2017
Merged

Conversation

effigies
Copy link
Member

@effigies effigies commented Oct 9, 2017

This adds niworkflows.interfaces.base.SimpleInterface to nipype.interfaces.base.

SimpleInterface is an interface that gives dictionary access to the output spec. We've found it a very useful boilerplate-reducer for making interfaces by simply defining input/output specs and _results.

Merging this would reduce friction for a lot of other poldracklab (niworkflows, mriqc, fmriprep) interfaces that people may want to see merged into nipype proper. I'm open to cleaning it up to be slightly less simple, as long as it achieves the same reduction in boilerplate in subclasses.

Slightly related: #2083

@oesteban EDITED: changed _runtime for _results.

Copy link
Contributor

@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.

@satra this has been very helpful in niworkflows/fmriprep/mriqc so far.

@@ -1212,6 +1212,41 @@ def save_inputs_to_json(self, json_file):
json.dump(inputs, fhandle, indent=4, ensure_ascii=False)


class SimpleInterface(BaseInterface):
""" An interface pattern that allows outputs to be set in a dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest:

"""
An interface pattern that allows outputs to be set in a dictionary called ``_results``
that is automatically interpreted by ``_list_outputs()`` to find the outputs.

...
"""

class SimpleInterface(BaseInterface):
""" An interface pattern that allows outputs to be set in a dictionary

When implementing `_run_interface`, set outputs with::
Copy link
Contributor

Choose a reason for hiding this comment

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

Double quotes for the docstring:

When implementing ``_run_interface``, set outputs with::


self._results[out_name] = out_value

This can be a way to upgrade a ``Function`` interface to do type checking:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing:

Example
-------

That is a standard in the docstring of nipype interfaces

@codecov-io
Copy link

Codecov Report

Merging #2220 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2220      +/-   ##
==========================================
- Coverage   72.31%   72.31%   -0.01%     
==========================================
  Files        1180     1180              
  Lines       58879    58885       +6     
  Branches     8474     8474              
==========================================
+ Hits        42579    42580       +1     
- Misses      14921    14926       +5     
  Partials     1379     1379
Flag Coverage Δ
#smoketests 72.31% <50%> (-0.01%) ⬇️
#unittests 69.87% <50%> (-0.01%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/base.py 84.76% <50%> (-0.22%) ⬇️
nipype/pipeline/plugins/multiproc.py 76.76% <0%> (-1.41%) ⬇️

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 8e1a86a...87c1c27. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Oct 9, 2017

Codecov Report

Merging #2220 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2220      +/-   ##
==========================================
+ Coverage   72.31%   72.31%   +<.01%     
==========================================
  Files        1180     1180              
  Lines       58879    58885       +6     
  Branches     8474     8474              
==========================================
+ Hits        42579    42585       +6     
  Misses      14921    14921              
  Partials     1379     1379
Flag Coverage Δ
#smoketests 72.31% <100%> (ø) ⬆️
#unittests 69.9% <100%> (+0.02%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/base.py 85.06% <100%> (+0.09%) ⬆️
nipype/pipeline/plugins/multiproc.py 76.76% <0%> (-1.41%) ⬇️
nipype/utils/profiler.py 41.35% <0%> (+1.23%) ⬆️

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 8e1a86a...cba28e4. Read the comment docs.

@satra
Copy link
Member

satra commented Oct 9, 2017

LGTM - i can see how this is useful and keeps data validation.

@effigies
Copy link
Member Author

effigies commented Oct 9, 2017

@oesteban any more comments?

@oesteban oesteban merged commit 56b7c81 into nipy:master Oct 9, 2017
@oesteban
Copy link
Contributor

oesteban commented Oct 9, 2017

Nope :)

oesteban added a commit to oesteban/niworkflows that referenced this pull request Oct 9, 2017
@satra satra added this to the 0.14.0 milestone Oct 20, 2017
@satra satra modified the milestone: 0.14.0 Oct 28, 2017
@effigies effigies deleted the enh/simple_interface branch April 2, 2018 14:06
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.

4 participants