Skip to content
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: Improving accessibility of confounds description #1877

Merged
merged 24 commits into from
Dec 6, 2019

Conversation

kfinc
Copy link
Contributor

@kfinc kfinc commented Nov 22, 2019

Changes proposed in this pull request

This PR addresses issues #489 #1726 #1049 related to improving the accessibility and educational value of fMRIPrep documentation, in particular, description of Confounds.

The work was done during fMRIPrep docusprint and is a result of collaborative effort of multiple people, including Pierre Bellec, Elizabeth DuPre, Karolina Finc, James Kent, Chris Markiewicz, Saren Seeley, Ursula Tooley, Hao-Ting Wang.

You can track our discussion here:
https://docs.google.com/document/d/14WyhCvEn0G5fbLY-Rk7BQa9ISzYEB2ll3YCjyd-OjB8/edit

Documentation that should be reviewed

We made major edits to the Confounds subsection in outputs.rst:

  • we added a general explanation of sources of noise in fMRI signal and introduction to confound regression - that may help users to grasp the idea and advantage of multiple regressors returned by fMRIPrep;
  • we warned a user of possible misuse of confounds_regressors.tsv table, such as (1) regressing out all confounds during denoising procedure, (2) using ICA-AROMA confounds on AROMA-denoised data, and (3) not selecting a subset of CompCor confounds;
  • we expanded the list of references by some fresh and informative papers;
  • we proposed a structure for describing confounds in sections: (1) Basic confounds, (2) Parameter expansion of basic confounds, (3) Confounds for outlier detection, (4) ICA-AROMA confounds, (5) CompCor confounds [I will be grateful for the discussion on the proposed structure as it is an alternative to the structure discussed during docusprint];
  • we added more description to almost all confound regressors.

There is still much to improve and clarify, but I think that this is a good start.

@welcome
Copy link

welcome bot commented Nov 22, 2019

Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄
We invite you to list yourself as an fMRIPrep contributor. To learn more about what that entails and how we credit our contributors, please check out the contributing guidelines. If your name is not already on the list, please insert it, in alphabetical order, into the contributors.json file. Example:

   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
}, ```

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

@auto-comment

This comment has been minimized.

@kfinc kfinc changed the title Docs/confounds desc acessibility Improving accessibility of confounds description Nov 22, 2019
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great at a first pass. Documentation build is failing because of the FSL link, added a suggestion to fix that.

A couple of iterated issues that I only marked once:

  • New sentences should start with new lines
  • fMRIPrep should be capitalized that way, and rendered in italics: fMRIPrep.

docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
@oesteban oesteban requested review from rciric and wiheto November 26, 2019 00:16
@oesteban
Copy link
Member

Eyes from @wiheto and @rciric very much appreciated :)

@kfinc kfinc changed the title Improving accessibility of confounds description DOC: Improving accessibility of confounds description Nov 29, 2019
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
@oesteban
Copy link
Member

oesteban commented Dec 5, 2019

Okay, rendered here https://10602-53175327-gh.circle-artifacts.com/0/tmp/src/fmriprep/docs/_build/html/outputs.html

Last call for @rciric and @poldracklab/fmriprep-docusprint-leaders to have a look.

docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated
and ``global_signal`` to enable applying 36-parameter denoising strategy
proposed by Satterthwaite et al. [Satterthwaite2013]_.

Derivatives and quadratic terms are stored under column names with suffixes: ``_derivative1`` and powers ``_power2``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably note that these are not BIDS-Derivatives-Draft conformant and subject to change in future versions.

docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
Co-Authored-By: Chris Markiewicz <markiewicz@stanford.edu>
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback - will address your mutually exclusive comments later.


.. warning::
If you are already using ICA-AROMA cleaned data (``~desc-smoothAROMAnonaggr_bold.nii.gz``),
do not include ICA-AROMA confounds during your design specification or denoising procedure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

"SingularValue": 25.8270209974,
"VarianceExplained": 0.1081970825
},
"dropped_0": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paging @rciric

Copy link
Contributor

@adelavega adelavega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks great. My comments are super nitpicky so take them with a grain of salt!

docs/outputs.rst Show resolved Hide resolved
docs/outputs.rst Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
@oesteban oesteban merged commit 493af6d into master Dec 6, 2019
@oesteban oesteban deleted the docs/confounds_desc_acessibility branch December 6, 2019 17:37
oesteban added a commit that referenced this pull request Dec 6, 2019
This PR addresses further issues:

- Address warnings 2) and 3) of #1049 - Close #1049, and I've opened #1905 to follow up conversations
- Add documentation about DCT regressors, close #1167.
- Address items 2) and 3) of #1521.
oesteban added a commit that referenced this pull request Dec 11, 2019
DOC: Refinement of confounds documentation after #1877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants