-
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
Add --masktype
option to control adaptive mask method
#1057
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
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.
Mostly looks good. I do want several people to compare the masks on real data to make sure nothing unexpected is happening. This change really shouldn't shrink the masks much, so, if it does, that's concerning.
tedana/utils.py
Outdated
last_decreasing_echo[last_decreasing_echo == 0] = n_echos # if no increase, set to n_echos | ||
|
||
# Retain the more conservative of the two adaptive mask estimates | ||
masksum = np.minimum(masksum, last_decreasing_echo) |
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.
Might want to make a second warning message if last_decreasing_echo
removes nontrivially more voxels than the other threshold. I'm viewing that as a sign that something is wrong with the 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.
I think we'll want to log the voxel counts with each different method. I wrote some code for that, but it was a bit ugly and I think it might be easier to do as a separate PR once all current adaptive mask PRs are merged.
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 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? |
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Based on our developer meeting a couple of weeks ago, I've added a EDIT: I've also made the old behavior ( |
--masktype
option to control adaptive mask method
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 have a bunch of comments, but, overall, this is good. The only major thing is that I don't think it will be possible to select decay
and dropout
through the CLI.
Also, after starting this, I realized it would be slightly nicer to merge #1060 first since you'll pretty much have to re-write #1060 to fit with these changes. Changed my mind on this. It would be easier to merge this with the updated doc strings and parameters and then align the other masking PRs to fit within these updates.
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
I think I've addressed everything, and I updated the t2smap integration test to use both |
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 noticed a few more things, but this is getting close.
tedana/utils.py
Outdated
last_decreasing_echo[last_decreasing_echo == 0] = n_echos # if no increase, set to n_echos | ||
|
||
# Retain the more conservative of the two adaptive mask estimates | ||
masksum = np.minimum(masksum, last_decreasing_echo) |
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'll want to log the voxel counts with each different method. I wrote some code for that, but it was a bit ugly and I think it might be easier to do as a separate PR once all current adaptive mask PRs are merged.
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
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.
LGTM! Thank you @tsalo!
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:
--masktype
to both tedana and t2smap. This parameter accepts one or more values, like--gscontrol
. The valid values are "dropout" (the current method), "decay" (flag any echoes that do not decrease in mean value), and "none" (set adaptive mask value to all echoes, for testing and the like).getsum
parameter frommake_adaptive_mask
. We don't usegetsum=False
outside of tests, so we really don't need it.