-
Notifications
You must be signed in to change notification settings - Fork 139
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 ignore_index argument to Mask.as_class_mask() and Mask.as_instance_mask() #1409
Add ignore_index argument to Mask.as_class_mask() and Mask.as_instance_mask() #1409
Conversation
Signed-off-by: Kim, Vinnam <[email protected]>
Signed-off-by: Kim, Vinnam <[email protected]>
Signed-off-by: Kim, Vinnam <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1409 +/- ##
========================================
Coverage 80.85% 80.86%
========================================
Files 271 271
Lines 30689 30713 +24
Branches 6197 6203 +6
========================================
+ Hits 24815 24836 +21
- Misses 4489 4490 +1
- Partials 1385 1387 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
# NOTE: This dispatching rule is required for a performance boost | ||
if ignore_index == flipped_zero_np_scalar: | ||
flipped_index = ~np.full(tuple(), fill_value=index, dtype=dtype) | ||
return ~(binary_mask * flipped_index) |
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.
Sorry, I was failed to understand this logic correctly. When ignore_index
is 255, this returns binary_mask * np.full(tuple(), fill_value=index, dtype=dtype)
. Is this right?
There is two negative ~
operations. Should we have this?
We have to explanation what ignore_index==255
stands for.
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.
Please see this example
datumaro/src/datumaro/util/mask_tools.py
Lines 152 to 157 in 53b3a5a
Examples: | |
>>> binary_mask = np.eye(2, dtype=np.bool_) | |
>>> index_mask = make_index_mask(binary_mask, index=10, ignore_index=255, dtype=np.uint8) | |
>>> print(index_mask) | |
array([[ 10, 255], | |
[255, 10]], dtype=uint8) |
We want to convert a binary mask to an index mask by filling 1
s with index
and 0
s with ignore_index
. This function would be a simple function like
return np.where(binary_mask, mask, ignore_index)
, but this function has if-else branching in the loop. Thus, it is hard to be accelerated by hardware.
~
is a bit-wise flipping operator. Therefore, this logic is doing (~255 == 0
)
[~10, 0]
[0, ~10]
=>
[10, 255]
[255, 10]
Anyway, 53b3a5a improved it further to cover the general cases.
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.
def kernel4(binary_mask, index, ignore_index=0, dtype=None):
if dtype is None:
dtype = np.min_scalar_type(index)
if dtype != np.min_scalar_type(ignore_index):
raise ValueError()
mask = binary_mask * np.array([index], dtype=dtype)
if ignore_index == 0:
return mask
return np.where(binary_mask, mask, ignore_index)
from timeit import timeit
binary_mask = np.random.randint(0, 2, size=[1024, 1024], dtype=np.bool_)
print("Kernel4 time: ", timeit(lambda: kernel4(binary_mask, index=10, ignore_index=230), number=200))
print("make_index_mask time: ", timeit(lambda: make_index_mask(binary_mask, index=10, ignore_index=230), number=200))
There was a huge performance gap with/without this optimization in my desktop
Kernel4 time: 0.8781529648695141
make_index_mask time: 0.03690833714790642
Signed-off-by: Kim, Vinnam <[email protected]>
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.
Could you update changelog to add this in 2.0 (will 1.6.0)?
Can we do the update after merging #1419 (review) and branching out |
Summary
How to test
Added an unit test for this change.
Checklist
License
Feel free to contact the maintainers if that's a concern.