ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
158 stars 94 forks source link

Add `--masktype` option to control adaptive mask method #1057

Closed tsalo closed 2 months ago

tsalo commented 3 months ago

Closes #312. I'd like to look into R-squared again at some point, but I think this is a solid improvement to the adaptive mask.

Changes proposed in this pull request:

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.75%. Comparing base (f084dc4) to head (a0d5f79).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1057 +/- ## ========================================== + Coverage 89.68% 89.75% +0.06% ========================================== Files 26 26 Lines 3482 3505 +23 Branches 615 620 +5 ========================================== + Hits 3123 3146 +23 Misses 211 211 Partials 148 148 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

handwerkerd commented 3 months ago

Thinking this over more: The code for these changes in simple, but this can non-trivially affect results in ways that are worth testing. One option to consider is that you tweak this PR to make it possible for an end-user to try the current adaptive mask, and several other options using t2smap (This could be hacky change to a couple of functions and the t2smap CLI/API, I could see either removing all of that or just the CLI/API option before merging.). It could also be tweaked to log the mask sizes & size differences with each option. Then it would be easy for several of us to run t2smap on a lot of data and compare the adaptive masks.

Depending on timing we could attempt to either have results to discuss by next Thursday's dev call or prod people to try this at the dev call. Thoughts?

tsalo commented 3 months ago

Based on our developer meeting a couple of weeks ago, I've added a --masktype parameter to control how the adaptive mask is generated. @handwerkerd could you take another look at some point?

EDIT: I've also made the old behavior (--masktype dropout) the default, so this should no longer change the results.

tsalo commented 2 months ago

I think I've addressed everything, and I updated the t2smap integration test to use both dropout and decay to ensure that the parser could handle multiple values.