ENH: Add tab completion for node and interface inputs properties #2735

leej3 · 2018-10-12T20:37:40Z

Summary

Improves ease of use by giving more convenient tab completion for constructing graphs.

Fixes # .

List of changes proposed in this PR (pull-request)

adds tab completion to the inputs property of both node and interface. This behavior is currently only observed in Ipython and if the user sets "c.IPCompleter.limit_to__all__ = True" in their ipython config or executes the magic command "%config IPCompleter.limit_to__all__ = True"

Acknowledgment

I acknowledge that this contribution will be available under the Apache 2 license.

effigies · 2018-10-13T12:03:49Z

Could you add a test for TraitedSpecs (e.g. CommandLine interface) and DynamicTraitedSpecs (e.g. Function interface)?

I'm guessing the reason for including this in BaseInterface, as opposed to BaseTraitedSpec, is that the spec should already be fleshed out by the time this is called. But I think Function at least calls super before adding inputs. Instead, I think maybe the better way to do this will be to update self.__all__ in BaseTraitedSpec on __init__ and when new traits are added.

But if the test passes for both static and dynamic specs, I'm okay with this, as it's pretty simple.

effigies · 2018-10-13T17:10:32Z

Also, is this targeting 1.x or 2.0?

work still needed for dynamic traits

codecov-io · 2018-10-15T18:02:24Z

Codecov Report

Merging #2735 into master will decrease coverage by 3.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2735      +/-   ##
==========================================
- Coverage   67.44%   63.99%   -3.46%     
==========================================
  Files         340      338       -2     
  Lines       43170    43121      -49     
  Branches     5353     5350       -3     
==========================================
- Hits        29117    27596    -1521     
- Misses      13355    14454    +1099     
- Partials      698     1071     +373

Flag	Coverage Δ
#smoketests	`?`
#unittests	`63.99% <100%> (-0.84%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/base/specs.py	`86.93% <100%> (-6.18%)`	⬇️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/freesurfer/base.py	`49.59% <0%> (-30.9%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35.39% <0%> (-29.21%)`	⬇️
nipype/interfaces/spm/base.py	`58.41% <0%> (-29.05%)`	⬇️
nipype/utils/provenance.py	`55.73% <0%> (-28.99%)`	⬇️
nipype/interfaces/fsl/model.py	`55.26% <0%> (-25.35%)`	⬇️
nipype/testing/fixtures.py	`77.33% <0%> (-21.34%)`	⬇️
... and 43 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 3ec8065...59a4ba0. Read the comment docs.

leej3 · 2018-10-15T18:05:38Z

Thanks. Your pointers helped me a lot. It does indeed work better by modifying BaseTraitedSpec. I've added tests for my desired behavior.

I'm still struggling to implement the functionality for DynamicTraitedSpec. I'm not sure where to modify it. Do you have any thoughts?

effigies · 2018-10-15T18:07:04Z

Perhaps override trait_set to update __all__ after running super().trait_set()?

leej3 · 2018-10-16T19:45:54Z

This is almost working. I would expect the following test to pass but the second assert doesn't:

    def test_DynamicTraitedSpec_tab_completion():
        extract_func = lambda list_out: list_out[0]

        # Define node
        list_extract = Node(Function(
            input_names=["list_out"],output_names=["out_file"],
            function=extract_func), name="list_extract")

        # Check __all__ for node outputs
        expected_output = sorted(list_extract.outputs.editable_traits())
        assert(sorted(list_extract.outputs.__all__) == expected_output)

        # Add trait and retest
        list_extract.outputs.add_trait("added_out_trait","val")
        expected_output = sorted(['added_out_trait',*expected_output])
       assert(sorted(list_extract.outputs.__all__) == expected_output)

This doesn't pass on the commits prior to mine though.
EDIT: Rather, I mean that I would expect to be able to add traits for DynamicTraitedSpec objects but cannot.

leej3 · 2018-10-16T19:51:41Z

Also, is this targeting 1.x or 2.0?

1.x. 2.0 will have to incorporate it following a pending pyhydra refactor.

effigies · 2018-10-16T19:58:58Z

I think the problem may be that Function can't take a lambda. It requires a normal function.

leej3 · 2018-10-16T20:04:27Z

good. to know. i'll change that. It doesn't fix this problem though

effigies · 2018-10-16T20:21:21Z

nipype/interfaces/base/tests/test_specs.py

+    assert(sorted(list_extract.outputs.__all__) == expected_output)
+
+    # Add trait and retest
+    list_extract.outputs.add_trait("added_out_trait","val")


The problem is that the output spec is actually instantiated in IOBase._outputs(), which calls Function._add_output_traits, which only will add things in self._output_names. So to do what you want here, you would need to do:

