-
Notifications
You must be signed in to change notification settings - Fork 17
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
Scale #5
base: master
Are you sure you want to change the base?
Scale #5
Conversation
Hello @pathway, thank you for this new pull request. Improving the efficiency of the open list (by using other data structures like \SplPriorityQueue or \SplMinHeap) is actually one of the things I had in my TODO list, but I never implemented it, so I'm really glad you did it. What I'm going to do is to copy my TODO list from my local computer to the GitHub's issue tracker, so everyone can be aware of the pending tasks. And what I absolutely love is that benchmark you did. That proves that your solution is better than the current implementation, so we should definitely incorporate your changes. However there are a couple of issues we need to solve first:
I think that probably the easiest solution could be this: I can create a new branch off master called Also, the fact that you had to change |
Please excuse my ignorance, shall i create a new pull request? I do not see how to change the target branch. |
I am not too proud of the NodePriorityQueue code on my branch, and it is quite rough. Im sure you could do a better job on that. If you want, you can take my tests and ignore or completely rewrite a node queue as you wish; my only request is, I would like to see my name+email in writing somewhere that I can point to as a project contributor, whether in repo history from a pull, or in comments or credits file. Thank you. |
I think you are right, you cannot change the target of a pull request. Would you mind to create a new pull request with the same commits, but this time pointing to Regarding your second paragraph, I would love that you appeared in the contributors section. In fact, if you check the commit history, you'll see that your commit is there. I knew that your commit broke the tests, but the reason I merged it into master to then revert it was just because I wanted you to show up in the contributors section. But for some reason you are not there. My guess is that you don't appear in that list because your pull request wasn't directly merged into master (I made some changes to master, then rebased your branch and merge it. I think that could have confused GitHub). However, what I plan to do now is to merge your new pull request into Thanks again for your help. |
@pathway, I have added you to the contributors file. Please feel free to add other personal information like your e-mail or website, or anything you consider relevant 😃 |
Thank you @jmgq you are very kind. |
No worries, and thank you very much for all your help. |
Scale branch includes these changes:
Using hhvm on an old Ubuntu desktop, timings to find solutions of different sized problems are below.
Columns are Goal Distance / Seconds
openList is array
0 0.065372943878174
50 0.57873010635376
100 1.9482820034027
150 5.3835060596466
200 11.145383119583
250 18.085386991501
300 28.876262903214
350 44.296004056931
400 65.069844007492
450 89.366780996323
openList is PriorityQueue:
0 0.13503909111023
50 0.65088295936584
100 1.6774241924286
150 4.1498160362244
200 7.7588210105896
250 11.37260389328
300 16.837527036667
350 23.381567001343
400 31.248064041138
450 38.205113887787
On the largest problem, 2.3x faster using PriorityQueue.
The phpunit tests are passing. However one test now finds a different solution with the same cost (Terrain/StaticExampleTest::testShouldPrintSolution3) so I updated the solution to match.
I look forward to your comments.