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

[REF, DOC] Document PAID combination method #264

Merged
merged 6 commits into from
Apr 23, 2019
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Apr 20, 2019

Closes #210, #214. This was originally part of #247, but has been split off because it is self-contained and should be merged separately.

Changes proposed in this pull request:

  • Rename combmode value “ste” to “paid”
  • Rename variable ste to source_tes for clarity
  • Drop PAID support from tedana (because PAID requires smoothed data and multi-echo denoising does not work with smoothed data) but keep it in t2smap workflow
  • Add note box about PAID to documentation

- Rename combmode value “ste” to “paid”
- Rename variable “ste” to “source_tes” for clarity
- Drop PAID support from tedana (because PAID requires smoothed data
and multi-echo denoising does not work with smoothed data) but keep it
in t2smap workflow.
@tsalo tsalo changed the title [REF] Rename "ste" to "paid" [REF] Document PAID combination method Apr 20, 2019
@tsalo tsalo changed the title [REF] Document PAID combination method [REF, DOC] Document PAID combination method Apr 20, 2019
@ME-ICA ME-ICA deleted a comment from codecov bot Apr 22, 2019
@tsalo tsalo requested a review from emdupre April 22, 2019 16:16
Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

This looks great to me ! There were a few places in the code previously where I found the interchange between source TEs (ste) and ste (combination) confusing, so this clears things up quite a bit.

I had a few small questions, all in the documentation.

docs/approach.rst Outdated Show resolved Hide resolved
docs/approach.rst Outdated Show resolved Hide resolved
The tedana package also supports another method for optimal combination that
does not use :math:`T_{2}^*`, known as the parallel-acquired inhomogeneity
desensitized (PAID) ME-fMRI combination method (`Poser et al., 2006`_).
This method is not supported in the tedana workflow, however, because
Copy link
Member

Choose a reason for hiding this comment

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

Since the t2smap workflow is called as part of the tedana workflow, this is a little confusing, to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could hypothetically allow users to run tedana without denoising (e.g., --disable-denoising?), which would be the same as the t2smap workflow, but that would mean managing how the combmode and denoising arguments interact (i.e., using paid and denoising should raise a warning and switch paid to t2s automatically).

Copy link
Member

Choose a reason for hiding this comment

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

Right, sorry, I wasn't thinking quite like that ! I just meant that it is technically in the tedana workflow, but it's not accessible, you're right.

Maybe we could just say something like

This method specifically assumes that noise in the acquired echoes is "isotopic and 
homogeneous throughout the image," meaning it should be used on smoothed data. 
As we do not recommend performing tedana denoising  on smoothed data, 
we discourage using PAID within the tedana workflow.
We do, however, make it accessible as an alternative combination method 
in the t2smap workflow.

docs/approach.rst Show resolved Hide resolved
Co-Authored-By: tsalo <tsalo006@fiu.edu>
@codecov
Copy link

codecov bot commented Apr 23, 2019

Codecov Report

Merging #264 into master will decrease coverage by 0.02%.
The diff coverage is 68.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
- Coverage   46.11%   46.08%   -0.03%     
==========================================
  Files          33       33              
  Lines        2045     2044       -1     
==========================================
- Hits          943      942       -1     
  Misses       1102     1102
Impacted Files Coverage Δ
tedana/model/fit.py 30.43% <ø> (ø) ⬆️
tedana/workflows/tedana.py 14.64% <0%> (ø) ⬆️
tedana/decomposition/eigendecomp.py 10.46% <0%> (ø) ⬆️
tedana/tests/test_t2smap.py 100% <100%> (ø) ⬆️
tedana/workflows/t2smap.py 67.1% <100%> (ø) ⬆️
tedana/tests/test_combine.py 100% <100%> (ø) ⬆️
tedana/combine.py 88% <87.5%> (-0.24%) ⬇️

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 c64ad6f...9eb250d. Read the comment docs.

emdupre and others added 2 commits April 22, 2019 20:54
Co-Authored-By: tsalo <tsalo006@fiu.edu>
@ME-ICA ME-ICA deleted a comment from codecov bot Apr 23, 2019
@ME-ICA ME-ICA deleted a comment from codecov bot Apr 23, 2019
emdupre
emdupre previously approved these changes Apr 23, 2019
Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

One small suggestion for clarity but this looks 💯

@tsalo
Copy link
Member Author

tsalo commented Apr 23, 2019

Not sure how I "dismissed" your review, but I just added a commit with the requested change. I also think using [skip ci] might have made a mess of things somehow?

@KirstieJane
Copy link
Member

@tsalo @emdupre - is the protected branch setup to dismiss reviews when there are new commits? That might explain @tsalo question above (#264 (comment))!

@tsalo tsalo merged commit cb9d2e0 into ME-ICA:master Apr 23, 2019
@emdupre
Copy link
Member

emdupre commented Apr 23, 2019

Yes, sorry, that's how it's set up ! Happy to think about whether that makes sense, but I went ahead and re-approved, for now 😸

@tsalo tsalo deleted the update-paid branch May 23, 2019 17:12
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.

Improve documentation of PAID combination method
3 participants