tedana icon indicating copy to clipboard operation
tedana copied to clipboard

[DOC] Add documentation page on denoising approaches

Open tsalo opened this issue 2 years ago • 20 comments

Closes None, but stems from #819.

Changes proposed in this pull request:

  • Add page on denoising strategies. I still need to figure out how to better integrate it into our documentation.
  • Add sphinx-inline-tabs to documentation requirements.
  • Add missing carpet plot (closes #803).

To do:

  • [x] Figure out where to put the new page/information in our documentation.
  • [ ] Add AFNI code to do the denoising.
  • [ ] Discuss when someone would want to use each approach and why.

tsalo avatar Oct 22 '21 18:10 tsalo

Check it out: https://tedana--823.org.readthedocs.build/en/823/denoising.html

tsalo avatar Oct 22 '21 18:10 tsalo

Codecov Report

Base: 93.30% // Head: 93.30% // No change to project coverage :thumbsup:

Coverage data is based on head (33ff086) compared to base (d85dac3). Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #823   +/-   ##
=======================================
  Coverage   93.30%   93.30%           
=======================================
  Files          28       28           
  Lines        2345     2345           
=======================================
  Hits         2188     2188           
  Misses        157      157           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Oct 22 '21 19:10 codecov[bot]

I really like the idea of adding several example preprocessing pipelines and noise regression pipelines to the documentation. I think the question of how to connect this information to the documentation and what to include on this page are linked. In the current text, these examples are linked with an explanation of aggressive vs non-aggressive denoising. Would you consider calling this something like "Using tedana output with other programs"? Then the focus will be how to use the output (including for aggressive vs non-aggressive denoising). It would also then fit as a link off off "Outputs of tedana"

Other general comments:

  • You're saying tedana is doing non-aggressive denoising, but by your definition, I think it's aggressive. See: https://github.com/ME-ICA/tedana/blob/f1e98cb1630ebdfd6ba044ea16e4c06f4a3830df/tedana/io.py#L378-L379 The rejected components are fit to the data without regard to the regressors of interest.
  • Looking at the python code sample I think you're trying to show how regress out things like motion in addition to the tedana rejected time series, but you don't clearly say that anywhere.
  • Perhaps this can be reorganized to show how to load the ICA mixing matrix and component table into python/AFNI/FSL and then regress out those data in isolation or in combination with other noise regressors. You'd still cover what you're calling aggressive vs non-aggressive, but focusing on that distinction is making the main use of these examples a bit more confusing.
  • Why are we including FSL if FSL currently is currently so multi-echo unfriendly? If someone knows how to load the time series and regress them out in FSL, I'm fine including that example, but I don't feel like re-learning enough of FSL to get that working, and I'm hesitant to present a use case that none of the current tedana developers are using/supporting.

handwerkerd avatar Nov 15 '21 18:11 handwerkerd

In the current text, these examples are linked with an explanation of aggressive vs non-aggressive denoising. Would you consider calling this something like "Using tedana output with other programs"? Then the focus will be how to use the output (including for aggressive vs non-aggressive denoising). It would also then fit as a link off off "Outputs of tedana"

To be honest, part of my goal was to document the differences in a general sense, so that AROMA users (or whomever) could read our documentation and understand what their own outputs mean. I'm hoping to link to whatever page we add this to in the AROMA documentation, and potentially in other related packages as well. That said, as long as we have some kind of subsection that is more specifically about different denoising approaches, I'm comfortable changing the file.

The rejected components are fit to the data without regard to the regressors of interest.

True, but the parameter estimates come from all components in the mixing matrix:

https://github.com/ME-ICA/tedana/blob/0f04f76bb51fc2ba8117e703c0ee0c9117c2388c/tedana/io.py#L372

Wouldn't that make it non-aggressive denoising?

Perhaps this can be reorganized to show how to load the ICA mixing matrix and component table into python/AFNI/FSL and then regress out those data in isolation or in combination with other noise regressors.

Great point. I can do that with the Python portion at least.

Why are we including FSL if FSL currently is currently so multi-echo unfriendly?

I just wanted to try it out with more than two tabs, but am happy to remove it since no one in the group uses it.

tsalo avatar Nov 15 '21 19:11 tsalo

Good point on the betas estimate. I think this makes me double-down on how aggressive vs non-aggressive can be confusing language. I'm also realizing we've sometimes used "aggressive"/"conservative" to describe variations in decision trees. We might need to be more precise (and unfortunately more wordy) We can say that noise removal is regressing out:

  • Rejected time series without regard to accepted time series (your aggressive)
  • Rejected time series in a model that also fits accepted time series (your non-aggressive) (effectively splits the shared signals of accepted and rejected time series)
  • Only the parts of rejected time series that aren't in the accepted time series (your orthogonalization) Note: rejected time series could be just what is rejected by tedana or also include things like motion or respiratory regressors.

If you want three tabs, would there be any difference between fmriprep and python?

handwerkerd avatar Nov 15 '21 19:11 handwerkerd

I think it's important that we incorporate external nuisance regressors and regressors of interest, but I'm not sure if that should go in the main part of each section or in separate parts. Maybe separate tabs? Like:

[ Python ] [ Python with external regressors ] [ AFNI ] [AFNI with external regressors ]

WDYT? Would having separate sections, like Nonaggressive denoising, followed by Nonaggressive denoising with external regressors be better?

If you want three tabs, would there be any difference between fmriprep and python?

There probably will be some differences in filenames once we merge in some of the changes we've been discussing with the fMRIPrep team, but I don't think the core operations (e.g., what type of file each element is stored in) will differ. That said, I think folks will start copying and pasting our example code into their own scripts, so it might be best to have examples for each of the major naming conventions, even when the core code remains the same.

tsalo avatar Nov 16 '21 16:11 tsalo

I like your tabs because they explain a bit more clearly what each includes. I'd lean into your tab suggestions more and use more mathy terminology rather than "aggressive" "non-aggressive". That would mean something like "Remove all noise-correlated fluctuations" "Only remove noise-correlated fluctuations that aren't correlated with fluctuations in accepted components" (That's probably a bit too wordy)

handwerkerd avatar Nov 17 '21 21:11 handwerkerd

@handwerkerd what difference, if any, is there between orthogonalizing the rejected components w.r.t. the accepted ones and including both rejected and accepted components in the initial regression (i.e., nonaggressive denoising)?

I'd lean into your tab suggestions more and use more mathy terminology rather than "aggressive" "non-aggressive". That would mean something like "Remove all noise-correlated fluctuations" "Only remove noise-correlated fluctuations that aren't correlated with fluctuations in accepted components" (That's probably a bit too wordy)

I've added the longer headings and we can iterate on the descriptions. The biggest things I want to focus on are when and why someone would choose each approach, but I don't know enough about each to say.

tsalo avatar Nov 18 '21 18:11 tsalo

@handwerkerd what difference, if any, is there between orthogonalizing the rejected components w.r.t. the accepted ones and including both rejected and accepted components in the initial regression (i.e., nonaggressive denoising)?

Maybe @CesarCaballeroGaudes and @smoia want to comment on this.

eurunuela avatar Nov 18 '21 18:11 eurunuela

I hesitated to tag Cesar because I remember him very patiently explaining it to me in DC and I've completely forgotten it all! 😆

tsalo avatar Nov 18 '21 18:11 tsalo

I hesitated to tag Cesar because I remember him very patiently explaining it to me in DC and I've completely forgotten it all! 😆

I believe he wanted to contribute to this PR.

eurunuela avatar Nov 18 '21 18:11 eurunuela

This isn't quite right but hopefully it helps explain. Accepted components X includes fluctuations A, B, C Rejected components Y includes fluctuations C, D, E Aggressive denoising removes Y without regard to X, therefeore, C, D, & E are all removed. If A-E are all fit in the model, then, when you remove the rejected comopnents, you removed 0.5*C, D, & E since C is split. If you Y from X first, then you remove anything in Y that's common to X. In this case, only D & E are removed.

handwerkerd avatar Nov 18 '21 20:11 handwerkerd

Hey, I'm listening 😄 (in fact I receive all TEDANA notifications). I was thinking for working on this PR or the AROMA during the forthcoming BHD

CesarCaballeroGaudes avatar Nov 18 '21 22:11 CesarCaballeroGaudes

As @handwerkerd explains, the main difference between orthogonalizing and non-aggressive is that with orthogonalization, all the variance common across the accepted and rejected components is put in the accepted basket and therefore it is not removed in the nuisance regression. In contrast, with the non-agressive denoising, the shared variance is smartly split during the estimation process and therefore some amount is removed.

CesarCaballeroGaudes avatar Nov 18 '21 22:11 CesarCaballeroGaudes

Looking at the RTD artifacts, Firefox and Safari ignore changes I made to theme_overrides.css in this PR, but Chrome doesn't. It's weird, but not a big deal. Still, if anyone is willing to take a look at it, I would appreciate it.

In terms of the content, please feel free to make any suggestions you want to the code. I'll try to distinguish between the three approaches soon, but direct suggestions might be better.

tsalo avatar Nov 19 '21 19:11 tsalo

@tsalo Are the overrides suppose to make a border around the topics look like this? image

If so, I see it on Chrome and Firefox. Maybe some browsers are caching the older css---could try clearing the cache before reloading?

notZaki avatar Nov 19 '21 20:11 notZaki

Yeah, it's supposed to go around topics, but I didn't realize that the TOC at the top of that file counted as a topic. I mostly just wanted it around the one I added to the denoising page (see below). If there are other elements that are technically "topics" then that's a whole new can of worms.

image

I guess I can just drop the new CSS and use an admonition with a custom title... 😞

tsalo avatar Nov 19 '21 20:11 tsalo

The following change to the overrides should exclude the TOC:

- div.topic {
+ div.topic:not(.contents) {

I didn't spot the border anywhere else.

notZaki avatar Nov 19 '21 20:11 notZaki

@notZaki thanks for the recommendation. I ended up playing with the general admonition settings instead of topic.

tsalo avatar Dec 02 '21 17:12 tsalo

Just to check in: Is this now ready for review ?

emdupre avatar Apr 28 '22 21:04 emdupre

I'm not sure what's going on with the linting CI step, but, other than that, I think this PR looks good.

I've decided to drop the AFNI and regular Python (without external regressors) tabs, since they seemed to be overcomplicating things.

Here's the latest version: https://tedana--823.org.readthedocs.build/en/823/denoising.html

tsalo avatar Nov 19 '22 17:11 tsalo

I still wish our recommendations for which type of denoising to use were more explicit (e.g., in situation A, use denoising strategy 1), but I think this is pretty good now. I'd love everyone's input.

tsalo avatar Nov 27 '22 16:11 tsalo

I don't think we can make much more specific recommendations. So much of this has to do with study specific goals. That is, for the specific goals of a study, is it worse to remove something that shouldn't be removed or keep something that should be removed.

handwerkerd avatar Nov 28 '22 01:11 handwerkerd

Sorry for the extra commit! I accidentally used a markdown-style link instead of rst. I'll merge once CI passes though, so no need to re-review.

tsalo avatar Nov 28 '22 19:11 tsalo