NF: Adding correct_mask to median_otsu #3140
Conversation
|
Hello @pjsjongsung, Thank you for updating !
Comment last updated at 2024-04-16 17:46:25 UTC |
|
Could you please add a test of |
pjsjongsung
force-pushed
the
seg_corr_change
branch
2 times, most recently
from
19b14ce
to
e62f7c3
Compare
There was a problem hiding this comment.
Hi @pjsjongsung,
Thank you for this. See below for minor comments
| @@ -297,7 +298,7 @@ def __predict(self, x_test): | |||
| def predict(self, T1, affine, | |||
| voxsize=(1, 1, 1), batch_size=None, | |||
| return_affine=False, return_prob=False, | |||
| largest_area=True): | |||
There was a problem hiding this comment.
You might have to use deprecate_params since this method is available the 2 last release at least.
There was a problem hiding this comment.
True. Do you know an example of deprecation by change in parameter name in DIPY I could follow?
There was a problem hiding this comment.
here it is :
Line 49 in 90489e8
dipy/segment/mask.py
Outdated
| correct_mask : bool, optional | ||
| Whether to remove potential holes or islands. | ||
| Useful for solving minor errors. | ||
| Default is True |
|
Can you also rebase or merge master? Thank you |
pjsjongsung
force-pushed
the
seg_corr_change
branch
3 times, most recently
from
b485557
to
a306942
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3140 +/- ##
==========================================
- Coverage 83.65% 83.62% -0.04%
==========================================
Files 153 153
Lines 21272 21281 +9
Branches 3434 3425 -9
==========================================
Hits 17796 17796
- Misses 2618 2625 +7
- Partials 858 860 +2
|
There was a problem hiding this comment.
Hi @pjsjongsung,
Sorry for the late review. See below some comments. It is almost ready to go
dipy/segment/mask.py
Outdated
| correct_mask : bool, optional | ||
| Whether to remove potential holes or islands. | ||
| Useful for solving minor errors. | ||
| Default is True |
dipy/segment/utils.py
Outdated
| except ValueError: | ||
| warnings.warn('The mask has no background. \ | ||
| Returning the original mask', | ||
| DeprecationWarning, stacklevel=2) |
There was a problem hiding this comment.
This is not a DeprecationWarning. I will keep it a UserWarning
dipy/segment/mask.py
Outdated
| @@ -124,7 +125,7 @@ def crop(vol, mins, maxs): | |||
|
|
|||
|
|
|||
| def median_otsu(input_volume, vol_idx=None, median_radius=4, numpass=4, | |||
| autocrop=False, dilate=None): | |||
| autocrop=False, dilate=None, correct_mask=True): | |||
There was a problem hiding this comment.
I will call this finalize_mask or something similar
correct_mask sounds like there is a bug so why should we put it False? lol
dipy/nn/evac.py
Outdated
| def predict(self, T1, affine, | ||
| voxsize=(1, 1, 1), batch_size=None, | ||
| return_affine=False, return_prob=False, | ||
| largest_area=True): | ||
| correct_mask=True): |
pjsjongsung
force-pushed
the
seg_corr_change
branch
from
a306942
to
2975244
Compare
|
Sorry about the late response. Hopefully it is all resolved! |
|
Hi @pjsjongsung, I did not look yet but your test is failing: =========================== short test summary info ============================
FAILED segment/tests/test_utils.py::test_remove_holes_and_islands - AssertionError:
Arrays are not equal
Mismatched elements: 34970 / 64000 (54.6%)
Max absolute difference: 1.0
Max relative difference: 1.0
x: array([[[1, 1, 1, ..., 0, 1, 1],
[1, 1, 1, ..., 0, 1, 1],
[1, 1, 1, ..., 1, 0, 1],...
y: array([[[ 0., 0., 0., ..., 0., 0., 0.],
[ 0., 0., 0., ..., 0., 0., 0.],
[ 0., 0., 0., ..., 0., 0., 0.],...
===== 1 failed, 1020 passed, 65 skipped, 8 warnings in 1785.02s (0:29:45) ======Can you look closely at this PR? thanks ! |
This PR aims to add a function that removes holes or islands of a binary mask to median_otsu. The function was already being used in dipy/nn/utils.py, so it has been moved to dipy/segment/utils.py, as it makes more sense for mask correction to be in segmentation module. Slice-wise hole removal was excluded from the function, since it can cause huge false positives if there is a lack of axial slices. Finally, the function's name has been changed to remove_holes_and_islands, to be more explicit about the functionality. Related parameter name and its descriptions have been changed as well.
pjsjongsung
force-pushed
the
seg_corr_change
branch
from
2975244
to
2e3c310
Compare
|
My bad. I didn't think the test would fail since it worked before when it was in a different module. Changed the test so that it would not create non realistic tasks where the method would fail. |
This PR aims to add a function that removes holes or islands of a binary mask to median_otsu.
The function was already being used in
dipy/nn/utils.py, so it has been moved to dipy/segment/utils.py, as it makes more sense for mask correction to be in segmentation module.
Slice-wise hole removal was excluded from the function, since it can cause huge false positives if there is a lack of axial slices.
Finally, the function's name has been changed to
remove_holes_and_islands, to be more explicit about the functionality.
This PR is related to #3124