[REF] Suppresses divide by 0 warning #786
Conversation
Codecov Report
@@ Coverage Diff @@
## main #786 +/- ##
==========================================
+ Coverage 93.27% 93.29% +0.01%
==========================================
Files 27 27
Lines 2217 2222 +5
==========================================
+ Hits 2068 2073 +5
Misses 149 149
Continue to review full report at Codecov.
|
tedana/utils.py
Outdated
| dsi = (2. * intersection.sum(axis=axis)) / arr_sum | ||
| if np.any(arr_sum == 0): | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") |
There was a problem hiding this comment.
| warnings.simplefilter("ignore") | |
| warnings.simplefilter("ignore", RuntimeWarning) |
If we're specific enough about the warning we catch, we could just get rid of the entire if statement starting at line 198. Right now we check if all thresholded maps are empty, then here we check if some are empty, and in the new else we run when none are empty. We should be able to just filter the warning and calculate across all maps without checking if any are empty, right?
There was a problem hiding this comment.
Or I guess we could mask things and unmask them? Maybe with a debug message reporting the number of empty maps?
There was a problem hiding this comment.
I agree with your suggestion, I actually meant to add that filter, whoops.
Yes, we can just not check for emptiness, but I'm inclined to think we actually should warn users if all of the maps are empty. The issue is it's difficult to make a meaningful warning in that context.
There was a problem hiding this comment.
What about a warning of how many are empty? Then we don't have to check if all are empty, then any are empty. We check if any are empty, raise a warning with how many, and then outside that if statement we apply the warning filter?
There was a problem hiding this comment.
That would be pretty sensible and straightforward. We also can add a prompt for the users to examine the component table for dice values of 0.
tedana/utils.py
Outdated
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") | ||
| dsi = (2.0 * intersection.sum(axis=axis)) / arr_sum | ||
| else: | ||
| dsi = (2.0 * intersection.sum(axis=axis)) / arr_sum |
There was a problem hiding this comment.
It would be simpler to drop the else completely, I think.
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| dsi = (2.0 * intersection.sum(axis=axis)) / arr_sum | |
| else: | |
| dsi = (2.0 * intersection.sum(axis=axis)) / arr_sum | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("ignore") | |
| dsi = (2.0 * intersection.sum(axis=axis)) / arr_sum |
Can we have the filter target the divide-by-zero warning specifically?
tedana/utils.py
Outdated
| f"Calculating dice coefficient with {total_zeros} " | ||
| "zero-elements in the denominator. " | ||
| "Please check your component table for dice columns with 0-" | ||
| "values" |
There was a problem hiding this comment.
How about something like f"{total_zeros}/{arr_sum.size} components have empty maps, resulting in Dice values of 0. Please check your..."
tedana/utils.py
Outdated
| dsi = np.zeros(arr_sum.shape) | ||
| else: |
There was a problem hiding this comment.
I can't tag the whole if statement above, but it can be removed now that you have the warning and filter below.
|
@tsalo I'm actually running into a mild style issue. If I run |
|
The Lines 1 to 5 in 4d5c645 However, we also have our Lines 9 to 15 in 4d5c645 Could it be a |
|
I forgot, did we pause this merge for some reason? I feel like I recall some hesitation due to not being sure about precisely what caused the underlying issue. |
I honestly do not remember we paused it for any reason. |
Closes #762 . Note that there is some root issue causing the divide by 0 in the first place, but I haven't figured out why it's happening. Since we at least want to suppress the warning for now, this is a small patch to only suppress the warning.
Changes proposed in this pull request: