-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix smoother removing tracking_id from detections #944
Fix smoother removing tracking_id from detections #944
Conversation
* Addresses issue roboflow#928 matching implementation in ByteTrack, but sidestepping the underlying cause outlined in roboflow#943.
Ready for review. |
@@ -115,4 +115,8 @@ def get_smoothed_detections(self) -> Detections: | |||
if track is not None: | |||
tracked_detections.append(track) | |||
|
|||
return Detections.merge(tracked_detections) | |||
detections = Detections.merge(tracked_detections) | |||
if len(tracked_detections) == 0: |
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.
if len(tracked_detections) == 0: | |
if len(detections) == 0: |
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.
Hi @LinasKo 👋🏻 ! Great find! That's probably the only change I'd make.
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.
Done 👍
Also verified the __len__
implementation just in case. Seems to be alright - can't imagine changes to Detections.empty
that would break this.
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.
I'm considering modifying the Detections.empty
API to include arguments such as with_confidence
, with_tracker_id
, etc. So that in the future, in a similar situation, we would only need to do Detections.empty(with_tracker_id = True)
.
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.
The problem with that (#943, Option 4) is that intermediary functions would also need with_confidence
and with_tracker_id
, or you need to forbid them from ever accepting zero-length iterables.
We're actually at the best place to show this.
If nothing is detected, detections = Detections.merge(tracked_detections)
takes []
. It then tries to call Detections.empty
- but what params should it set?..
Addresses issue #928 matching implementation in ByteTrack, but sidestepping the underlying cause outlined in #943.
Description
Smoother takes as input Detections with a defined
tracker_id
, but when no detections are left after smoothing, it would return an object wheretracker_id
isNone
.When accessing such detections, for example, using
zip(detections.class_id, detections.tracker_id)
, aNoneType is not iterable
error would be raised.Similar to implementation of
ByteTrack.update_with_detections()
does it, thetracker_id
is added explicitly in cases where it's not present already (in fact - only when no detections are left).Fixes issue: #928
Related Issue: #943
List any dependencies that are required for this change.
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?
Input Video:
https://github.com/roboflow/supervision/assets/6500785/d61b7e9d-2992-46d0-9cd1-6a02388dbe72
Test Code:
Test code
Any specific deployment considerations
None.
Docs
No updates.