Skip to content
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

Optimize line zone and add unit tests #1205

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

tc360950
Copy link
Contributor

@tc360950 tc360950 commented May 17, 2024

Description

My experiences with supervision's tracking indicate that (quite surprisingly) in some cases LineZone may be a bottleneck.
Think about edge computing scenarios - where detection model is small and one tracks a lot of objects. In such cases every FPS counts.
The following is a cProfile output for Yolov8m + ByteTrack + LineZone run 10 times on market square video.

image

As you can see the cost of LineZone is already quite considerable. Now, consider that there are not a lot of objects (average of 35 per frame) and that the easiest, most common approach to improving counting precision is adding more lines and averaging results.
For these reasons, in my usage of supervision, I've had to optimize LineZone.
The new implementation takes only 0.74 seconds on market square example compared to 11.1s seconds of the old implementation. To support this change I've added unit tests for LineZone.

Note that quite unnecessarily internals of LineZone are exposed through two public static methods - calculate_region_of_interest_limits and is_point_in_limits. I can only guess that it's because unit tests were just written for parts (encapsulated in those two methods) of LineZone code. These methods should not be public because they expose specifics of how line counting is implemented, hindering any attempts at changing the method (as is in this case). I've left those two methods unchanged, because I don't want to break peoples' code (even though those two methods were not documented as a part of API), but I've added docstring with warning that they are not a part of the API.

Type of change

Please delete options that are not relevant.

  • Feature optimization (non-breaking change which optimizes existing functionality)

How has this change been tested, please provide a testcase or example of how you tested the change?

New LineZone unit tests.

@LinasKo
Copy link
Collaborator

LinasKo commented May 17, 2024

Hi @tc360950 👋

Looking purely at the numbers, it's a massive improvement. I'll see if I can review it soon.

Is this in comparison with the develop branch (supervision==0.21.0rc5), or the main supervision package?

@tc360950
Copy link
Contributor Author

Hi @LinasKo
I compared to the develop branch but LineZone code is the same on main.

@LinasKo
Copy link
Collaborator

LinasKo commented May 17, 2024

Am I reading the graph right - is cross_product the most expensive operation here? (also given we're calling it frequently)

Is that the main reason for transforming to a vertical-line system?

@tc360950
Copy link
Contributor Author

Exactly, that's why it's easier to transform to coordinates where the line is vertical - then everything can be vectorized.

@LinasKo
Copy link
Collaborator

LinasKo commented May 17, 2024

@SkalskiP, when you're back - I'd like your input. I think this is a fascinating case where an issue is revealed, and the problem is solved, but with a method that's too complicated for us. Specifically, I see that:

  • LineZone takes too long. Very obviously so.
  • We're repeatedly calling the non-vectorized vector.cross_product. It would be much faster if vectorized, possibly precomputed before the loop - similar to in_limit here.
  • I think there's a tracker_id inconsistency in prior code - I can't recall when we'd expect a None while iterating over tracker_id. The None check here is mildly getting in the way of vectorization. Related: sv.Detections has exactly N of each member, except for tracker_id. #1092
  • I wholeheartedly agree that is_point_in_limits and calculate_region_of_interest_limits should be flagged as private to this class.
  • A lot of work went into producing the tests - we should keep those, with full credit to @tc360950.

Let's wait for @SkalskiP, but my verdict is that to merge this, I'd need to know it's significantly more efficient than a solution where we use vectorized and possibly precomputed cross products.

Meanwhile, @tc360950, if you have some time, would you be able to split the test into a separate PR? I assume they'd apply just as well for the current solution.

@tc360950
Copy link
Contributor Author

@LinasKo, sure. I've created a separate PR.
I will think about optimizing the code with the existing approach.

@LinasKo
Copy link
Collaborator

LinasKo commented May 17, 2024

Thanks! To add more freedom, I'd say, implementations of trigger don't have to use the inefficient calculate_region_of_interest_limits and is_point_in_limits methods.

If there's a significantly faster way to line count, we can deprecate them.

@SkalskiP
Copy link
Collaborator

Hi @tc360950 👋🏻 Improving LineZone performance by 10-20x sounds fantastic, but like @LinasKo, I am concerned that the proposed changes significantly increase complexity. I strive to keep the Supervision codebase relatively simple to understand, making maintenance easier and lowering the entry barrier for future contributors.

Therefore, I'd like to propose the following course of action:

  • Merge the LineZone unit tests.
  • Profiling analysis shows that most time is spent on is_point_in_limits and cross_product because these calculations are done in a for loop instead of being vectorized. I propose writing an alternative implementation where we minimize for loops and process as much as possible in parallel using numpy. If you have time, @tc360950, I think this approach has the potential to achieve similar optimization levels. If you're willing, please open a separate PR and conduct similar profiling.
  • Depending on the results, we will choose between the option proposed in this PR and the new vectorized approach.

@tc360950, please let me know your thoughts.

@tc360950
Copy link
Contributor Author

@SkalskiP I think it's a good idea. I actually have a working draft of vectorization of the original method. I'll open a separate PR soon.

@SkalskiP
Copy link
Collaborator

Hi @tc360950 👋🏻 I just wanted to check in and see how it's going. Do you need any help?

@tc360950 tc360950 mentioned this pull request May 24, 2024
1 task
@tc360950
Copy link
Contributor Author

@SkalskiP I was waiting for the unit test PR :) Here's the PR with vectorized LineZone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants