-
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] Improving clarity of docs and figures #419
Conversation
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
=======================================
Coverage 82.17% 82.17%
=======================================
Files 41 41
Lines 2552 2552
=======================================
Hits 2097 2097
Misses 455 455 Continue to review full report at Codecov.
|
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.
This looks great ! A few comments.
The only structural one (and the most nitpicky) is it would be great if we could start new sentences on new lines !!
docs/approach.rst
Outdated
@@ -165,24 +183,18 @@ These components are subjected to component selection, the specifics of which | |||
vary according to algorithm. | |||
|
|||
In the simplest approach, ``tedana`` uses Minka’s MLE to estimate the | |||
dimensionality of the data, which disregards low-variance components. | |||
dimensionality of the data, which disregards low-variance components. (the `mle` option in for `--tedpca`). |
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.
dimensionality of the data, which disregards low-variance components. (the `mle` option in for `--tedpca`). | |
dimensionality of the data, which disregards low-variance components (the `mle` option in for `--tedpca`). |
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.
This reverts commit 28098e1.
2e90d29
to
e28447a
Compare
docs/considerations.rst
Outdated
|
||
A minimum of 3 echoes is required for running the current implementation fo TE-dependent denoising in | ||
``tedana``. | ||
While there are successful studies that don’t follow this rule, |
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.
Can we invert this sentence ? I'm still a little confused on first read-through, personally 😕
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've removed the not follow language - and just say we need three echoes, but left in the note that dual echo is not what we are talking about here.
Co-Authored-By: Elizabeth DuPre <emd222@cornell.edu>
Co-Authored-By: Elizabeth DuPre <emd222@cornell.edu>
Co-Authored-By: Elizabeth DuPre <emd222@cornell.edu>
Co-Authored-By: Elizabeth DuPre <emd222@cornell.edu>
I believe i have addressed all of your concerns - I believe it is ready to go. |
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.
But why is the math broken ?! 😭
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.
Small patches to trigger a new build ?
@all-contributors please add @mvaziri for documentation ! |
I've put up a pull request to add @mvaziri! 🎉 |
Closes #83, .
Changes proposed in this pull request: