supervision icon indicating copy to clipboard operation
supervision copied to clipboard

Fixed obb key check in ultralytics_results

Open macc-n opened this issue 1 year ago • 4 comments

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

  • [x] 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:

macc-n avatar Apr 04 '24 15:04 macc-n

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 04 '24 15:04 CLAassistant

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?

LinasKo avatar Apr 04 '24 16:04 LinasKo

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

macc-n avatar Apr 04 '24 16:04 macc-n

@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.

LinasKo avatar Apr 04 '24 16:04 LinasKo

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 avatar Apr 05 '24 11:04 SkalskiP