list_extract._interface._output_names.append('added_out_trait')

indeed. Thank you. I don't think I would have got that.

due to an error in understanding of dynamic trait instantiation: output spec is actually instantiated in IOBase._outputs(), which calls Function._add_output_traits, which only will add things in self._output_names

effigies · 2018-10-16T20:41:55Z

nipype/interfaces/base/specs.py

@@ -83,9 +83,11 @@ def __repr__(self):
        """ Return a well-formatted representation of the traits """
        outstr = []
        for name, value in sorted(self.trait_get().items()):
-            outstr.append('%s = %s' % (name, value))
+            if not name == '__all__':
+                outstr.append('%s = %s' % (name, value))


Is this still necessary, with __all__ becoming a property?

effigies · 2018-10-16T20:42:13Z

nipype/interfaces/base/specs.py

        return '\n{}\n'.format('\n'.join(outstr))

+


We should remove this line.

effigies · 2018-10-16T20:42:19Z

nipype/interfaces/base/specs.py

+    def __all__(self):
+        return [k for k,v in self.items() if not k == "__all__"]
+
+


Too many blank lines.

effigies · 2018-10-16T20:50:23Z

nipype/interfaces/base/specs.py

@@ -309,6 +311,11 @@ def _get_sorteddict(self,
                    out = objekt
        return out

+    @property
+    def __all__(self):
+        return [k for k,v in self.items() if not k == "__all__"]


Given that items() provides a dict keyed by self.copyable_trait_names(), and you only need keys, I think you can just make this:

@property def __all__(self): return self.copyable_trait_names()

You'll need to change the BET tests to either sort or check set equality, but this should work.

leej3 · 2018-10-16T21:31:42Z

Thanks. I made all those changes

effigies

LGTM, thanks.

effigies · 2018-10-17T00:18:23Z

nipype/interfaces/base/tests/test_specs.py

+
+    # Add trait and retest
+    list_extract._interface._output_names.append('added_out_trait')
+    expected_output = sorted(['added_out_trait',*expected_output])


Python 2.7, 3.4 don't like the [*x] syntax. How about making expected_output a set above, moving to set comparisons, and changing this to:

expected_output.add('added_out_trait')

nice. Thanks for the tip. done

effigies changed the title ~~PR: ENH add tab completion for node and interface inputs properties~~ ENH: Add tab completion for node and interface inputs properties Oct 13, 2018

leej3 added 2 commits October 13, 2018 16:03

modify BaseTraitedSpec for tab completion

7eb9d03

add tab completion with tests

d092d99

work still needed for dynamic traits

leej3 force-pushed the master branch from 6aff70c to d092d99 Compare October 15, 2018 18:01

fix tab completion for dynamic traits and tidy tests

e494b6b

effigies added this to the 1.1.4 milestone Oct 16, 2018

make tab_comletion test function a normal function

b83a0e8

effigies reviewed Oct 16, 2018

View reviewed changes

leej3 added 2 commits October 16, 2018 16:25

make fix for tab_comnpletion test

09372cd

due to an error in understanding of dynamic trait instantiation: output spec is actually instantiated in IOBase._outputs(), which calls Function._add_output_traits, which only will add things in self._output_names

remove superfluous __all__ definition

c695ed8

effigies reviewed Oct 16, 2018

View reviewed changes

tidy code for tab comletion of inputs and outputs

84ed702

effigies approved these changes Oct 16, 2018

View reviewed changes

effigies reviewed Oct 17, 2018

View reviewed changes

pep errors and use sets for tab_completion comparisons

59a4ba0

effigies merged commit eacd567 into nipy:master Oct 17, 2018

		def __all__(self):
		return [k for k,v in self.items() if not k == "__all__"]

ENH: Add tab completion for node and interface inputs properties #2735

ENH: Add tab completion for node and interface inputs properties #2735

Uh oh!

Conversation

leej3 commented Oct 12, 2018

Summary

List of changes proposed in this PR (pull-request)

Acknowledgment

Uh oh!

effigies commented Oct 13, 2018

Uh oh!

effigies commented Oct 13, 2018

Uh oh!

codecov-io commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leej3 commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Oct 15, 2018

Uh oh!

leej3 commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leej3 commented Oct 16, 2018

Uh oh!

effigies commented Oct 16, 2018

Uh oh!

leej3 commented Oct 16, 2018

Uh oh!

effigies Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leej3 Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

effigies Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

leej3 commented Oct 16, 2018

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

leej3 Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 15, 2018 •

edited

Loading

leej3 commented Oct 15, 2018 •

edited

Loading

leej3 commented Oct 16, 2018 •

edited

Loading

effigies Oct 16, 2018 •

edited

Loading