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

feat: Merge Multiple Polygons in a COCO Dataset Annotation #1229

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

Conversation

DancinParrot
Copy link

Description

This PR attempts to address issue #1209. Within an appropriately structured COCO dataset, an annotation may contain a segmentation list with multiple nested lists of vertices. This results in supervision terminating prematurely and raising a ValueError.

The solution as discussed in the issue includes the creation of binary masks for all polygons within the segmentation list and the usage of numpy's logical_or() function to merge all masks which is used in the coco_annotations_to_masks function.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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

The change has been tested using a custom COCO dataset which produces the ValueError mentioned in the issue linked above. You may test out the changes by following the guide on the provided Jupyter Notebook using the following resources:

Any specific deployment considerations

Documentation may require an update.

Docs

  • Docs updated? What were the changes: Created an entry under docs/datasets/utils.md with description and types of the function and its parameters. An example and type have also been added for its output.

If a segmentation list of an annotation in a COCO dataset contains more than 1 nested list, seperate binary masks are created for each nested list (polygon). Then, the first polygon is designated as a parent and subsequent polygons in the list will be merged with the parent using numpy logical or.
The function merge_masks() has been renamed to merge_polygons(). It has been refactored to include explicit function parameters and return value.
@CLAassistant
Copy link

CLAassistant commented May 25, 2024

CLA assistant check
All committers have signed the CLA.

@@ -46,6 +47,59 @@ def approximate_mask_with_polygons(
]


def merge_polygons(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the merge_polygons name is a bit misleading and not conststant with outhe util names in supervision. I think we should call it polygons_to_mask.

And of course we already have a function called polygon_to_mask that does the same but only take one mask.

It seems a bit silly that we would have 2 functions doeng almost the same. I thin kwe should:

  • rename merge_polygons to polygons_to_mask.
  • make sure that polygons_to_mask is not using polygon_to_mask internally.
  • modify places where polygon_to_mask was used and use polygons_to_mask instead.
  • mask polygon_to_mask as deprecated and recommend using polygons_to_mask instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DancinParrot, here is an example of how we mark functions as deprecated. We use deprecated decorator to do it.

@deprecated(
    "`Detections.from_roboflow` is deprecated and will be removed in "
    "`supervision-0.22.0`. Use `Detections.from_inference` instead."
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use supervision-0.27.0 as a deprecation version.

@SkalskiP
Copy link
Collaborator

Hi @DancinParrot 👋🏻 Thanks a lot for your contribution 🙏🏻 This is a valid change. As I try to keep the code clean, I left a comment proposing a rename to polygons_to_mask and deprecation of the polygon_to_mask function. What do you think?

@DancinParrot
Copy link
Author

Hi @SkalskiP ! Thanks for reviewing and providing feedback on my PR!

As I try to keep the code clean, I left a comment proposing a rename to polygons_to_mask and deprecation of the polygon_to_mask function. What do you think?

Yup, I think that's a great idea. It'd certainly be a more accurate representation of the function's intended purpose. Since multiple lists of vertices within the "segmentation" field (which in itself is a list) of a COCO dataset annotation is perfectly acceptable, the function should be capable of handling both one and multiple lists without relying on yet another function that introduces another unnecessary layer of abstraction.

I'll proceed with implementing the suggested changes and deprecating the current polygon_to_mask() function in favour of polygons_to_mask().

Thanks again for your feedback, appreciate it!

@DancinParrot
Copy link
Author

Hi @SkalskiP ! During refactoring, I noticed a very similar function _polygons_to_mask() (see here). The function follows a similar methadology in calling polygon_to_mask() for each polygon in the input list, though it does not require the merging of masks into one. In this case, given the similarity of the names with a slight difference in terms of functionality, would the introduction of polygons_to_mask() cause confusion?

I suppose we could refactor it in such a way but it does seem to be a bit odd:

def _polygons_to_masks(
    polygons: List[np.ndarray], resolution_wh: Tuple[int, int]
) -> np.ndarray:
    return np.array(
        [
            polygons_to_mask(polygon=[polygon], resolution_wh=resolution_wh)
            for polygon in polygons
        ],
        dtype=bool,
    )

@DancinParrot
Copy link
Author

Hi @SkalskiP ! Any updates on the above?

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