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

Fixed obb key check in ultralytics_results #1093

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

macc-n
Copy link
Contributor

@macc-n macc-n commented Apr 4, 2024

Description

Please include a summary of the change and which issue is fixed or implemented. Please also include relevant motivation and context (e.g. links, docs, tickets etc.).
Fix issue #8107

Allows you to convert detections from Ultralytics which do not contain the key "obb".
An if statement is added before accessing the "obb" key in the dictionary in order to verify its presence.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

The fix was tested with the code snippet provided in the official documentation

Any specific deployment considerations

No

Docs

  • Docs updated? What were the changes:

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2024

CLA assistant check
All committers have signed the CLA.

@LinasKo
Copy link
Collaborator

LinasKo commented Apr 4, 2024

Thank you for a quick submission!

I'd like to request one change - can you please merge the two if conditions?

if "obb" in ultralytics_results and ultralytics_results.obb is not None:


Update: actually it's a bit unusual that string-accessor and attribute accessor is needed. The latter should just return None.

Can you provide a test where this causes an issue?

@macc-n
Copy link
Contributor Author

macc-n commented Apr 4, 2024

Here's a Google Colab notebook with a test to reproduce the error.

However, the latest version of ultralytics returns the Results object with "obb" key equals to None. This fix can improve compatibility with older versions.


Update: the two if conditions are now merged

@LinasKo
Copy link
Collaborator

LinasKo commented Apr 4, 2024

@SkalskiP, what's your take on it?
I say this is a very neat contribution and we should merge it.

The error does appear on ultralytics==8.0.188, whereas v8.1.0 was only released in January.

@SkalskiP
Copy link
Collaborator

SkalskiP commented Apr 5, 2024

Hi @LinasKo and @macc-n, I like that change. It was a bit shortsighted on our part. Not everyone has to use the latest version of ultralytics.

@SkalskiP SkalskiP merged commit 299c933 into roboflow:develop Apr 5, 2024
9 checks passed
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.

4 participants