-
Notifications
You must be signed in to change notification settings - Fork 45
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
Crash (infinite loop?) in visualize_qtree #46
Comments
OK, not creating a PR because I don't know how to make this binary search work. Haha 😄 The search is even difficult to describe in words. Ideally what you want is the smallest circle at centre But... you could have arbitrarily many points at any distance and even at the same distance from If your current smallest circle has more than My closest "best guess" is to start small with a radius of say If you get to |
I think another approach to this would be to use a k-nearest-neighbours algorithm instead of using a range query lookup. I don’t know how to do that but I’ll give it a go. 😃 |
Woop! 🎉 OK, got the nearest neighbors algorithm working and it's nice and fast 😄 Did spot an inconsistency with the Rectangle though. The Going to assume it is the latter though as that's what the tests show 😄 Expect, when using Will fix those too 😄 But for now... it's 1am so it's bed time 😴 |
OK... it's still not working and one of the issues is due to the inconsistency of how Rectangle width and height are used. I think I'll make a PR for that first and then work with the rest. 😄 |
Sorry if you've felt like you've been talking in a vacuum! I'd like to hear more about your approach! The binary search can have issues with the outer limits and if it's also hard to understand, I'd be interested in using a different approach! |
I looked into it more and you're exactly right! If you provide a I've created a fix for this problem until we can get a closer look at your search method. How does this sound to you? |
@crhallberg hey, thanks for the reply. For some reason I don't seem to be getting emails from Github so apologies for not replying sooner. That looks like a good fix for now 👍 |
RE the k-nearest neighbours. I think the first thing to fix is the inconsistency with the usage of This causes some issues when trying to dive into the algorithm to adjust it a bit but there are lots of tests that are built working around the inconsistency so it's difficult to unpick it as changing it either way breaks a lot of the tests. 😄 |
For some info... k-nearest-neighbours iterates over the structure of the quad tree building a list of "neighbours" instead of using a range lookup. If it finds a node closer than any of the current "neighbours" then it adds that to the list and then removes the furthest one. It can skip a quadrant (and all of its children) if the quadrant is further away than all of the current neighbours as none of its nodes will be closer than any of the neighbours. I think it's important too to remove the overlap of each sector. Instead of using |
Hey, @oliver-foggin! I'm sorry for being away for so long. I was finally able to fix some permissions errors that was preventing me from merging any pull requests! In review of #51, I wanted to check back in on this. Do you still have your k-nearest-neighbor code? I'd be interested to see the approach, since I think my binary search query method might be a bit hard to grasp for the intent of this library. |
@crhallberg hey, I never got to the point of having the k-nearest-neighbours code working due to the width & height inconsistencies in the repo. The k-nearest-neighbours algorithm was almost working but because it relies on knowing the width and height of each node it was getting confused and so never actually returned the correct values. I had a look into fixing them but got to the point where there were too many tests built on top of them and so they would all need updating. Def happy to have a look at it again. I think the main task first would be to fix the inconsistencies so that "width" and "height" have a definite meaning within the repo. I'm happy to have a look at that too but it will be a fairly big update not functionally, just due to the number of changes required. |
Let me have a play and see if I can come up with something. |
Just linking for completeness... Here's the PR for the fix of the width/height issue. |
Thanks for cleaning up both the inconsistencies ( I'm going to give this a closer look but I think the PR is almost set. I left a couple of comments. |
New use of k-nearest fixes the infinite loop! |
I don't know the cause of this yet but there is a crash in visualize_qtree.
I think it's hitting an infinite loop somewhere but I haven't been able to find where yet. It seems to happen near the top of the canvas when you only have one point near the edge of the circle.
Just adding this as a placeholder. Will continue looking for the infinite loop.
EDIT
OK, found it.
In quadtree.js,
closest
function.If
point
has 0 points with themaxDistance
then, during the binary search theinner
radius will get asymptotically close tomaxDistance
butlimit
is never adjusted and so the search will never end.Creating a PR for it now.
The text was updated successfully, but these errors were encountered: