-
Notifications
You must be signed in to change notification settings - Fork 249
OC-SORT on top of 2.1.0 #207
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
base: develop
Are you sure you want to change the base?
Conversation
| minimum_consecutive_frames: int = 3, # should change this for min_hits? | ||
| minimum_iou_threshold: float = 0.3, | ||
| direction_consistency_weight: float = 0.2, | ||
| high_conf_det_threshold: float = 0.6, |
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 want argument names to be easy to understand. Avoid abbreviations. Keep them short and clear.
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.
| high_conf_det_threshold: float = 0.6, | |
| high_conf_detect_threshold: float = 0.6, |
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 can change it here, but we need to also change it in bytetrack @SkalskiP
trackers/core/ocsort/tracker.py
Outdated
| self, | ||
| lost_track_buffer: int = 30, | ||
| frame_rate: float = 30.0, | ||
| minimum_consecutive_frames: int = 3, # should change this for min_hits? |
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.
re: should change this for min_hits? why?
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.
it was a now removed comment, because in the original version there was that parameter, but in our api it translates to that
trackers/core/ocsort/tracker.py
Outdated
| direction_consistency_matrix: Direction of the tracklet consistency cost matrix. | ||
|
|
||
| Returns: | ||
| tuple[list[tuple[int, int]], list[int], list[int]]: Matched indices, |
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.
Let's drop types here as well.
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.
Do it for all the classes/methods/functions we have in our API.
| @@ -0,0 +1,261 @@ | |||
| import numpy as np | |||
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.
Take a look at current develop. I added license headers to all our files. Make sure to include them as well.
| import numpy as np | ||
|
|
||
|
|
||
| def xyxy_to_xcycsr(xyxy: np.ndarray) -> np.ndarray: |
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.
Nice. Make sure we add those unit tests.
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.
where do unit tests go?
| return np.array([x, y, s, r]) | ||
|
|
||
|
|
||
| def xcycsr_to_xyxy(xcycsr: np.ndarray) -> np.ndarray: |
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 pretty sure we could make it faster with inplace operations. I jsut vibe-coded this version, but please dig a bit deeper into this.
def xcycsr_to_xyxy_vec(boxes: np.ndarray) -> np.ndarray:
w = np.sqrt(boxes[:, 2] * boxes[:, 3])
half_w = w / 2
half_h = boxes[:, 2] / w / 2
xyxy = np.empty_like(boxes)
xyxy[:, 0] = boxes[:, 0] - half_w
xyxy[:, 1] = boxes[:, 1] - half_h
xyxy[:, 2] = boxes[:, 0] + half_w
xyxy[:, 3] = boxes[:, 1] + half_h
return xyxyThere 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.
makes sense, though im using it to convert one box at a time and at worst it can run in a loop that has only a few iterations, so performance-wise it should have a big impact, but then you can't input several boxes
| from numpy.typing import NDArray | ||
|
|
||
|
|
||
| class KalmanFilter: |
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.
Just so I understand. Why do we have specific SORTKalmanBoxTracker and ByteTrackKalmanBoxTracker per tracker? And here KalmanFilter is placed in general directory. The docstring looks like it would match per tracker implementation.
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.
we discussed it briefly over slack, this is one of the changes i wanted to add for easier scalability of the code. The previous trackers built all in the tracklet class (which are called like ByteTrackKalmanBoxTracker). What i did here is decoupling the KF functionalities from the tracklet. Now the tracklet consumes a KF and makes it nicer to code the different tracklets. We should maybe rewrite the other trackers in this style
trackers/core/ocsort/utils.py
Outdated
| # Copyright (c) 2026 Roboflow. All Rights Reserved. | ||
| # Licensed under the Apache License, Version 2.0 [see LICENSE for details] | ||
| # ------------------------------------------------------------------------ | ||
| # Copied from OC-SORT https://github.com/noahcao/OC_SORT/ |
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 would make it Modified from C-SORT https://github.com/noahcao/OC_SORT/ and invest in code quality of those utils.
…w/trackers into feat/core/ocsort-release
…d update also when unmatched detections = 0
|
@AlexBodner I just merged big PR adding support for |
docs/trackers/ocsort.md
Outdated
|
|
||
| ## API | ||
|
|
||
| ::: trackers.core.ocsort.tracker.OCSORTTracker |
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.
This should be moved to API Reference docs tab. http://trackers.roboflow.com/develop/api/trackers/
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
trackers/core/ocsort/utils.py
Outdated
| from trackers.core.ocsort.tracklet import OCSORTTracklet | ||
|
|
||
|
|
||
| def speed_direction(bbox1: np.ndarray, bbox2: np.ndarray) -> np.ndarray: |
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.
All the functions that we want to keep privat should be prefixed with _. Most of those utils should be private.
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
…w/trackers into feat/core/ocsort-release
What does this PR do?
Adding OC-SORT tracker
Type of Change