-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Tracker #203
Tracker #203
Conversation
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.
Hello there, thank you for opening an PR ! 🙏🏻 The team was notified and they will get back to you asap.
Hi @dbroboflow, I'm currently reviewing other PRs. There is a lot of work happening in parallel. I will do my best to check it today. |
Hi @dbroboflow 👋🏻
|
Hello @SkalskiP and @dbroboflow, I saw the initial implementation. My thoughts after reviewing the implementation are as following. It would be easy to use something like
curent implementation has too much overhead for example, byte_track.py
If we do this then when there is a new tracker, it can also be implemented easily with same structure. Tracker API usage will be same in the future also. |
@hardikdava 100 agree. Take a look at #195, The design I proposed is matching how you see stuff as well. |
We are trying to pass arrays on the inference server. We have some bottlenecks. Also we want to remove object conversion, and pass to np array directly. Current version:
Plan:
I can add
But also we want to keep original bytetrack input. If it works, I can do it as optional. Thank you, |
time_since_update = 0 | ||
|
||
# multi-camera | ||
location = (np.inf, np.inf) |
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.
Suggestion: The class variables track_id, is_activated, state, history, etc., will be shared across instances of BaseTrack. If they are supposed to be different for each instance, they should be instance variables (i.e initialized in a init method).
def __init__(self):
self.track_id = 0
self.is_activated = False
self.state = TrackState.New
self.history = OrderedDict()
self.features = []
self.curr_feature = None
self.score = 0
self.start_frame = 0
self.frame_id = 0
self.time_since_update = 0
self.location = (np.inf, np.inf) # Coordinates in a multi-camera setup
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.
Great point @artyaltanzaya! We absolutely want to make those tied to instance not a class.
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 updated as in the comment. But rn we are creating new object for each camera. Do you need some features for tracker like add camera(camera_link), remove camera(camera_id), update(self, output_results, img_info, img_size, camera_id) ?
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.
Nope. The whole camera abstraction should be on the inference server side.
docs/tracker/tracker_bytetrack.py
Outdated
|
||
return tracker_ids | ||
|
||
model = YOLO('yolov5s.pt') |
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 file yolov5s.pt is directly used in the script. We might want to check if this file exists before running.
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 don't want us to have that script inside the project. We should aim for a similar format of documentation as with all other supervision
features. Take a look here: https://roboflow.github.io/supervision/dataset/core/
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 is ultralytics repo and it downloads automatically the pre-trained model. I added that one as a sample. We can remove the test files and add to another doc if you want.
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 @dbroboflow 👋🏻! We don't store example scripts in supervision. But we document API, just like in the link I provided. If you look there, we also have examples in the docs. We just format those in a different way.
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 @SkalskiP! I updated the tracker as update_from_numpy and update_from_detections. When we are merging it we can delete the samples. I don't delete these files, when you tested it again. I will remove it.
@SkalskiP let me know once it ready to test. I can do that and also review it together. |
@hardikdava I updated the repo. Also I tested it and checked the outputs. I think we can do review. Also, if you will set a meeting, I would like to join when you are reviewing it. |
Hmm I just touch the setup.py file which you will delete in the future. I will check the conflicts. Also I added the similar docs comment for docs generation. Which tool are you using like Sphinx? |
We won't be able to merge if you have conflicts. So please resolve them before we start the review.
We use MKDocs. You can check if everything works. Just run |
Hi, @dbroboflow 👋🏻!
|
return mean, covariance | ||
|
||
def predict(self, mean, covariance): | ||
"""Run Kalman filter prediction step. |
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.
Reformat docstrings to format used in other parts of the codebase.
Hi, @artyaltanzaya 👋🏻! Could you take a look at the PR and confirm it builds, installs, and work as expected in the issue: #195? |
@SkalskiP it looks like you left comments recently, were those incorporated? should I wait until the changes are made to run tests? |
return fuse_cost | ||
|
||
|
||
def bbox_ious(boxes, query_boxes): |
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.
@dbroboflow We can utilize sv.box_iou_batch
to find intersection over union
between (N, 4)
and (M,4)
boxes. This method is much faster and implemented in vectorized. You implementations will be slower by utilizing for
loops. Please check it out.
@@ -0,0 +1,57 @@ | |||
from tqdm.notebook import tqdm | |||
from ultralytics import YOLO |
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.
@SkalskiP @dbroboflow import ultralytics
package might create an issue for license. Please verify it once.
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.
These are just for test scripts. When we are merging it, we will delete it. Also I added it to the docs. We don't need to install ultralytics package.
], | ||
long_description_content_type="text/markdown", | ||
url="https://github.com/roboflow/supervision", | ||
install_requires=["numpy>=1.20.0", "opencv-python", "matplotlib", "pyyaml"], |
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.
@dbroboflow @SkalskiP Please add lap
or scikit
based on usage.
], | ||
long_description_content_type="text/markdown", | ||
url="https://github.com/roboflow/supervision", | ||
install_requires=["numpy>=1.20.0", "opencv-python", "matplotlib", "pyyaml"], |
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.
@SkalskiP @onuralpszr Can we make installation like pip install supervision[tracker]
and it will install dependency for tracker otherwise use normal installation. Installation of scikit-learn
might create an issue for arm
devices. What do you think?
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 see "arm64" support for scikit-learn
https://pypi.org/project/scikit-learn/1.3.0/#files
"arm" (32-bit) already consider old. And If you buy a new arm based board, it will be high likely arm64 devices so it is fine on that as well any other package worry about ?
img_info=img_info, | ||
img_size=img_size, | ||
) | ||
tracker_id = match_detections_with_tracks(detections=detections, tracks=tracks) |
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.
@dbroboflow Why do we have to match detections
and tracklets
again?
Detection: supervision detection result with track id | ||
""" | ||
# update tracker | ||
tracks = self.update_from_numpy( |
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.
@dbroboflow Just try to convert tracks
to sv.Detections
format and you should be fine.
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 Hardik,
We will use it in the inference server. Because of that, there are separated methods.
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.
Okay. Understood. But still in the method of update_from_detection
method you are matching detection and tracks again which is unnecessary part. I meant you can just convert result of update_from_numpy
to sv.Detection
format.
Closing this PR. @dbroboflow opened a new clean one here: #256 |
Description
This commit includes updates to the repository's main README.md file, providing usage details, and explanations for the example code located in the "examples" folder. The updates aim to improve the documentation and guide users on how to utilize the Roboflow Computer Vision Library for object detection and tracking.
Dependencies required for this change:
Dependencies specified in setup.py
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?
The code for the example "Object Tracking with Byte Track" was tested on a sample video and successfully performed object detection and tracking. The test case includes verifying the correct detection of objects, assigning unique tracker IDs to each object, and generating the output video with annotated bounding boxes.
Docs
Bytetrack added under the supervision/tracker folder.
Updated the "Examples" section to include instructions for running the "Object Tracking with Byte Track" example code under the "examples" folder.