-
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
[FIX] Remove hardcoded numbers of echoes. #95
Conversation
Codecov Report
@@ Coverage Diff @@
## master #95 +/- ##
=======================================
Coverage 42.74% 42.74%
=======================================
Files 27 27
Lines 1516 1516
=======================================
Hits 648 648
Misses 868 868
Continue to review full report at Codecov.
|
I don't think the removal of '/ 3' in utils.py is a number of echoes issue. Line 200 in bfaec06
Looking at the code above and below, this function is a simple, but arbitrary way to create a voxelwise mask. It makes the assumption that at least 1/3 of the volume isn't in the brain and it finds the voxel with the 33rd percentile magnitude in the first echo. It then uses the same voxel for all 3 echoes since it's assumed to be an out-of-brain voxel. It takes a threshold from each echo and uses that to define the echo-specific masking threshold. The '/ 3' seems to be used to set the threshold to 1/3 of the selected voxel in each echo so that you only exclude voxels that are really noise. Both the 33rd percentile and the divide by 3 steps seem like arbitrary thresholds, but the division by 3 doesn't look like it has anything to do with the number of echoes so it shouldn't be changed. Also, this is passing CI, but does any of the sample data this is being tested on include more than 3 echoes? If not, then this change wouldn't alter the output. |
That makes sense. I'll change that function back. |
I also changed `med_val` to `perc_val` to make it clearer.
WDYT of flagging that value as arbitrary in the code comments, @tsalo ? Maybe with a |
@emdupre That's a great idea! Adding notes now. |
LGTM ! I'm ok to merge this in and then re-visit when we add additional tests from other 4+ echo datasets. WDYT @handwerkerd, @tsalo ? |
Closes #77.
Changes proposed in this pull request: