added changes for https://github.com/hjweide/pyastar2d/issues/29 #35
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @peterchenadded for all this work! I'll try to take a look soon. |
hjweide
reviewed
May 5, 2022
src/cpp/astar.cpp
Outdated
| int dy1 = current_y - goal_y; | ||
| int dx2 = start_x - goal_x; | ||
| int dy2 = start_y - goal_y; | ||
| return std::abs(dx1*dy2 - dx2*dy1); |
There was a problem hiding this comment.
This value could be arbitrarily large based on the size of the grid, right? Does this break the admissibility of the heuristic?
There was a problem hiding this comment.
According to http://theory.stanford.edu/~amitp/GameProgramming/Heuristics.html, the tie breaker logic is still optimal.
Not sure how one could prove otherwise, the very small coefficient number suggests it may not be optimal for all coefficient numbers.
hjweide
reviewed
May 5, 2022
src/cpp/astar.cpp
Outdated
| // add tiebreaker cost | ||
| if (tiebreaker_coefficient > 0) { | ||
| heuristic_cost = heuristic_cost + | ||
| tiebreaker_coefficient/1000.0f * tie_breaker_func(nbrs[i] / w, nbrs[i] % w, |
There was a problem hiding this comment.
Why pass the tiebreaker_coefficient as an int and not just pass this scaled-down float directly?
There was a problem hiding this comment.
Yeah, your right. Will change to float.
Didn't want to pass a new type to c at the time.
There was a problem hiding this comment.
Update it to float. So now it is just
if (tiebreaker_coefficient > 0.0f) {
heuristic_cost = heuristic_cost +
tiebreaker_coefficient * tie_breaker_func(nbrs[i] / w, nbrs[i] % w,
hjweide
reviewed
May 5, 2022
src/pyastar2d/astar_wrapper.py
Outdated
Comment on lines
31
to
33
| allow_diagonal: bool = False, | ||
| heuristic_override: int = 0, | ||
| tiebreaker_coefficient: int = 0) -> Optional[np.ndarray]: |
There was a problem hiding this comment.
I'm wondering if we should instead define a new function for uniform grids with these tiebreaking arguments to make it clear that the solution is no longer strictly a shortest path depending on the choice of arguments.
There was a problem hiding this comment.
By default it is shortest path, if users are overriding the heuristic or tiebreaker they better know what they are doing
Maybe we can just add a comment in the docstring? That shortest path guarantee and maybe others are out of the door when using the two new parameters?
There was a problem hiding this comment.
Added below to docstring:
param float tiebreaker_coefficient: Add tiebreaker to heuristic cost, 0=disable, positive enables it by that amount
Important: Please take care when using heuristic_override and tiebreaker_coefficient, they may not take the shortest path.
|
Let's hold on with this, i've worked out what heuristic i actually need which works a-lot better than the orthogonal_x and orthogonal_y that is there at the moment. The complex tie breaker func will also be removed for just the coefficient as i have seem other algorithms using that. Will update the PR and let you know when ready. |
|
@hjweide, can you please review? Looking to use it on monday with my application. |
Fixed issue where there were bumps
hjweide
requested changes
May 7, 2022
There was a problem hiding this comment.
Thanks @peterchenadded for doing all this work! I left some feedback. My biggest concern is the tiebreak_coefficient.
|
Hi @hjweide, i believe i have completed every recommendation you suggested. Can you check and let me know. In summary it is just support for two new heuristics and nothing more. The tiebreaker_coefficient stuff is completely removed. |
Removed magical numbers
Added heuristic
Fix compiling error on linux
Moved heuristic override to its own method as per suggestion
hjweide
requested changes
May 7, 2022
There was a problem hiding this comment.
Thanks for making these changes, it's looking pretty good. I took a quick look and still have some concerns that I pointed out.
Also, could you please re-run the benchmarking to ensure performance remains the same for the existing implementation?
I did this here the last time I made substantial changes for performance:
Eurecat/astar-gridmap-2d#9
This code should still work for benchmarking:
https://gist.github.com/hjweide/1586715f5a2762e80a917bfc2c827dd1
You can get the files from the repo I linked above.
|
Sure no worries, will run the benchmarking. It has one less conditional to check what heuristic, so I suspect it is very slightly faster but than we are passing more args... Will also make the other changes and get back. |
|
Code changes are made, please review once. Seems it is alittle faster on my pc: Your one: My one: Below is existing master code without any of my changes |
|
Ran a few times and the result was always more than what you had. Made below two additional changes:
Straight after the change noticed the benchmarks were consistently better than before. |
|
Also below are the results for orthogonal x and y x: y: |
|
Thanks for running the benchmarks! The results I linked were from my old
laptop that was almost ten years old, so it makes sense that you’re seeing
faster times. I’ll take a closer look when I get home, but for now, do you
know which of those two changes you made contributes most to the
improvement?
…On Sun, May 8, 2022 at 10:36 peterchenadded ***@***.***> wrote:
Also below are the results for orthogonal x and y
x:
----------------------------------------------------------------------------------- benchmark: 3 tests -----------------------------------------------------------------------------------
Name (time in ms) Min Max Mean StdDev Median IQR Outliers OPS Rounds Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_small 5.9934 (1.0) 8.7435 (1.0) 6.4926 (1.0) 0.6759 (1.0) 6.2597 (1.0) 0.1944 (1.0) 13;17 154.0208 (1.0) 120 1
test_large 130.4186 (21.76) 147.1419 (16.83) 134.7627 (20.76) 6.0778 (8.99) 131.4681 (21.00) 6.9806 (35.90) 1;0 7.4205 (0.05) 8 1
test_smooth_1000 246.1046 (41.06) 250.3633 (28.63) 247.6769 (38.15) 2.0077 (2.97) 246.3962 (39.36) 3.3839 (17.40) 1;0 4.0375 (0.03) 5 1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
y:
----------------------------------------------------------------------------------- benchmark: 3 tests -----------------------------------------------------------------------------------
Name (time in ms) Min Max Mean StdDev Median IQR Outliers OPS Rounds Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_small 5.3225 (1.0) 7.8012 (1.0) 5.6007 (1.0) 0.4463 (1.0) 5.4763 (1.0) 0.1172 (1.0) 10;19 178.5493 (1.0) 184 1
test_smooth_1000 57.9232 (10.88) 64.8103 (8.31) 60.2252 (10.75) 2.2575 (5.06) 59.1165 (10.79) 2.5011 (21.34) 4;0 16.6043 (0.09) 18 1
test_large 124.1422 (23.32) 139.4562 (17.88) 128.1577 (22.88) 5.4054 (12.11) 125.2997 (22.88) 5.9423 (50.70) 1;0 7.8029 (0.04) 8 1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
—
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSN6EBAM2MCWIFPI4SKXEDVI7GOVANCNFSM5VAZBEIA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Seems both helps. Definitely the precalculating goal deltas over the function pointers |
hjweide
requested changes
May 8, 2022
|
Pushed up below branches, you if you want to see the code |
|
@hjweide done, please check and let me know if you would like anything else changed. thanks for supporting this. Also benchmarked the code and no difference whether in another file or not. |
|
Let me know if you also want below changes:
modulus is considered expensive, so we remove them using existing calculation we already did. |
|
Nice idea. Feel free to put up a new PR for this if you like. It looks like the improvement is small but still worth including. I've already merged and published this one because I figured you want to use this tomorrow (Monday). |
|
Yeah improvement is too small, will cancel the PR. |
|
Thanks for all the awesome help and patience, we finally got there! |
|
Happy to help! Thanks for seeing this through. And thanks for your work to get the package published automatically -- that was a big help. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes as discussed at #29