Fix/compcor #1992

satra · 2017-05-02T23:07:02Z

closes #1982 supercedes #1984

…functions

satra · 2017-05-02T23:08:44Z

confounds.py and test_compcor.py are the target files - a few other style and test fixes got included.

codecov-io · 2017-05-03T00:56:34Z

Codecov Report

Merging #1992 into master will increase coverage by 0.02%.
The diff coverage is 90.67%.

@@            Coverage Diff             @@
##           master    #1992      +/-   ##
==========================================
+ Coverage   72.31%   72.33%   +0.02%     
==========================================
  Files        1089     1089              
  Lines       55624    55623       -1     
  Branches     8017     8012       -5     
==========================================
+ Hits        40222    40234      +12     
+ Misses      14148    14138      -10     
+ Partials     1254     1251       -3

Flag	Coverage Δ
#smoketests	`72.33% <90.67%> (+0.02%)`	⬆️
#unittests	`69.86% <90.67%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/afni/preprocess.py	`85.61% <ø> (ø)`	⬆️
nipype/interfaces/afni/utils.py	`75.76% <ø> (ø)`	⬆️
nipype/algorithms/tests/test_auto_ACompCor.py	`85.71% <ø> (ø)`	⬆️
nipype/algorithms/tests/test_auto_TCompCor.py	`85.71% <100%> (-14.29%)`	⬇️
nipype/algorithms/tests/test_compcor.py	`100% <100%> (ø)`	⬆️
nipype/interfaces/fsl/utils.py	`72.73% <100%> (ø)`	⬆️
nipype/algorithms/confounds.py	`77.93% <89.32%> (+3.28%)`	⬆️

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 a092cde...85b5692. Read the comment docs.

mgxd · 2017-05-03T15:38:32Z

@satra still running into the error mentioned in #1982, mapnode of the interface causes a crash

tcompcor = MapNode(TCompCor(), 
                   iterfield=['mask_files', 'realigned_file'],
                   name='tcompcor')

func1 = 'somefunc'
func2 = 'anotherfunc'

tcompcor.inputs.components_file = 'comp_file.txt'
tcompcor.inputs.realigned_file = ['somefunc', 'anotherfunc']
tcompcor.inputs.mask_files = ['mask1', 'mask2']
tcompcor.run()

Automatic pdb calling has been turned ON
170503-11:32:14,653 workflow INFO:
	 Executing node tcompcor in dir: /code/tcompcor/tcompcor
170503-11:32:14,659 workflow INFO:
	 Executing node _tcompcor0 in dir: /code/tcompcor/tcompcor/mapflow/_tcompcor0
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-46cbc0876b60> in <module>()
      1 get_ipython().magic('pdb')
----> 2 eg = tcompcor.run()

/code/nipype/nipype/pipeline/engine/nodes.py in run(self, updatehash)
    370                     self.inputs.get_traitsfree())
    371             try:
--> 372                 self._run_interface()
    373             except:
    374                 os.remove(hashfile_unfinished)

/code/nipype/nipype/pipeline/engine/nodes.py in _run_interface(self, execute, updatehash)
   1285             nodenames = ['_' + self.name + str(i) for i in range(nitems)]
   1286             self._collate_results(self._node_runner(self._make_nodes(cwd),
-> 1287                                                     updatehash=updatehash))
   1288             self._save_results(self._result, cwd)
   1289             # remove any node directories no longer required

/code/nipype/nipype/pipeline/engine/nodes.py in _collate_results(self, nodes)
   1170                         values = []
   1171                     if node.result.outputs:
-> 1172                         values.insert(i, node.result.outputs.get()[key])
   1173                     else:
   1174                         values.insert(i, None)

AttributeError: 'str' object has no attribute 'insert'

> /code/nipype/nipype/pipeline/engine/nodes.py(1172)_collate_results()
   1170                         values = []
   1171                     if node.result.outputs:
-> 1172                         values.insert(i, node.result.outputs.get()[key])
   1173                     else:
   1174                         values.insert(i, None)

ipdb> self.outputs
Bunch(components_file='components_file.txt', header=<undefined>, high_variance_masks=<undefined>, ignore_exception=False, mask_files=<undefined>, mask_index=<undefined>, merge_method=<undefined>, num_components=6, realigned_file=<undefined>, regress_poly_degree=1, use_regress_poly=True)

After running into this, I'm wondering if we should add a basic check at the node.output level that converts the value to a list, something like

if not isinstance(values, list):
    values = [values]

since we are assuming it is a list anyways.

satra · 2017-05-03T15:47:56Z

@mgxd - is this with python 3 or 2? if 2, can you check with python 3? and are you using my branch to check?

mgxd · 2017-05-03T15:50:02Z

py-3.6.1, your branch

* upstream/master: enh: preliminary testing + altered auto tests minor typo Instead of invoking Popen, use CommandLine fix: clarity allow_compression fix: trailing parenthesis enh: imagetype base for spm, new image formats, allow_compression wip: check against image format enh: new traits applied to spm fix: catch compressed files for spm realign set memory minimum to 0.25 GB [ENH] Migrating resource handler to Node level

satra · 2017-05-03T15:54:32Z

@mgxd - thanks taking a look

chrisgorgo

Sorry I did not go through the original PR thoroughly enough.

There is some serious refactoring going on (+1) which makes the diff hard to read, but I trust the tests.

chrisgorgo · 2017-05-03T16:01:32Z

nipype/algorithms/tests/test_compcor.py

