-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
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.