ENH: Add cosine-basis high-pass-filter to CompCor #2107
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2107 +/- ##
===========================================
+ Coverage 48.56% 72.09% +23.52%
===========================================
Files 116 1144 +1028
Lines 23598 57675 +34077
Branches 0 8265 +8265
===========================================
+ Hits 11461 41582 +30121
- Misses 12137 14786 +2649
- Partials 0 1307 +1307
Continue to review full report at Codecov.
|
effigies
changed the title
|
This is ready to go. |
|
@effigies - the code looks reasonable. why are we adding high pass filtering to compcor? i can't recollect this being in the behzadi paper. further the polynomial regression achieves a similar effect. it's not a cosine basis, but the functionality is the same. |
nipype/algorithms/confounds.py
Outdated
| data = data.reshape((-1, timepoints)) | ||
|
|
||
| frametimes = timestep * np.arange(timepoints) | ||
| X = _full_rank(_cosine_drift(128, frametimes))[0] |
There was a problem hiding this comment.
this number appears to be coming from the spm default high-pass filter. if this is indeed necessary shouldn't the high-pass filter cutoff be a variable?
|
From Behzadi et al.
I'm definitely not an expert on the theory here, and certainly the Legendre basis set seems equally valid. One distinction I would draw between these approaches is in selecting the degree; perhaps there's a way to select the polynomial order based on frequency cutoff, but it's a very natural approach with a DCT basis. At present, the default order is 1, and I think I've seen "order 2 polynomial detrending" in papers to account for nonlinear drift. This is my first foray into CompCor territory, so I don't have strong opinions here. Arguments are welcome. Also, I did write this to be "side-by-side" compatible with polynomial detrending, but if it makes more sense to be an |
| @@ -945,3 +1022,69 @@ def _compute_tSTD(M, x, axis=0): | |||
| stdM[stdM == 0] = x | |||
| stdM[np.isnan(stdM)] = x | |||
| return stdM | |||
|
|
|||
|
|
|||
| # _cosine_drift and _full_rank copied from nipy/modalities/fmri/design_matrix | |||
There was a problem hiding this comment.
Is there a a big disadvantage of depending on nipy here instead of copying the code?
There was a problem hiding this comment.
Originally I did it simply to avoid adding a dependency. I ended up augmenting it to set a minimum order of 1.
|
Hi @satra - highpass filtering has been requested as a more explicit form of detrending by an FMRIPREP user. The idea is to capture nontrivial variation with the CompCor components. Providing a highpass cutoff rather than polynomial order seem to be a more interpretable way to do this kind of detrending. The functionality is optional and should not interfere with the traditional uses. |
|
@effigies - it would be good to make regress_poly and high_pass_filter be xor - since they are supposed to achieve similar end goals. if we are going to do high-pass-filter does it make sense to simply do bandpass filter? i don't know why it should be high-pass in particular? @chrisfilo - on the user side, the goal of compcor is to remove nuisance projectors. to some extent this overlaps with the notion of temporal filtering. the reason a basic polynomial regressor was used was to clearly take out linear/quadratic trends as per the paper. by allowing for more general filtering, users will have to be careful as to what they are really meaning by nuisance and what impact such filtering has on subsequent steps. finally, by bringing more general filtering into the picture, everything about filter design comes into play. something often ignored in the algorithms that implement this. |
|
+1 for xor
- bandpass might be a bit harder to implement (I'm not sure it can be done
just with cosines) - I would leave it for another PR
- I do agree this might seem more complex. The idea is that users wanting
to limit CompCor just to higher frequencies should include the high pass
cosine regressors returned by this interface. This way you can get the full
set of regressors used in CompCor for denoising (high pass cosines and PCA
components). From what I am hearing from users this is a valuable option to
have.
On Jul 5, 2017 2:05 PM, "Satrajit Ghosh" <notifications@github.com> wrote:
@effigies <https://github.com/effigies> - it would be good to make
regress_poly and high_pass_filter be xor - since they are supposed to
achieve similar end goals. if we are going to do high-pass-filter does it
make sense to simply do bandpass filter? i don't know why it should be
high-pass in particular?
@chrisfilo <https://github.com/chrisfilo> - on the user side, the goal of
compcor is to remove nuisance projectors. to some extent this overlaps with
the notion of temporal filtering. the reason a basic polynomial regressor
was used was to clearly take out linear/quadratic trends as per the paper.
by allowing for more general filtering, users will have to be careful as to
what they are really meaning by nuisance and what impact such filtering has
on subsequent steps. finally, by bringing more general filtering into the
picture, everything about filter design comes into play. something often
ignored in the algorithms that implement this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2107 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp2nIcffejro7qFmAPFE25T7QGtK7ks5sK9BqgaJpZM4OJzoO>
.
|
nipype/algorithms/confounds.py
Outdated
| num_components = traits.Int(6, usedefault=True) # 6 for BOLD, 4 for ASL | ||
| use_regress_poly = traits.Bool(True, usedefault=True, | ||
| use_regress_poly = traits.Bool(True, usedefault=True, xor=['high_pass_filter'], |
There was a problem hiding this comment.
After adding the xor entries, in order to use high_pass_filter, we need to turn off usedefault here, which means we could be breaking existing scripts in one way or another.
Given that, maybe the better way to do this is to switch to an Enum, such as
pre_filter=traits.Enum('polynomial', 'high_pass', False, usedefault=True,
desc='Detrend time series prior to component extraction')WDYT?
|
Sounds like a great idea to me, but we have to make sure it's backwards
compatible.
…On Jul 5, 2017 3:38 PM, "Chris Markiewicz" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In nipype/algorithms/confounds.py
<#2107 (comment)>:
> num_components = traits.Int(6, usedefault=True) # 6 for BOLD, 4 for ASL
- use_regress_poly = traits.Bool(True, usedefault=True,
+ use_regress_poly = traits.Bool(True, usedefault=True, xor=['high_pass_filter'],
After adding the xor entries, in order to use high_pass_filter, we need
to turn off usedefault here, which means we could be breaking existing
scripts in one way or another.
Given that, maybe the better way to do this is to switch to an Enum, such
as
pre_filter=traits.Enum('polynomial', 'high_pass', False, usedefault=True,
desc='Detrend time series prior to component extraction')
WDYT?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2107 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAOkp8VWrag1sBoX7Ra3tzVaipshiXyvks5sK-ZNgaJpZM4OJzoO>
.
|
effigies
force-pushed
the
enh/hpf_compcor
branch
2 times, most recently
from
e0d2ad3 to
56ff3c2
Compare
|
Okay. I've moved to an I'm also generally making the filtering regressors available. There's no cost to keeping them around if people want. |
| desc='Detrend time series prior to component ' | ||
| 'extraction') | ||
| use_regress_poly = traits.Bool(True, | ||
| deprecated='0.15.0', new_name='pre_filter', |
There was a problem hiding this comment.
@satra I set this to deprecate at 0.15.0, two minor versions in the future. Should that be 3.0.0, instead? It's pretty unclear what a deprecation schedule should look like.
|
@satra I think this is ready to go, but want your input on the deprecation. |
|
@effigies - looks fine to me. at this point it's possible it will get deprecated with the planned 1.0 release in fall. i think that's ok. |
Changes proposed in this pull request
high_pass_filterTodo:
make specs