@@ -142,7 +142,8 @@ def run_cc(self, ccinterface, expected_components, expected_header='CompCor'):
            components_data = [line.split('\t') for line in components_file]

            header = components_data.pop(0) # the first item will be '#', we can throw it out
-            expected_header = [expected_header + str(i) for i in range(expected_n_components)]
+            expected_header = [expected_header + '{:02d}'.format(i) for i in
+                               range(expected_n_components)]


It's ok to change those names now, but after this will be released we need to be more careful since it's technically an API change.

chrisgorgo · 2017-05-03T16:31:56Z

nipype/algorithms/confounds.py

+                               desc=('Merge method if multiple masks are '
+                                     'present - `union` aggregates all masks, '
+                                     '`intersect` computes the truth value of '
+                                     'all masks, `none` performs CompCor on '


What about

union uses voxels included in at least one input mask, intersect uses only voxels present in all input masks

chrisgorgo · 2017-05-03T16:33:50Z

nipype/algorithms/confounds.py

+    mask_index = traits.Range(low=0, xor=['merge_method'],
+                              requires=['mask_files'],
+                              desc=('Position of mask in `mask_files` to use - '
+                                    'first is the default'))


This is a bit confusing, because it suggests that only the first mask will be used by default. Also the field does have usedefaults. Is it a single index or range? This should be fleshed out in the description.

chrisgorgo · 2017-05-03T16:34:35Z

nipype/algorithms/confounds.py

+                                       desc='the degree polynomial to use')
+    header = traits.Str(desc=('the desired header for the output tsv file (one '
+                              'column). If undefined, will default to '
+                              '"CompCor"'))


Since we are breaking API I would rename this to header_prefix

satra · 2017-05-03T21:05:16Z

this should be fixed now - good catch. the TCompCorOutputSpec was derived from CompCorInputSpec

https://github.com/nipy/nipype/blob/master/nipype/algorithms/confounds.py#L493

i also missed it during the refactor.

mgxd · 2017-05-03T21:11:07Z

nipype/algorithms/confounds.py

        np.savetxt(components_file, components, fmt=b"%.10f", delimiter='\t',
                   header=self._make_headers(components.shape[1]), comments='')
        return runtime

+    def _process_masks(self, mask_images, timeseries=None):


for my curiosity - why split this up?

mgxd · 2017-05-03T21:11:49Z

nipype/algorithms/confounds.py

+class TCompCorOutputSpec(CompCorOutputSpec):
+    # and all the fields in CompCorOutputSpec
+    high_variance_masks = OutputMultiPath(File(exists=True),
+                                          desc=("voxels excedding the variance "


mgxd · 2017-05-03T21:12:23Z

nipype/algorithms/confounds.py

+        self._header = 'tCompCor'
+        self._mask_files = []
+
+    def _process_masks(self, mask_images, timeseries=None):


ah - I see now.

satra · 2017-05-03T22:00:37Z

@chrisfilo - description changes updated. the traits Range trait is still an integer (or float) just allows validating with high and low.

satra · 2017-05-03T22:01:35Z

@chrisfilo - if you have a chance to run fmriprep and compare that would be good feedback as well. should work as normal.

satra · 2017-05-04T21:15:52Z

@chrisfilo - yeah, nay?

satra added 2 commits May 2, 2017 19:05

fix: refactor compcor to not assign to self.inputs and re-use common …

15440fd

…functions

fix: clean auto tests and white spaces

c0a3c7f

satra requested review from mgxd and chrisgorgo May 2, 2017 23:08

satra mentioned this pull request May 3, 2017

fix: renamed mask_file to mask_files #1984

Closed

chrisgorgo reviewed May 3, 2017

View reviewed changes

fix: ensure outputspec is derived from outputspec

ca205ef

mgxd approved these changes May 3, 2017

View reviewed changes

satra added 3 commits May 3, 2017 17:36

fix: clean up tests and help messages

3551a97

fix: typo

783c6a7

fix: update autotests

85b5692

satra merged commit a01275f into nipy:master May 5, 2017

satra deleted the fix/compcor branch October 30, 2017 15:16

Fix/compcor #1992

Fix/compcor #1992

Uh oh!

Conversation

satra commented May 2, 2017

Uh oh!

satra commented May 2, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mgxd commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satra commented May 3, 2017

Uh oh!

mgxd commented May 3, 2017

Uh oh!

satra commented May 3, 2017

Uh oh!

chrisgorgo left a comment

Choose a reason for hiding this comment

Uh oh!

chrisgorgo May 3, 2017

Choose a reason for hiding this comment

Uh oh!

chrisgorgo May 3, 2017

Choose a reason for hiding this comment

Uh oh!

chrisgorgo May 3, 2017

Choose a reason for hiding this comment

Uh oh!

chrisgorgo May 3, 2017

Choose a reason for hiding this comment

Uh oh!

satra commented May 3, 2017

Uh oh!

mgxd May 3, 2017

Choose a reason for hiding this comment

Uh oh!

mgxd May 3, 2017

Choose a reason for hiding this comment

Uh oh!

mgxd May 3, 2017

Choose a reason for hiding this comment

Uh oh!

satra commented May 3, 2017

Uh oh!

satra commented May 3, 2017

Uh oh!

satra commented May 4, 2017

Uh oh!

Uh oh!

codecov-io commented May 3, 2017 •

edited

Loading

mgxd commented May 3, 2017 •

edited

Loading