Skip to content
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 mask_annotate for int dtypes #1445

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

kymillev
Copy link

Description

When using the sv.MaskAnnotator.annotate() function and providing masks as non-bool dtype, the code hangs unexpectedly long for large image shapes and will not produce the correct mask outputs. Below code provides a minimal reproduction

import numpy as np
import supervision as sv

image = np.zeros((2560,2560,3), dtype=np.uint8)
masks = np.zeros((2560,2560), dtype=np.uint8)
masks[:1280] = 1

# This works as intended
# masks = np.array([masks], dtype=bool)

# This does not
masks = np.array([masks], dtype=int)

detections = sv.Detections(
    xyxy=sv.mask_to_xyxy(masks=masks),
    class_id=np.ones(1, dtype=int),
    mask=masks
)

mask_annotator = sv.MaskAnnotator()
out = mask_annotator.annotate(scene=image.copy(), detections=detections)

This PR addresses this by simply converting each mask to bool when constructing the colored overlay mask.

Type of change

Please delete options that are not relevant.

  • [ x] Bug fix (non-breaking change which fixes an issue)

How has this change been tested, please provide a testcase or example of how you tested the change?

All tests pass.

Any specific deployment considerations

This PR might result in unexpected mask annotations when the user input is of dtype float, or if one masks contains multiple unique integer values (e.g. semantic segmentation output). All non-zero values will then become True and masked as the same object. Perhaps an additional check or warning should be given when masks are not in the expected format.

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2024

CLA assistant check
All committers have signed the CLA.

@LinasKo
Copy link
Contributor

LinasKo commented Aug 14, 2024

Hi @kymillev 👋

Thank you for your effort in making our library better!

I'm seeing more and more people trying to construct our objects by hand - it does feel valuable to make it less confusing.

Annotators aren't the right place for this. Instead, could you add the validation to the Detections class? It should have a validate_mask method. Make sure it informs the user why it failed - you may raise a separate error if the dtype isn't correct.

Note: Please share a Google Colab with minimal code to test the new feature. We know it's additional work, but it will speed up the review process. You may use the Starter Template. The reviewer must test each change. Setting up a local environment to do this is time-consuming. Please ensure that Google Colab can be accessed without any issues (make it public). Thank you! 🙏

@kymillev
Copy link
Author

@LinasKo I did try this first, but it resulted in some tests failing because the mask inputs were integers (see below example). So I was not sure if Supervision supports integer masks or if these integer values are used somewhere else in the codebase.

Example of a failing test: tests/utils/test_internal.py

(
  Detections(
      xyxy=np.array([[1, 2, 3, 4], [5, 6, 7, 8]]),
      class_id=np.array([1, 2]),
      confidence=np.array([0.1, 0.2]),
      mask=np.array([[[1]], [[2]]]),
      tracker_id=np.array([1, 2]),
      data={"key_1": [1, 2], "key_2": [3, 4]},
  ),
  False,
  {
      "xyxy",
      "class_id",
      "confidence",
      "mask",
      "tracker_id",
      "data",
  },
  DoesNotRaise(),
  ),

@LinasKo
Copy link
Contributor

LinasKo commented Aug 15, 2024

I see your point. Let's do it more cautiously.

Let's add a deprecation warning (from supervision.utils.internal import warn_deprecated) in the mask validation. Let's check if the datatype is bool, and if not, warn the users with something like this:

warn_deprecated(
    f"A `Detections` object was created with a mask of type {type(mask)}."
    " Masks of type other than `bool` are deprecated. Stricter type checking"
    " will be introduced in `supervision-0.28.0`. Please use "
    " `mask = np.array(..., dtype=bool)` when creating the mask manually."
    " If you did not create the mask manually, please report the issue to the `supervision` team."
)

Then, if there's no similar test with bool values, let's add it. If there is one, let's remove this. Would be great to check if anything else is changing.

Are you comfortable with making this change? I'll help out if needed, albeit a bit later.

@kymillev
Copy link
Author

@LinasKo I added the warning and reverted the change in the mask annotator. I also made the masks in the testing files of bool dtype, as all other tests also used such masks. I think the check: np.issubdtype(mask.dtype, bool) is correct, but I'm not a numpy datatypes expert.

Here is a Colab showing the warning and the problems of using integer masks.

@LinasKo
Copy link
Contributor

LinasKo commented Sep 19, 2024

Just found a bug where polygon_to_mask generates masks of np.float64 type (1s and 0s), rather than bool, which are expected by Detections. I should fix that before merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants