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 issue #299 #300

Merged
merged 4 commits into from Aug 22, 2023
Merged

Conversation

ShubhamKanitkar32
Copy link
Contributor

Description

Fixes issue - #299

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • 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?

Test script provided under the issue

@CLAassistant
Copy link

CLAassistant commented Aug 18, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there, thank you for opening an PR ! 🙏🏻 The team was notified and they will get back to you asap.

@@ -20,7 +20,10 @@ def polygon_to_mask(polygon: np.ndarray, resolution_wh: Tuple[int, int]) -> np.n
"""
width, height = resolution_wh
mask = np.zeros((height, width))
cv2.fillPoly(mask, [polygon], color=1)

# Empty numpy array check

Choose a reason for hiding this comment

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

Thank you for addressing the bug; I suggest using if polygon.size > 0: to directly check for an empty array, ensuring against cases with all-zero coordinates.

@SkalskiP SkalskiP self-requested a review August 18, 2023 22:11
@SkalskiP
Copy link
Collaborator

Hi, @ShubhamKanitkar32 👋🏻! Thanks a lot for submitting a PR so fast. We definitely can't allow supervision to crash when processing roboflow results. The question that I have is: are we fixing the right thing?

It seems reasonable for me for the polygon_to_mask to crash with invalid input. After all... you need at least three distinct points to draw a polygon on a 2d plane.

Maybe we should move that fix to sv.Detections.from_roboflow and check if returned detection is valid? It is a bit surprising that the segmentation result can be an empty list of points and be valid.

@SkalskiP SkalskiP added bug Something isn't working version 0.14.0 Feature to be added in `0.14.0` release labels Aug 18, 2023
@SkalskiP SkalskiP added this to the version: 0.14.0 milestone Aug 18, 2023
@ShubhamKanitkar32
Copy link
Contributor Author

@SkalskiP no problem, happy to contribute!

I agree with you. It makes more sense for it to live under sv.Detections.from_roboflow() -> process_roboflow_result()

@SkalskiP
Copy link
Collaborator

Awesome @ShubhamKanitkar32! 🔥 In that case, I have to ask you for two things:

Once again thanks for helping us to get to the bottom of this problem 🙏🏻

@SkalskiP
Copy link
Collaborator

Hi @ShubhamKanitkar32 👋🏻! I think we are ready to implement. Take a look at my last comment under your issue: #299 (comment)

@SkalskiP
Copy link
Collaborator

Hi, @ShubhamKanitkar32 I've made some changes to your implementation and added unit tests to make sure it works as expected. I'm merging changes into develop. Please check if everything works as expected.

@SkalskiP SkalskiP merged commit 2ec33db into roboflow:develop Aug 22, 2023
6 checks passed
@SkalskiP
Copy link
Collaborator

btw, @onuralpszr I love per-commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working version 0.14.0 Feature to be added in `0.14.0` release
Projects
Status: Current Release: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants