Skip to content

Update k-nearest-neighbors algorithm and added example from @crhallberg #59

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

Merged
merged 6 commits into from
Apr 2, 2021

Conversation

oliver-foggin
Copy link
Contributor

@oliver-foggin oliver-foggin commented Mar 17, 2021

As pointed out by @crhallberg the algorithm I had implemented was not as optimised as it could be and was searching too many points when displayed in the example he wrote.

I have now improved the algorithm and it cuts down how many quads and points are searched by a lot.

The screenshot shown here is searching for the single closest point with no distance limit...

Screen.Recording.2021-03-19.at.10.08.21.mov

It is still not ideal but that is due to the fact that the tree is not "balanced". Ideally, all the points of the quad tree should be contained in the leaves of the tree (i.e. the very bottom sub trees) with no points left higher up the tree. This allows the k-nearest-neighbors search to eliminate entire subsections of the quad tree just by the frame they have and only then start searching for points.

Perhaps this is something we could re-address. There are some nice PRs open that address this and could be good to merge in with some fixes.

@crhallberg crhallberg changed the title Update k-nearest-neighbors algorithm and added example from @crhalberg Update k-nearest-neighbors algorithm and added example from @crhallberg Mar 17, 2021
@oliver-foggin
Copy link
Contributor Author

Oops, sorry for the typo in your name 😅

@oliver-foggin
Copy link
Contributor Author

Hmm... thinking about this I can improve it a bit more. Just trying to get this working now. :D

@oliver-foggin
Copy link
Contributor Author

OK 💥

I've fixed it. Just putting the update in now. 👍🏻

@crhallberg just wait. You'll love it 😄

@oliver-foggin
Copy link
Contributor Author

oliver-foggin commented Mar 19, 2021

Latest update of the kNearest algorithm. It is now properly depth first and goes to the quad nearest the search point first and works outwards from there. I was thinking about your comment @crhallberg about it not being depth first and couldn't get my head around it for a while but woke up this morning with new ideas which worked perfectly 👍🏻

Screen.Recording.2021-03-19.at.10.08.21.mov

In the previous iteration I was searching the points of a tree before diving into the children. The main change for this now is that it goes to the children first.

Only after searching the children does it then start search the parents. And only then if they could contain points closer than those already found.

Much better implementation of this now. It's amazing what sleeping on a problem can do. 😄

@oliver-foggin
Copy link
Contributor Author

This works even better when rebalancing the tree too. (Although we can keep that for later).

On average when the tree is rebalanced. With 1000 points it will search about 10 points to find the closet one.

@oliver-foggin
Copy link
Contributor Author

Spoiler alert... rebalancing the tree makes the kNearest algorithm so cool...

Screen.Recording.2021-03-19.at.17.23.45.mov

😃

@oliverfoggin
Copy link

@crhallberg hey, this should be good to go in now unless there is anything you'd like me to update.

Sorry for the messed up first version. I'm still learning myself so this has been a good challenge. 😆

@crhallberg
Copy link
Collaborator

Hey! Everything does look good. I just wanted to run a few more manual tests with finding multiple closest points but haven't found the time. Thanks for the quick fix on this!

@oliverfoggin
Copy link

Hey! Everything does look good. I just wanted to run a few more manual tests with finding multiple closest points but haven't found the time. Thanks for the quick fix on this!

Cool! Happy to answer any questions.

The structure of the tree causes some points to be tested that you would think would not be tested. And with multiple closest it highlights that even more.

However, the rebalance PR that I have in the works addresses this. With the rebalance the closest algorithm is much much better. (Without having to change it)

@oliver-foggin
Copy link
Contributor Author

oliver-foggin commented Mar 25, 2021

I previously removed the sort of the points before iterating over them. This isn't ideal as it means the furthestDistance depends on the order in which the points are iterated.

If the closest point is iterated first then fine. But if the furthest point goes first then it will be the furthest distance. So even though we've found the closest point and no other rects are closer it's actually using the furthest point in that rect as the furthestDistance which will mean that additional rects are searched when it isn't necessary to.

Because of this I have now put back the sort of points before iterating along them.

The sketch example is unchanged as I didn't remove it from there.

@crhallberg crhallberg merged commit 77bc75d into CodingTrain:main Apr 2, 2021
@crhallberg
Copy link
Collaborator

I might add a few comments later but this all looks good! Thanks for the fix!

@oliverfoggin
Copy link

Great thanks 😊

@oliverfoggin
Copy link

I might add a few comments later but this all looks good! Thanks for the fix!

I def agree that there is some improvement to be made.

I've got another branch where I rebalance the tree. I'll show you that one shortly.

It massively improves the search but there's still something not quite perfect with it.

I need to spend more time with it.

It's def good enough now but can be improved 👍🏻

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.

3 participants