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

[WIP, ENH] Add GODEC #120

Closed
wants to merge 26 commits into from
Closed

[WIP, ENH] Add GODEC #120

wants to merge 26 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 26, 2018

Closes #79. Ref #106.

@dowdlelt I know that you were able to get the GODEC implementation here working. I used that code as well, with several changes to remove redundancy and unused functions. Perhaps you could take a look at this PR?

Changes proposed in this pull request:

  • Add Go Decomposition to package under tedana.decomposition.godecomp.tedgodec.
  • Integrate GODEC into tedana workflow with ws_denoise argument. Still haven't added a command-line argument.
  • Fix potential bug in MEDN construction within tedana.utils.io.gscontrol_mmix. I've removed this to prevent bloat in the PR. I'm going to open a separate PR ([FIX] Include ignored components in ME-DN T1c time series #125) for this potential bug fix.
  • Replace label with out_dir and add out_dir argument to many functions. Removed to reduce bloat.

To do:

  • Code review
  • Add unit tests
  • Update documentation
  • Update CLI

@dowdlelt
Copy link
Collaborator

I looked for anything obvious, and it seems fine by me. I believe 'vn' is for variance normalization, rather than demeaning.

I am not sure there I ever actually got the python implementation working correctly, though I do recall getting close. When I brought this up on the MEICA repository I was pointed towards the T1c outputs provided by MEICA, by @prantikk here. Limited testing suggested that the T1c mimicked the characteristics of the godec output from the 2018 Power et al paper, all with less fiddling. I believe the biggest problem I ran into was in the output from the algorithm, rather than running it.

In any case, once this is tested it would be nice to compare the T1c data with the GoDec output to confirm (or not) similarities in a single dataset.

MEDN time series was being constructed without the ignored components.

Also the High-Kappa time series in T1c was being constructed without
the mean.
@tsalo
Copy link
Member Author

tsalo commented Aug 28, 2018

Thanks for taking a look @dowdlelt. Also thanks for catching vn's meaning!

Do you know if there's any kind of citation for the minimum image regression? The Power paper (as you know) recommended GODEC or a more standard GSR (which doesn't seem to be implemented in the same way in gscontrol_raw in tedana), but I'm guessing that the minimum image regression came later.

Certainly a quantitative comparison of the recommended method from the literature (GODEC) against the new, simpler method recommended here (T1c) would be nice for anyone who wants to publish using T1c.


On a more general note, I think this is a good opportunity to move gscontrol_mmix out of tedana.utils.io (where it definitely doesn't belong) to someplace that makes more sense. I was considering moving it to tedana.decomposition. Does that sound reasonable? Alternatively, both gscontrol_mmix and tedgodec could go into a new submodule for methods for removing widespread noise (as they were referred to in the Power paper).

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #120 into master will decrease coverage by 1.64%.
The diff coverage is 13.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #120      +/-   ##
=========================================
- Coverage   51.54%   49.9%   -1.65%     
=========================================
  Files          32      33       +1     
  Lines        1969    2058      +89     
=========================================
+ Hits         1015    1027      +12     
- Misses        954    1031      +77
Impacted Files Coverage Δ
tedana/model/fit.py 27.71% <ø> (ø) ⬆️
tedana/io.py 35.55% <0%> (ø) ⬆️
tedana/workflows/tedana.py 14.18% <0%> (-0.2%) ⬇️
tedana/decomposition/__init__.py 100% <100%> (ø) ⬆️
tedana/decomposition/godecomp.py 12.79% <12.79%> (ø)

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 16cfa72...8b03fbb. Read the comment docs.

tedana/utils/io.py Outdated Show resolved Hide resolved
tedana/utils/io.py Outdated Show resolved Hide resolved
tedana/utils/io.py Outdated Show resolved Hide resolved
tedana/workflows/tedana.py Outdated Show resolved Hide resolved
tedana/workflows/tedana.py Outdated Show resolved Hide resolved
@KirstieJane KirstieJane added the enhancement issues describing possible enhancements to the project label Oct 31, 2018
# Conflicts:
#	tedana/decomposition/eigendecomp.py
#	tedana/io.py
#	tedana/model/fit.py
#	tedana/selection/select_comps.py
#	tedana/tests/test_tedana.py
#	tedana/workflows/t2smap.py
#	tedana/workflows/tedana.py
# Conflicts:
#	tedana/decomposition/__init__.py
#	tedana/io.py
#	tedana/workflows/tedana.py
@jbteves
Copy link
Collaborator

jbteves commented May 23, 2019

I remembered this PR while re-reading #79. With all of the merges there are no some conflicts. Would you mind updating this branch to be up-to-date with master?

@tsalo tsalo closed this Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues describing possible enhancements to the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GODEC to package
5 participants