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.

Sign up for GitHub

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

Jump to bottom

DOC: Refinement of confounds documentation after #1877 #1906

Merged
merged 7 commits into from
Dec 11, 2019
Merged

DOC: Refinement of confounds documentation after #1877 #1906

merged 7 commits into from
Dec 11, 2019

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Dec 6, 2019 •
edited
Loading

This PR addresses further issues after #1877:

@oesteban
DOC: Refinement of confounds documentation after #1877
a20e75c 
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 oesteban added this to the 1.6.0 milestone Dec 6, 2019
@oesteban oesteban requested review from jdkent and kfinc December 6, 2019 19:11
@auto-comment

This comment has been minimized.

Sign in to view
@oesteban
Copy link
Member Author

oesteban commented Dec 6, 2019

Quick link: https://10794-53175327-gh.circle-artifacts.com//0/tmp/src/fmriprep/docs/_build/html/outputs.html

@oesteban oesteban mentioned this pull request Dec 7, 2019
Merged
3 tasks
mgxd
mgxd approved these changes Dec 10, 2019
View reviewed changes
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

LGTM

effigies
effigies reviewed Dec 10, 2019
View reviewed changes
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
rciric
rciric reviewed Dec 10, 2019
View reviewed changes
Copy link
Contributor

@rciric rciric left a comment

Choose a reason for hiding this comment

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

Looks great! A few minor comments.

docs/outputs.rst Show resolved Hide resolved
docs/outputs.rst Show resolved Hide resolved
@oesteban
Copy link
Member Author

I'd really love to have a green tick from @kfinc and/or @jdkent before merging this...

jdkent
jdkent reviewed Dec 10, 2019
View reviewed changes
Copy link
Collaborator

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

One comment on what to include about recalculation of counfounds when using ICA-AROMA

docs/outputs.rst Show resolved Hide resolved
effigies
effigies reviewed Dec 11, 2019
View reviewed changes
docs/outputs.rst Outdated Show resolved Hide resolved
jdkent
jdkent reviewed Dec 11, 2019
View reviewed changes
Copy link
Collaborator

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

Added one more crucial citation for the debate.

docs/outputs.rst Show resolved Hide resolved
docs/outputs.rst Outdated Show resolved Hide resolved
oesteban
oesteban commented Dec 11, 2019
View reviewed changes
docs/outputs.rst Outdated Show resolved Hide resolved
@oesteban @effigies @jdkent
Apply suggestions from code review
5fe23ae 
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: James Kent <[email protected]>
@jdkent jdkent self-requested a review December 11, 2019 23:22
@oesteban oesteban merged commit ca65f35 into master Dec 11, 2019
@oesteban oesteban deleted the docs/issue-1049 branch December 11, 2019 23:28
@kfinc
Copy link
Contributor

kfinc commented Dec 13, 2019

Hi! Sorry for the delay, I was out of the office for 1 week. Confounds page looks great! Many thanks for incorporating all the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies approved these changes

@rciric rciric rciric left review comments

@jdkent jdkent jdkent approved these changes

@mgxd mgxd mgxd approved these changes

@kfinc kfinc Awaiting requested review from kfinc

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
20.0.0
Development

Successfully merging this pull request may close these issues.

Add description of CosineX regressors to the documentation
6 participants
@oesteban @kfinc @effigies @jdkent @rciric @mgxd