-
Notifications
You must be signed in to change notification settings - Fork 3
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
quad tree with points that have a set of line indices as data #43
quad tree with points that have a set of line indices as data #43
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.
Looks good to me. Thanks.
Performance is also improved on my Mac. Even sligthly faster than the old non-quad tree implementation.
def __init__(self, width: float, height: float): | ||
"""Create a LinesQuadTree instance. | ||
|
||
The LinesQuadTree will contain an actual quad tree with the end points of all lines, as well as a hashmap to |
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 end points confused me for a second. Maybe it's better to say with all start and end points of all lines.
I think you meant it the right way, a line (segment) has two end points. But since our implementation has line.start and line.end; it can be confusing.
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.
Yes, good point, I see that it can be confusing.
However, "end point" is a rather standard name for the extremities of a line segment. And our line segments don't really have an inherent direction, so maybe we should reconsider the naming of "line.start" and "line.end", rather than avoid the word "end point" in other contexts such as here...
What do you think? Merge it like this and deal with it later?
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.
Merge it and deal with it later. The implementation is rather explicit. So any confused reader will figure it out quickly.
f6a3bb8
into
feat/exact_line_merging
Code review proposal for #38
I think that some unit tests for the LinesQuadTree class could be really useful, but I did not have time to implement any new tests yet. I'll leave that up to you @redur to either add some unit tests, or to accept this PR as-is already and to leave writing unit tests for later.