Sort out terminal_output #2209

oesteban · 2017-10-04T18:26:12Z

This PR revises the terminal_output feature, making it a class attribute and deprecating the old InputSpec trait.

It also implements new ways of storing the output of a CommandLine, as discussed in #1407. Thus, closes #1407.

Work in progress:

Update documentation
Before merging, I make specs should be run. I'm holding off to ease code review.

…o enh/FSLstd2imgcoords

This PR revises the terminal_output feature, making it a class attribute and deprecating the old InputSpec trait. It also implements new ways of storing the output of a CommandLine, as discussed in nipy#1407. Thus, closes nipy#1407.

Integrate the code from that branch that was stale waiting for `terminal_output` to be revised. Close nipy#1398.

codecov-io · 2017-10-05T09:29:38Z

Codecov Report

Merging #2209 into master will increase coverage by <.01%.
The diff coverage is 87.8%.

@@            Coverage Diff             @@
##           master    #2209      +/-   ##
==========================================
+ Coverage   72.26%   72.27%   +<.01%     
==========================================
  Files        1177     1178       +1     
  Lines       58642    58695      +53     
  Branches     8446     8453       +7     
==========================================
+ Hits        42376    42419      +43     
- Misses      14902    14909       +7     
- Partials     1364     1367       +3

Flag	Coverage Δ
#smoketests	`72.27% <87.8%> (ø)`	⬆️
#unittests	`69.81% <84.75%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/fsl/__init__.py	`100% <ø> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_ROIStats.py	`85.71% <ø> (ø)`	⬆️
nipype/interfaces/tests/test_base.py	`96.81% <100%> (+0.07%)`	⬆️
nipype/interfaces/matlab.py	`79.79% <100%> (ø)`	⬆️
...pe/interfaces/afni/tests/test_auto_OutlierCount.py	`85.71% <100%> (-14.29%)`	⬇️
nipype/pipeline/plugins/multiproc.py	`77.94% <100%> (ø)`	⬆️
nipype/interfaces/afni/preprocess.py	`80.75% <46.15%> (-0.55%)`	⬇️
nipype/interfaces/freesurfer/base.py	`73.07% <50%> (-0.61%)`	⬇️
nipype/interfaces/fsl/utils.py	`72.91% <82.35%> (+0.18%)`	⬆️
...nterfaces/fsl/tests/test_auto_WarpPointsFromStd.py	`85.71% <85.71%> (ø)`
... and 4 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 2e5f5fc...82263dc. Read the comment docs.

codecov-io · 2017-10-05T09:30:14Z

Codecov Report

Merging #2209 into master will decrease coverage by <.01%.
The diff coverage is 82.75%.

@@            Coverage Diff            @@
##           master   #2209      +/-   ##
=========================================
- Coverage    72.3%   72.3%   -0.01%     
=========================================
  Files        1178    1179       +1     
  Lines       58768   58789      +21     
  Branches     8458    8463       +5     
=========================================
+ Hits        42494   42509      +15     
- Misses      14901   14905       +4     
- Partials     1373    1375       +2

