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: ME | Add missing output to documentation, improve boilerplate #2608

Merged
merged 4 commits into from
Oct 20, 2021

Conversation

oesteban
Copy link
Member

Document one missing output of the T2star workflow, and edit boilerplate.

@oesteban oesteban requested review from tsalo and emdupre October 20, 2021 07:56
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #2608 (8fbb026) into master (3f985d6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2608   +/-   ##
=======================================
  Coverage   45.80%   45.80%           
=======================================
  Files          41       41           
  Lines        3133     3133           
=======================================
  Hits         1435     1435           
  Misses       1698     1698           
Impacted Files Coverage Δ
fmriprep/workflows/bold/t2s.py 43.75% <ø> (ø)
fmriprep/workflows/bold/base.py 17.62% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f985d6...8fbb026. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Oct 20, 2021

Hello @oesteban! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-20 14:43:23 UTC

Combine multiple echos of :abbr:`ME-EPI (multi-echo echo-planar imaging)`.

This workflow wraps the `tedana`_ `T2* workflow`_ to optimally
combine multiple echos and derive a T2* map.
combine multiple echos and derive a T2\ :sup:`★` map.
The following steps are performed:

#. :abbr:`HMC (head motion correction)` on individual echo files.
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we are at it. I've been searching through tedana's docs and didn't see that the workflow runs any sort of HMC internally, is that correct?

If it does, I would need to confirm we are not running HMC twice in fMRIPrep. In that case, would it be possible to disable HMC in tedana?

If it doesn't, I think I will revise this docstring to say that HMC'd echos are assumed (i.e., the workflow does not run HMC again).

Copy link
Collaborator

Choose a reason for hiding this comment

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

tedana doesn't perform any HMC, so I agree that the docstring needs to be revised.

oesteban added a commit that referenced this pull request Oct 20, 2021
Although I brought up the issue within #2530 and got some mildly opposed initial
opinion from @effigies
(#2530 (comment)),
I've come to think that we should provide the T2star workflow with
HMC'ed and SDC'ed echos (confirmation awaiting:
#2608 (review)).

This comes as an issue related to #2606.

This PR proposes that individual echoes are HMC'ed (and SDC'ed when
fieldmaps are available) before calculating the optimal combination
and the T2star map.

I believe this may be beneficial when we combine SDC with modulation by
the Jacobian of the distortion (nipreps/sdcflows#238) because that might
help restore some (little, admittedly) of the dropout of the later
echoes in distorted regions. I would expect more voxels will be deemed
as acceptable for the exponential fitting for this reason.

It also conceptually simplifies a little, as both SE and ME paths look
more alike, and only the little hacks to generate ME iterables and the
``boldbuffer`` are now necessary. Otherwise, SDC would be inserted at a
much later stage for ME only.
Copy link
Collaborator

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Hopefully my suggestions will address the HCM documentation issue.

Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
@oesteban oesteban requested a review from tsalo October 20, 2021 14:44
@oesteban oesteban merged commit 32f7b06 into master Oct 20, 2021
@oesteban oesteban deleted the doc/multi-echo-improvements branch October 20, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants