-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Adding support for Yolo-OBB (Oriented Bounding Boxes) format #1227
Conversation
Hi @LinasKo, I would be needing some suggestions. What changes needs to be done in |
Hi @SkalskiP, can you please review this once and leave comments? |
Hi @Bhavay-2001, Apologies for the wait and the lack of responses in the issue page. Submitting a PR with your proposal of the solution is the right way to go. I'll review it asap.
Edit: I see this is the start of the solution. Let me think of how to address your question. |
Converting to draft for now, until we have a working implementation |
Copying your old comment from the issue:
Within the Looking at the the lines you mentioned, they seem to treat everything above |
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.
Very good attempt!
I left a few review comments. Nothing major, though it will be important to test in the Colab to check if everything works well.
Hi @LinasKo, could you please let me know how can I create a Colab notebook to test these changes. I mean any sample/steps I can follow to check this? |
To test, you'd need the following:
|
Hi Linas, can I change the tests in this file only |
Adding tests would be useful, but Colabs are the primary way we verify feature correctness. P.S. I've updated step 2, as I forgot |
Soo the checks suggests that some of the tests on I wanted to discuss a thing for this function. In this function, I should be adding the is_obb parameter too in order for the test to pass, but should it be passed as a single boolean value or list of boolean values, because in the tests above, some test have only 1 annotation while some have 2, so it really depends on the size of the annotation. Could you suggest something here? Also, I was thinking should we add a default value for the |
Indeed, Where modifications to tests are needed, you may design the inputs whichever way it feels better. |
Hi @Bhavay-2001 , based on a quick glance, it looks good. Would you be able to share the dataset? The simplest option would be to put it on Google Drive, and create a shareable link. It can be zipped into one file, if that's more convenient for you. |
Hi @LinasKo, is it okay to upload it here like this? |
Yes, thank you very much! |
Hi @LinasKo, any updates on this? |
* Ultralytics return (-1, 4, 2), and that's waht our annotators expect
Hi @Bhavay-2001, Apologies for the wait. I've added a bit of code that adapts this to our system. Here's an upgraded version of your Colab: Overall, I believe this can now be merged. And if other reviewers request a change, I'll update the code. Thank you very much for the contribution! In the end, I found that barely any changes were needed 🙂 |
Description
PR for issue #1096
Type of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?
Any specific deployment considerations
Docs
Tagging @LinasKo @SkalskiP