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

Adding support for Yolo-OBB (Oriented Bounding Boxes) format #1227

Merged
merged 23 commits into from
Jun 19, 2024

Conversation

Bhavay-2001
Copy link
Contributor

@Bhavay-2001 Bhavay-2001 commented May 24, 2024

Description

PR for issue #1096

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?

Any specific deployment considerations

Docs

  • Docs updated? What were the changes:

Tagging @LinasKo @SkalskiP

@Bhavay-2001
Copy link
Contributor Author

Hi @LinasKo, I would be needing some suggestions.
I have made few initial changes. Currently we calculate the xyxy which is passed to the Detections class here. If we calculate xyxyxyxy pairs, how are we gonna pass it to the Detections class such that we get the annotations returned?

What changes needs to be done in Detections class?

@Bhavay-2001
Copy link
Contributor Author

Bhavay-2001 commented May 26, 2024

Hi @SkalskiP @LinasKo, any updates on this?

@Bhavay-2001
Copy link
Contributor Author

Hi @SkalskiP, can you please review this once and leave comments?

@LinasKo
Copy link
Collaborator

LinasKo commented May 31, 2024

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.

Meanwhile, what would help is having a Colab Notebook that shows shows how the changes work. Us reviewers get stuck in tons of different issues and have little time to give to each one. 🙂

Edit: I see this is the start of the solution. Let me think of how to address your question.

@LinasKo LinasKo marked this pull request as draft May 31, 2024 09:59
@LinasKo
Copy link
Collaborator

LinasKo commented May 31, 2024

Converting to draft for now, until we have a working implementation

@LinasKo
Copy link
Collaborator

LinasKo commented May 31, 2024

Copying your old comment from the issue:

What I meant was that this line reshapes the annotations to a 4x2 matrix in case we have 8 numbers.
And this line calculates the min and max values from the 4x2 matrix.

What I am suggesting is that maybe we can add a parameter which when set True can simply multiply the annotations with width and height and return it.

Within the yolo_annotations_to_detections() method, the goal is to insert the OBB into Detections.data under the key ORIENTED_BOX_COORDINATES, as that's where OBB data is set by our other connects.

Looking at the the lines you mentioned, they seem to treat everything above len==5 as polygon, drawing a bounding box around for xyxy. Indeed, we need to clearly distinguish when OBB output is required and only set the Detections.data[ORIENTED_BOX_COORDINATES] key when we're 100% sure that's what the user intends. Therefore, I believe you're on the right track and as_obb: bool is required here too.

Copy link
Collaborator

@LinasKo LinasKo left a 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.

supervision/dataset/formats/yolo.py Outdated Show resolved Hide resolved
supervision/dataset/formats/yolo.py Outdated Show resolved Hide resolved
supervision/dataset/formats/yolo.py Outdated Show resolved Hide resolved
supervision/dataset/formats/yolo.py Outdated Show resolved Hide resolved
@Bhavay-2001
Copy link
Contributor Author

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?
Thanks

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 6, 2024

To test, you'd need the following:

  1. Create a new notebook in Google Colab
  2. Add a command to install your branch, !pip install git+https://github.com/Bhavay-2001/roboflow-supervision.git@OBB
  3. Get the data, by either:
  • Uploading the dataset images and annotations to the colab directly
  • Uploading to Roboflow App and downloading like in this example
  1. Call from_yolo, check that OBB dataset is created and has the OBB boxes inside. You can probably print the object and see the values.
  2. Call yolo_annotations_to_detections and see that the extra field in data is added and filled with obb values.

@Bhavay-2001
Copy link
Contributor Author

Hi Linas, can I change the tests in this file only test\dataset\formats\test_yolo.py. Will it count as testing the new feature only or creating a Colab notebook will be a better idea?

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 6, 2024

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 git+ in pip install command.

@Bhavay-2001
Copy link
Contributor Author

Bhavay-2001 commented Jun 6, 2024

Soo the checks suggests that some of the tests on yolo_annotations_to_detections failed because I didn't made the changes to the function calling in the test file.

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 is_obb parameter? If the user wants the OBB format, then we can mention True and obtain it else it would be False

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 6, 2024

Indeed, is_obb should have a default value of False, so no one's code fails after the update. This should fix test failures.

Where modifications to tests are needed, you may design the inputs whichever way it feels better.

@Bhavay-2001
Copy link
Contributor Author

Hi @LinasKo, this is the link to the colab notebook. Please have a look and let me know.
Thanks

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 13, 2024

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.

@Bhavay-2001
Copy link
Contributor Author

Bhavay-2001 commented Jun 13, 2024

Hi @LinasKo, is it okay to upload it here like this?
dataset.zip

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 13, 2024

Yes, thank you very much!
I'll make sure to review this as soon as I can.

@Bhavay-2001 Bhavay-2001 marked this pull request as ready for review June 13, 2024 17:40
@Bhavay-2001
Copy link
Contributor Author

Hi @LinasKo, any updates on this?

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 18, 2024

Hi @Bhavay-2001,

Apologies for the wait. I've added a bit of code that adapts this to our system.
It turns out, ultralytics provides [[x, y], [x, y], [x, y], [x, y]], and that's what our Obb annotator expects.

Here's an upgraded version of your Colab:
https://colab.research.google.com/drive/1ld5aiOGrhaWzfN1HNNnxMuyy9PQhIrVn?usp=sharing

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 🙂

@LinasKo LinasKo merged commit fbb770a into roboflow:develop Jun 19, 2024
9 checks passed
@Bhavay-2001 Bhavay-2001 deleted the OBB branch June 27, 2024 14:20
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.

[DetectionDataset] - expand from_yolo to include support for OBB (Oriented Bounding Boxes)
2 participants