-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 detections_to_coco_annotations
function for empty polygons.
#1086
base: develop
Are you sure you want to change the base?
Conversation
Hi @Youho99! Thanks a lot for the time you spend creating this PR. Could you explain what's the problem with having |
I don't know if it's really a problem to have both Just, in this case, there are things to modify:
For information, I dissociated
Would you like me to also do the PS: the result of the |
Small modification: I left the "segmentation" field empty when the technology is detection (bbox), because otherwise, we could not import into CVAT (so I suppose that the "segmentation" field is obligatory in the coco format) |
@LinasKo could you take a look at this next week? |
Sure. I've already verified the error happens and is subsequently fixed by this PR, but haven't looked at all into coco's format specification. |
Hi @Youho99 👋 I'll be reviewing this tomorrow. So far, I have one question: should Can you also comment a bit about the |
Hi @LinasKo ! What do I think of it : I would therefore be in favor of leaving the bbox information (and the area, it goes together) in addition to the segmentation information. Note: In my current PR, I decided to follow the same logic as the yolo and pascal_voc formats, which are formats that do not save bbox information. For the question of the difference in the calculation of the bbox: So here's the problem: I made a diagram to illustrate my points, admire my artistic talents! |
That's a very thorough explanation :) Let's add back the I'd be keen to fix the Grounded SAM bbox issue too. Am I right that if we used Grounded SAM, e.g. via I'll ask internally too - I might be confused about potential entry points of grounded SAM data into our ecosystem. But this sounds like a separate PR. |
Yes, I agree to add it too For the second part of the message: I made a code (for another use) allowing to recalculate the SAM bboxes precisely starting from a larger bbox (this simulates our use case). I'll see about making it open-source (I'll have the info in a few hours) if that helps. |
@Youho99 Nowhere in the Supervision repository do we directly depend on GroundingDINO results. Could you point out where you saw this?
Once again, I'm not sure why we are talking about GroundingDINO here. :) |
Here's why I'm talking about grounding dino and grounded sam: I use Supervision, autodistill_grounding_dino and autodistill_grounded_sam, as in issue #1085 In fact, when we do as_coco(), the code takes the Detections object, reads the annotations.bbox and annotations.segmentation part However, I'm talking about how the Detections object is constructed, where the data inside comes from. And so, when we convert the Detections object into coco, we note the bboxes of Grounding Dino, and the segmentations of Grounded SAM (and therefore we do not record the bboxes of the segmentation masks) Therefore, it may also seem logical to modify the registration of the segmentation bboxes directly in autodistill_grounded_sam (by which I mean that it will certainly be more modular to do so, by solving the problem at the root, if other modules do not not present the problem) |
There would therefore be 2 PRs: A second allowing the bboxes of the segmentation masks to be recorded in the Detections object generated by autodistill_grounded_sam (on autodistill_grounded_sam) |
This is the solution I would lean towards. Supervision strives to be a general library. In this case, its task is to save the user's input as a COCO dataset. Regarding the specific problem with GroundedSAM, it should be resolved in autodistill. |
Always include box, area, and segmentation in the result COCO JSON.
@SkalskiP @LinasKo Then, I will switch to autodistill_grounded_sam to calculate the bbox of the segmentations While doing my tests, I noticed that I had an error when I tried to regenerate the DetectionDataset object with I tried with supervision 0.19.0, and I get the error I therefore deduce that I did not introduce this bug, but that it exists... |
Allows reconstruction of disjointed masks
Hello ! I took the opportunity to correct the bug cited in my previous message as well. Now, the |
Description
Fix issue #1085
Allows you not to cause an error if a polygon is empty
Modify the fields of the json file:
Type of change
How has this change been tested, please provide a testcase or example of how you tested the change?
Follow
Minimal Reproducible Example
of issue #1085Any specific deployment considerations
Json fields modified (see comment in description)
Docs