ENH CompCor #1599

berleant · 2016-09-01T02:42:21Z

Putting this up early

berleant · 2016-09-02T18:52:33Z

Yeah, I wanted to ask what (the best way to create test data is)/(if I should create test data). The ones I have in there are temporary.

chrisgorgo · 2016-09-02T19:36:28Z

nipype/algorithms/compcor.py


-class aCompCor(Node):
+class aCompCor(Workflow):


Shouldn't this be BaseInterface?

Like @satra 's comment on the issue (#1594), I am proposing to write an interface containing the core CompCor logic. tCompCor and aCompCor would be workflows that contain nodes for creating the masks, a node for the core CompCor logic, and possibly (?) nodes for applying the extracted noise components. What do you think?

I see... Workflows go here:
https://github.com/nipy/nipype/tree/master/nipype/workflows and follow a
slightly different pattern (they are functions returning a workflow object
rather than sublasses of Workflow).

I personally would just create two Interfaces: aCompCor(BaseInterface) that
takes a mask, calculates PCA and return the components, and
tCompCor(aCompCor) that takes a percentile argument n, creates a mask that
includes only the n% most variable voxels and reuses the code from its
parent aCompCor to return components. This is as flexible as the workflows,
but easier to integrate with pipelines as well as implement. Let me know
what do you think.

On Fri, Sep 2, 2016 at 10:16 PM, Shoshana Berleant <notifications@github.com

wrote:

In nipype/algorithms/compcor.py
#1599 (comment):

-class aCompCor(Node):
+class aCompCor(Workflow):

Like @satra https://github.com/satra 's comment on the issue (#1594
#1594), I am proposing to write an
interface containing the core CompCor logic. tCompCor and aCompCor would be
workflows that contain nodes for creating the masks, a node for the core
CompCor logic, and possibly (?) nodes for applying the extracted noise
components. What do you think?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nipy/nipype/pull/1599/files/50da63f9a8c74e60ef43b48349d9e5e19f5ad719..9e0d32796cec4ef237e2ccb2b7b74554f01c6ad8#r77431634,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOkp4CKN3GLOC9k9ZZtcY92UazGtooFks5qmQK7gaJpZM4JyQmp
.

…compcor1594

coveralls · 2016-09-02T20:13:27Z

Coverage decreased (-0.004%) to 72.259% when pulling b45b7b1 on shoshber:compcor1594 into da1f544 on nipy:master.

coveralls · 2016-09-02T20:13:27Z

Coverage decreased (-0.3%) to 71.98% when pulling b45b7b1 on shoshber:compcor1594 into da1f544 on nipy:master.

coveralls · 2016-09-04T03:53:54Z

Coverage decreased (-0.02%) to 72.242% when pulling 414f11a on shoshber:compcor1594 into da1f544 on nipy:master.

coveralls · 2016-09-04T03:53:54Z

Coverage decreased (-0.02%) to 72.242% when pulling 414f11a on shoshber:compcor1594 into da1f544 on nipy:master.

coveralls · 2016-09-04T22:50:30Z

Coverage decreased (-0.03%) to 72.228% when pulling bca197e on shoshber:compcor1594 into da1f544 on nipy:master.

coveralls · 2016-09-04T22:50:30Z

Coverage decreased (-0.3%) to 71.987% when pulling bca197e on shoshber:compcor1594 into da1f544 on nipy:master.

satra · 2016-09-06T07:08:00Z

nipype/algorithms/compcor.py

+class CompCoreOutputSpec(TraitedSpec):
+    components_file = File(desc='text file containing the noise components', exists=True)
+
+class CompCore(BaseInterface):


@shoshber - checking if this is a pun intended class name :) we should use the name from the paper just for searchability purposes. also see how doi's are now included to be able to generate a citation list for an interface.

Not the best place to mention this (I can make a separate issue) but is there a keyword search functionality in nipype? In other words it would be great if interfaces had keywords (like "motion correction", "brain masking"...it would be important to choose them carefully!) and then there was some way to search through them to find an interface that does what you want. Seems relevant here as it is best to have the name match what's in the paper, but then that name isn't actually all that descriptive about what the interface does!

