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

Add LineZone unit tests #1206

Merged
merged 22 commits into from
Jun 5, 2024
Merged

Conversation

tc360950
Copy link
Contributor

@tc360950 tc360950 commented May 17, 2024

Description

Added unit tests for LineZone.

Type of change

Please delete options that are not relevant.

  • New unit tests

test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
test/detection/test_line_counter.py Outdated Show resolved Hide resolved
@LinasKo
Copy link
Collaborator

LinasKo commented May 23, 2024

Thank you for the changes, @tc360950 !

Would you have some time to add a few extra cases? What I'm thinking of is the following:

  • object crossing pependicular to the line, but to its side. As if crossing the line extended to infinity, but not the segment defined by start-end.
  • diagonal line
  • negative coordinates
  • Ensure error is raised when no anchors are defined

Does that sound like something you'd have time for, by any chance?

@tc360950
Copy link
Contributor Author

@LinasKo Ok, I will add the changes. Could you clarify what you mean by "diagonal line" - there are already test cases for lines which are not horizontal or vertical. Do you want more (e.q. with different slope)?

@LinasKo
Copy link
Collaborator

LinasKo commented May 24, 2024

Ah, indeed non-vertical and non-horizontal is what I meant. If we have it, we don't need any additional tests for it.

@tc360950
Copy link
Contributor Author

@LinasKo I've added changes you had requested

@LinasKo
Copy link
Collaborator

LinasKo commented May 24, 2024

Thank you @tc360950, this looks great!

I'll verify over the weekend, potentially push a tiny stylistic update, but otherwise, it looks good to go 😉

@LinasKo
Copy link
Collaborator

LinasKo commented May 28, 2024

Hey @tc360950,

Apologies for the delay! To save time, I've pushed a few updates to the branch.
Despite our questions, the function names were descriptive enough so that docstrings aren't needed. We typically go very light on comments if there's code explaining the behaviour nearby.
Also, while not a typical practice, we internally agreed to ship a similar ValueError check to PolygonZone. I've added some tests for that.

@SkalskiP, This is ready to merge, but your review is outstanding. When you have the time, please resolve it ;)

* Add Colab for visuals: https://colab.research.google.com/drive/179sq8joJ-7JSYMqYNBQlMIPYEClq1PDi?usp=sharing
* Need to visualise other cases - not sure if all are necessary
@LinasKo
Copy link
Collaborator

LinasKo commented May 31, 2024

Hey @tc360950,

We've had a chat with @SkalskiP - we really want to make sure that if we merge this, it needs to be thorough.

You've done a lot of work already. To make life a bit easier, I'll take this one over, get the tests to the point we're happy. There might be internal discussion yet to come and I don't want to force minor back-and-fourth changes on you 🙂

@SkalskiP: For our records, here's some visuals.
https://colab.research.google.com/drive/179sq8joJ-7JSYMqYNBQlMIPYEClq1PDi?usp=sharing

My plan is to visualise the multi-detection cases too, check if those are needed.
Moving forward, given we want to be thorough, if we're evaluating trickier tests, let's ask for visuals too.

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 1, 2024

I've added additional tests but found a case where LineZone does not work.
Crossing the line while moving from outside to inside the limits, or vice versa, does not count as a crossing.

This is an issue for large jumps or short lines.

I did not test cases with multiple lines present, for I don't see how they would interact.

Happy to merge this, ahead of the vectorized solution.

@LinasKo LinasKo mentioned this pull request Jun 1, 2024
1 task
@tc360950
Copy link
Contributor Author

tc360950 commented Jun 3, 2024

I've added additional tests but found a case where LineZone does not work. Crossing the line while moving from outside to inside the limits, or vice versa, does not count as a crossing.

This is an issue for large jumps or short lines.

@LinasKo Isn't it an intended result? LineZone is "dumb" in the sense that it does not look at trajectories, there's no correlation between subsequent positions of an object.
For instance:
If an objects appears "in limits" on the left of the line, then goes around it and lands "in limits" on the right hand side it will be counted as crossing.

@LinasKo
Copy link
Collaborator

LinasKo commented Jun 3, 2024

@tc360950 potentially intended, potentially not. If we assume small time increments, we're also in many cases assuming direct motion.

I can imagine an algo where we check for whether each anchor displacement line crosses the LineZone (well, only a line in this case), which would work for motions coming from outside the limits. I think it's more intuitive for the users as well.

However, that's a long-term thought. For now, we're 100% sticking with your vectorized solution.

@LinasKo LinasKo requested a review from SkalskiP June 5, 2024 08:32
@LinasKo
Copy link
Collaborator

LinasKo commented Jun 5, 2024

Hi @SkalskiP,

I believe this is ready to merge. When you have time, please have a look.

@SkalskiP SkalskiP merged commit 111d4c6 into roboflow:develop Jun 5, 2024
9 checks passed
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