-
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 information about using tedana with fMRIPrep v21.0.0 #847
Conversation
"scipy": ("https://docs.scipy.org/doc/scipy/reference/", None), | ||
"scipy": ("https://docs.scipy.org/doc/scipy/", None), |
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 was getting the following when I built the docs locally:
intersphinx inventory has moved: https://docs.scipy.org/doc/scipy/reference/objects.inv -> https://docs.scipy.org/doc/scipy/objects.inv
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.
It seems like either now work, so I'm fine with this as-is !
.. warning:: | ||
We will try to keep the following gist up-to-date, but there is no guarantee that it will work for a given version. | ||
Use it with caution! | ||
|
||
If you do find that the gist isn't working for an fMRIPrep version >= 20.2.1, | ||
please comment on `Issue #717 <https://github.com/ME-ICA/tedana/issues/717>`_ (even if it's closed) | ||
and we will take a look at the problem. | ||
|
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.
Given that fMRIPrep added an option to output individual echoes, we don't really need to keep the gist up to date.
Codecov Report
@@ Coverage Diff @@
## main #847 +/- ##
=======================================
Coverage 93.27% 93.27%
=======================================
Files 27 27
Lines 2217 2217
=======================================
Hits 2068 2068
Misses 149 149 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.
I'm fine with this being merged.
That said, I have two comments:
- I've reviewed the demo code, but I have not run it. I've never run fMRIprep so I'm really not the right person to confirm it works as intended. It would be nice if an fMRIprep user could try it, but having this in the documentation in it's current form is better than not documenting this at all.
- The length of this section stretches what should be in a FAQ. We really should have a clearer page on pipelines (likely related to [DOC] Add documentation page on denoising approaches #823 ). I again think it's better to merge into the FAQ and move to a more logical place later.
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, thanks @tsalo ! I believe that we don't have any other fmripreppers (is that the formal term?) to check this out but it looks good as a resource for people to start from, so I'm approving.
fMRIPrep versions < 21.0.0 | ||
========================== | ||
|
||
Prior to version 21.0.0, `fMRIPrep`_ outputted the preprocessed, optimally-combined fMRI data, rather than echo-wise data. |
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.
Technically, it could also output preprocessed, individual echos using the --echo-idx
option. It might be worth bringing this up only in that previously, folks might have thought they could use those echos in tedana (which we tried to discourage).
Just a note for myself. @jbdenniso wrote a gist for running tedana on fMRIPrepped data that I'd like to link to in the fMRIPrep-related documentation: https://gist.github.com/jbdenniso/73ec8281229d584721563a41aba410cf |
Co-authored-by: Elizabeth DuPre <emd222@cornell.edu>
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
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 think we can merge as-is ! This will likely only continue to evolve for subsequent versions, so it'd be good to get these updates in -- and we can re-visit as necessary.
Thanks, @tsalo !
So I completely forgot about this PR. Going to merge it now since I have the necessary approvals. |
Closes None.
Changes proposed in this pull request:
--me-output-echos
parameter.