Yes it's a pun :) I'll change it.

re doi's, is this what you're referring to? #1464

Yup! It's just a question of adding references_ field such as here:

nipype/nipype/interfaces/spm/base.py

Line 239 in 0f2cb9f

references_ = [{'entry': BibTeX("@book{FrackowiakFristonFrithDolanMazziotta1997,"

On Tue, Sep 6, 2016 at 2:15 PM, Shoshana Berleant notifications@github.com
wrote:

In nipype/algorithms/compcor.py
#1599 (comment):

+import numpy as np
+from scipy import linalg, stats
+import os
+
+from nipype.pipeline.engine import Workflow
+
+class CompCoreInputSpec(BaseInterfaceInputSpec):

realigned_file = File(exists=True, mandatory=True, desc='already realigned brain image (4D)')

mask_file = File(exists=True, mandatory=True, desc='mask file that determines ROI (3D)')

num_components = traits.Int(6, usedefault=True) # 6 for BOLD, 4 for ASL

additional_regressors??

+class CompCoreOutputSpec(TraitedSpec):

components_file = File(desc='text file containing the noise components', exists=True)

+class CompCore(BaseInterface):

Yes it's a pun :) I'll change it.

re doi's, is this what you're referring to? #1464
#1464

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nipy/nipype/pull/1599/files/ef50858d3b3016018f0078af6b7da29412803c00#r77720814,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOkp7UyOpbrANACeVwGabDKi06T-Wveks5qndgEgaJpZM4JyQmp
.

chrisgorgo · 2016-09-06T18:42:06Z

Please rebase or merge master to fix tests running.
Please get the doctests to run
shoshber: they run on my setup. we'll see about travis/circleCI
I would suggest renaming CompCore to aCompCor and creating a child class tCompCor that takes percentile argument and does the variance based CompCor (tCompCor) reusing code from it's parent class.
shoshber: per @satra 's suggestion above, I renamed CompCore to CompCor. It isn't really aCompCor. I will make tCompCor a class of CompCor

oesteban · 2016-09-07T05:32:47Z

I have the impression that absolute imports are messing up with the installed and the development versions in travis. I guess the easiest way to solve that is using explicit absolute imports.

chrisgorgo · 2016-09-07T05:40:51Z

I had a chat with @satra (we are both at #NHW16) about the temporal denoising. We think the best way to go forward is to add regress_poly (implemented like in https://github.com/nipy/nipype/blob/master/nipype/algorithms/misc.py#L312) to CompCor with defaults reflecting what is in the paper (I believe it's order 1 (linear) for aCompCor and order 2 (linear + qaudratic) for tCompCor, but please double check). It might be worthwhile to refactor tSNR to get the polynomial regression function shared between the interfaces.

coveralls · 2016-09-12T03:13:52Z

Coverage increased (+0.2%) to 72.508% when pulling c9aa6d6 on shoshber:compcor1594 into 10f28fe on nipy:master.

coveralls · 2016-09-12T03:13:52Z

Coverage increased (+0.2%) to 72.508% when pulling c9aa6d6 on shoshber:compcor1594 into 10f28fe on nipy:master.

coveralls · 2016-09-12T03:13:53Z

Coverage increased (+0.2%) to 72.508% when pulling c9aa6d6 on shoshber:compcor1594 into 10f28fe on nipy:master.

coveralls · 2016-09-12T03:13:53Z

Coverage increased (+0.2%) to 72.508% when pulling c9aa6d6 on shoshber:compcor1594 into 10f28fe on nipy:master.

codecov-io · 2016-09-12T09:53:46Z

Current coverage is 69.76% (diff: 99.02%)

No coverage report found for master at 10f28fe.

Powered by Codecov. Last update 10f28fe...cb2faa2

chrisgorgo · 2016-09-12T23:42:16Z

nipype/algorithms/compcor.py

+                                        exclude_low=True, exclude_high=True,
+                                        usedefault=True, desc='the percentile '
+                                        'used to select highest-variance '
+                                        'voxels. By default the 2% of voxels '


The default value is ".02" not "2%".

there should be some clarity that this should be expressed as value ranging from 0.0-1.0 rather than 0-100

I'll fix the documentation.

chrisgorgo · 2016-09-13T16:48:35Z

Other than the two comments I made (inf and extra_regressors) I think this is ready to merge.

satra · 2016-09-13T20:30:03Z

where did the duecredit reference go?

chrisgorgo · 2016-09-13T20:32:34Z

Good question - I saw it when I was reviewing this PR a few days ago. @shoshber ?

berleant · 2016-09-13T21:57:35Z

see #1630

compcor skeleton

50da63f

satra added the in-progress label Sep 1, 2016

incremental changes, baby test

9e0d327

add doctest

9f7886c

chrisgorgo reviewed Sep 2, 2016
View reviewed changes

Shoshana Berleant added 2 commits September 2, 2016 19:56

add doctest

55aec65

Merge branch 'compcor1594' of http://github.com/shoshber/nipype into …

b45b7b1

…compcor1594

Shoshana Berleant added 3 commits September 3, 2016 06:22

fix imports

f454cbe

better comments, minor re-arranging

ca930d3

set up test prereq's

414f11a

Shoshana Berleant added 2 commits September 4, 2016 22:35

complete _run_interface implementation

bca197e

implement _list_outputs

124d346

pretty ok test

ef50858

satra reviewed Sep 6, 2016
View reviewed changes

Shoshana Berleant added 5 commits September 6, 2016 21:39

compcore -> compcor

5e8b997

Merge http://github.com/nipy/nipype into compcor1594

6127924

TCompCor skeleton, tests

ac8dbfe

make python2 happy

a5402f7

factor out tSTD calculation

8f5eabc

Shoshana Berleant added 2 commits September 12, 2016 03:18

forgot to regress pre-computing std

ea60782

use relative paths in doctests

6a1497d

berleant added 3 - Review in-progress and removed in-progress 3 - Review labels Sep 12, 2016

Shoshana Berleant added 3 commits September 12, 2016 03:48

update documentation

e245073

robuster tests

6a88b3a

fix dumb error, comments

5347881

berleant added the needs-review label Sep 12, 2016

chrisgorgo reviewed Sep 12, 2016
View reviewed changes

clarify documentation

cb2faa2

remove stuff

bb45785

berleant added needs-review and removed in-progress needs-review labels Sep 13, 2016

berleant changed the title ~~WIP CompCor~~ ENH CompCor Sep 13, 2016

berleant mentioned this pull request Sep 13, 2016

fix up rsfmri_vol_surface_preprocessing_nipy.py #1616

Closed

chrisgorgo merged commit a746633 into nipy:master Sep 13, 2016

ENH CompCor #1599

ENH CompCor #1599

Uh oh!

Conversation

berleant commented Sep 1, 2016

Uh oh!

berleant commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

coveralls commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 2, 2016

Uh oh!

coveralls commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 4, 2016

Uh oh!

coveralls commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 4, 2016

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

additional_regressors??

Uh oh!

chrisgorgo commented Sep 6, 2016 • edited by berleant Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Sep 7, 2016

Uh oh!

chrisgorgo commented Sep 7, 2016

Uh oh!

coveralls commented Sep 12, 2016

Uh oh!

coveralls commented Sep 12, 2016

Uh oh!

coveralls commented Sep 12, 2016

Uh oh!

coveralls commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 69.76% (diff: 99.02%)

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!

chrisgorgo commented Sep 13, 2016

Uh oh!

satra commented Sep 13, 2016

Uh oh!

chrisgorgo commented Sep 13, 2016

Uh oh!

berleant commented Sep 13, 2016

Uh oh!

Uh oh!

berleant commented Sep 2, 2016 •

edited

Loading

coveralls commented Sep 2, 2016 •

edited

Loading

coveralls commented Sep 4, 2016 •

edited

Loading

coveralls commented Sep 4, 2016 •

edited

Loading

chrisgorgo commented Sep 6, 2016 •

edited by berleant

Loading

coveralls commented Sep 12, 2016 •

edited

Loading

codecov-io commented Sep 12, 2016 •

edited

Loading