-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DOC] Add documentation page on denoising approaches #823
Conversation
Codecov ReportBase: 93.30% // Head: 93.30% // No change to project coverage 👍
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. ☔ View full report at Codecov. |
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:
|
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.
True, but the parameter estimates come from all components in the mixing matrix: Line 372 in 0f04f76
Wouldn't that make it non-aggressive denoising?
Great point. I can do that with the Python portion at least.
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. |
Good point on the
If you want three tabs, would there be any difference between fmriprep and python? |
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?
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. |
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 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'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. |
Maybe @CesarCaballeroGaudes and @smoia want to comment on this. |
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. |
This isn't quite right but hopefully it helps explain. |
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I few minor suggestions on typos or style, but otherwise LGTM.
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Already added in ME-ICA#840.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Left a minor comment.
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. |
Closes None, but stems from #819.
Changes proposed in this pull request:
Add sphinx-inline-tabs to documentation requirements.Add missing carpet plot (closes Carpet plot image in documentation is missing #803).To do:
[ ] Add AFNI code to do the denoising.