Flag	Coverage Δ
#smoketests	`72.3% <82.75%> (-0.01%)`	⬇️
#unittests	`69.87% <79.31%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/afni/tests/test_auto_Qwarp.py	`85.71% <ø> (ø)`	⬆️
...pe/interfaces/freesurfer/tests/test_auto_MS_LDA.py	`85.71% <ø> (ø)`	⬆️
...ipype/interfaces/fsl/tests/test_auto_ApplyTOPUP.py	`85.71% <ø> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_Axialize.py	`85.71% <ø> (ø)`	⬆️
...pype/interfaces/camino/tests/test_auto_Shredder.py	`85.71% <ø> (ø)`	⬆️
...nterfaces/camino/tests/test_auto_TrackBootstrap.py	`85.71% <ø> (ø)`	⬆️
nipype/interfaces/afni/tests/test_auto_Refit.py	`85.71% <ø> (ø)`	⬆️
...erfaces/freesurfer/tests/test_auto_MakeSurfaces.py	`85.71% <ø> (ø)`	⬆️
...ipype/interfaces/afni/tests/test_auto_ABoverlap.py	`85.71% <ø> (ø)`	⬆️
nipype/interfaces/fsl/tests/test_auto_BEDPOSTX5.py	`85.71% <ø> (ø)`	⬆️
... and 694 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 4d08138...a66678c. Read the comment docs.

oesteban · 2017-10-05T13:20:33Z

@satra, @effigies, this is ready for final review

satra · 2017-10-05T13:38:04Z

@oesteban - this will need to resolve a few things post the display PR merge.

effigies · 2017-10-05T13:46:24Z

@satra @oesteban The conflicts looked simple to resolve to me, so I went ahead and did it to get the tests queued up. Please check the diff to make sure you agree with the changes.

oesteban · 2017-10-05T16:10:19Z

Testing one conflict that wasn't correctly resolved. If it passes locally I'll push very soon. Thanks a lot @effigies for the merge with upstream :)

effigies

Looks good. Everything I saw ended up being stylistic, so feel free to ignore.

effigies · 2017-10-05T18:10:13Z

nipype/interfaces/base.py

-            if sep is None:
-                sep = ' '
-            if argstr.endswith('...'):
+            sep = trait_spec.sep or ' '


This will replace '' with ' '. I'm not sure that's what people will expect. Actually equivalent:

sep = trait_spec.sep if trait_spec.sep is not None else ' '

effigies · 2017-10-05T18:12:15Z

nipype/interfaces/base.py

-            return self._gen_outfilename()
-        else:
-            return None
+        return self._gen_outfilename() if name == 'out_file' else None


Functions return None implicitly. This could just drop the else condition.

def _getn_filename(self, name): if name == 'out_file': return self._gen_outfilename()

I'm not sure which one is easier to understand. Your choice.

I generally prefer explicit over implicit ...

effigies · 2017-10-05T18:15:37Z

nipype/pipeline/plugins/multiproc.py

+        # Don't allow streaming outputs
+        if hasattr(node.interface, 'terminal_output') and \
+                node.interface.terminal_output == 'stream':
+            node.interface.terminal_output = 'allatonce'


This might be something I messed up with my merge, but I think the getattr approach is cleaner.

# Don't allow streaming outputs if getattr(node.interface, 'terminal_output', '') == 'stream': node.interface.terminal_output = 'allatonce'

satra · 2017-10-05T18:41:41Z

nipype/interfaces/base.py

@@ -1457,7 +1471,7 @@ class must be instantiated with a command argument
    {'args': '-al',
     'environ': {'DISPLAY': ':1'},
     'ignore_exception': False,
-     'terminal_output': 'stream'}
+     'terminal_output': <undefined>}


this is syntactically off.

what about:

# Use get_traitsfree() to check all inputs set >>> pprint.pprint(cli.inputs.get_traitsfree()) # doctest: +NORMALIZE_WHITESPACE +ALLOW_UNICODE {'args': '-al', 'environ': {'DISPLAY': ':1'}, 'ignore_exception': False}

satra · 2017-10-05T18:43:24Z

nipype/interfaces/base.py

@@ -1652,7 +1672,7 @@ def _filename_from_source(self, name, chain=None):
            ns = trait_spec.name_source
            while isinstance(ns, (list, tuple)):
                if len(ns) > 1:
-                    iflogger.warn('Only one name_source per trait is allowed')
+                    iflogger.warning('Only one name_source per trait is allowed')


i think in a next refactor, we should move this up to the base class.

satra · 2017-10-05T18:46:48Z

nipype/interfaces/afni/tests/test_auto_ROIStats.py

@@ -25,9 +25,8 @@ def test_ROIStats_inputs():
    quiet=dict(argstr='-quiet',
    position=1,
    ),
-    terminal_output=dict(mandatory=True,
+    terminal_output=dict(deprecated='1.0.0',


can we set version temporarily to 1.0.0 and make sure nothing breaks :) doesn't have to be a in a test. could be your local test.

Running locally with version 1.0.1. Will also run some examples.

Confirmed it runs correctly.

satra · 2017-10-05T18:48:47Z

doc/devel/interface_specs.rst

+
+  import nipype.interfaces.fsl as fsl
+  mybet = fsl.BET(from_file='bet-settings.json')
+  mybet.terminal_output = 'file_split'


does this suggest that we should consider two types of fields in the json files? things that set node/interface attributes, like mem, proc, terminal_output, etc.,. and then a section that's exclusively the inputs?

good point. We could think of a JSON representation of the interface state for nipype 2.0, don't you think?

satra

in general things look good, left a few comments.

oesteban · 2017-10-05T21:14:09Z

I still have to add in your comments and make specs. Please hold off merging till then.

oesteban added 25 commits March 1, 2016 13:34

[ENH] Added new fsl.WarpPointsFromStd interface

ba97ef1

Merge branch 'master' into enh/FSLstd2imgcoords

4bdd241

do not inherit from WarpPoints interfaces

82e1ce7

update specs

91a7ac4

updated CHANGES, fixed docstring

f5bae9f

Merge branch 'master' into enh/FSLstd2imgcoords

4540fd2

use terminal_output

4d9a080

Merge branch 'enh/FSLstd2imgcoords' of github.com:oesteban/nipype int…

57d3a3b

…o enh/FSLstd2imgcoords

Merge remote-tracking branch 'upstream/master' into enh/FSLstd2imgcoords

e5a73b7

add +IGNORE_UNICODE to doctest, update specs

e7e4dcc

Merge branch 'master' into enh/FSLstd2imgcoords

51a7e72

[WIP] Revise terminal_output

9e3ee31

This PR revises the terminal_output feature, making it a class attribute and deprecating the old InputSpec trait. It also implements new ways of storing the output of a CommandLine, as discussed in nipy#1407. Thus, closes nipy#1407.

fix multiproc access to terminal_output

77bcb60

update failing spec

e269e74

fix terminal_output tests

484da5d

Merge remote-tracking branch 'upstream/master' into fix/1407

037e932

Merge branch 'enh/FSLstd2imgcoords' into fix/1407

91a0194

Integrate the code from that branch that was stale waiting for `terminal_output` to be revised. Close nipy#1398.

fix xor in AFNI OutlierCount. fixes nipy#1406

617bba0

update specs of the two interfaces affected so far

19a356b

make ants.OutlierCount use the terminal_output. Close nipy#1406

1d22038

update documentation

8b08f90

fix tests

6279fb2

move documentation to the interface_specs file

0abc8ac

fix CHANGES

5372a79

remove unnecessary dependency

82263dc

Merge branch 'master' into fix/1407

ec4d612

effigies mentioned this pull request Oct 5, 2017

PIN: Update nipype nipreps/niworkflows#197

Merged

2 tasks

fix tests

d951ab4

oesteban requested review from effigies and satra October 5, 2017 17:19

effigies approved these changes Oct 5, 2017

View reviewed changes

satra reviewed Oct 5, 2017

View reviewed changes

oesteban added 4 commits October 5, 2017 19:09

Merge remote-tracking branch 'upstream/master' into fix/1407

4d4d3f0

@effigies' and @satra's comments

799dfab

fix overcorrection

8ed12f6

update specs

a66678c

satra merged commit 2b10ff2 into nipy:master Oct 6, 2017

oesteban deleted the fix/1407 branch October 6, 2017 16:33

satra added this to the 0.14.0 milestone Oct 20, 2017

mgxd mentioned this pull request Nov 27, 2017

Question: Significance of different terminal_output values when not explicitly set? #2123

Open

Sort out terminal_output #2209

Sort out terminal_output #2209

Uh oh!

Conversation

oesteban commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 5, 2017

Codecov Report

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

oesteban commented Oct 5, 2017

Uh oh!

satra commented Oct 5, 2017

Uh oh!

effigies commented Oct 5, 2017

Uh oh!

oesteban commented Oct 5, 2017

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satra left a comment

Choose a reason for hiding this comment

Uh oh!

oesteban commented Oct 5, 2017

Uh oh!

Uh oh!

oesteban commented Oct 4, 2017 •

edited

Loading

codecov-io commented Oct 5, 2017 •

edited

Loading