-
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
Identify the last good echo in adaptive mask instead of sum of good echoes #1061
Merged
Merged
Changes from all commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
89f4bb6
Limit adaptive mask calculation to brain mask.
tsalo 28bc80f
Use `compute_epi_mask` in t2smap workflow.
tsalo c5d7d91
Try fixing the tests.
tsalo 3fa7593
Fix make_adaptive_mask.
tsalo 4cc86c6
Update test_utils.py
tsalo 5b379e1
Update test_utils.py
tsalo ccff6dc
Improve docstring.
tsalo a2dc300
Identify the last good echo instead of sum.
tsalo beeae89
Fix.
tsalo 7097a75
Update utils.py
tsalo ea5c364
Update utils.py
tsalo 32d1cb7
Try fixing.
tsalo bb0dbdc
Update utils.py
tsalo d096d08
Update utils.py
tsalo 1f72638
add checks
tsalo 18b66ac
Just loop over voxels.
tsalo aea9fe2
Update utils.py
tsalo 28267f7
Update utils.py
tsalo 259b002
Update test_utils.py
tsalo 55a2694
Revert "Update test_utils.py"
tsalo d34c65a
Update test_utils.py
tsalo b3bfbbd
Update test_utils.py
tsalo 1014000
Remove checks.
tsalo b80524b
Don't take absolute value of echo means.
tsalo 2bfa240
Log echo-wise thresholds in adaptive mask.
tsalo 7888c4e
Add comment about non-zero voxels.
tsalo 20de578
Update utils.py
tsalo def2770
Update utils.py
tsalo 15b80f7
Merge remote-tracking branch 'upstream/main' into fix-old-adaptive-mask
tsalo e44878b
Update test_utils.py
tsalo f762573
Update test_utils.py
tsalo d91c016
Update test_utils.py
tsalo 097d3a7
Log the thresholds again.
tsalo 3a3f115
Merge branch 'fix-old-adaptive-mask' into fix-old-adaptive-mask-2
tsalo a99cca2
Update test_utils.py
tsalo b0192c4
Update test_utils.py
tsalo 508d9c2
Update test_utils.py
tsalo 189cb8e
Merge remote-tracking branch 'upstream/main' into fix-old-adaptive-ma…
tsalo 30f5e41
Merge remote-tracking branch 'upstream/main' into fix-old-adaptive-ma…
tsalo e76c1d1
Merge remote-tracking branch 'upstream/main' into fix-old-adaptive-ma…
tsalo fcb6104
Add simulated data to adaptive mask test.
tsalo 2ccc8aa
Clean up the tests.
tsalo 60482db
Merge remote-tracking branch 'upstream/main' into fix-old-adaptive-ma…
tsalo 2d34d98
Add value that tests the base mask.
tsalo 682f8a6
Remove print in test.
tsalo ef85ca1
Update tedana/utils.py
tsalo 2d3712a
Update tedana/utils.py
tsalo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 was trying to get a better handle with what was happening within this PR. In
make_adaptive_mask
I addedolder_mask =(np.abs(echo_means) > lthrs).sum(axis=-1)
right afterdropout_adaptive_mask
was calculated and compared results in my debugger. Calls like((older_mask==2) * (dropout_adaptive_mask==2)).sum()
show voxels that used to be 1 in the adaptive mask and are now 2.Voxels that were 0 are still 0. (This interacts with other masking steps, but seems matched at this point in the code)
Of the 3590 voxels that were 1, 1061 are now 2, and 913 are now 3.
Of the voxels 5276 voxels that were 2, 1900 are now 3.
As expected, none of the voxels that had a higher values are now lower.
That means this change will substantively expand the number of voxels used in ICA and will balance out some of the drop caused by raising the threshold from masking. The one thing that concerns me is it turns how this test data had 913 voxels where the first and second echos were both below threshold, but the third was above threshold. That doesn't seem great, but the proposed enhancement would give this voxels a 0 in the adaptive mask, which would mean they'd be even be excluded from the optimally combined data. There's nothing to change here, but this is a discussion that will be relevant if a future enhancement is considered.
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've opened #1083 about this. Can you follow up in that issue with your ratio idea?