Skip to content
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

Stop shortest path returning no path when destination node is reached on suboptimal path #42

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

mmiller-max
Copy link
Contributor

Fixes two issues that result from one bug

  1. If the end node was assessed on a suboptimal path with alt >= dists[v] (for example if the last edge weight was Inf but goal was still reached), then parents was returned from astar but parents[v] could be undefined, meaning that LightOSM errored with the no path exists message
  2. If the end node was reached on a path (alt < dists[v]) then the traversal would return this path, even if a better one existed.

To solve the first, the early return was moved inside the if alt < dists[v] statement, and to solve the second, the additional condition of checking that the goal is the first in the PriorityQueue enables this to be fixed without extra allocations/worsening performance.

We may need to re-run the benchmarks

@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2021

Codecov Report

Merging #42 (b005cdf) into master (f1be1a9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #42   +/-   ##
=======================================
  Coverage   67.10%   67.10%           
=======================================
  Files          11       11           
  Lines         836      836           
=======================================
  Hits          561      561           
  Misses        275      275           
Impacted Files Coverage Δ
src/traversal.jl 61.36% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1be1a9...b005cdf. Read the comment docs.

@mmiller-max
Copy link
Contributor Author

Once #41 is in as I can add some explicit tests

src/traversal.jl Outdated Show resolved Hide resolved
@@ -69,10 +69,9 @@ function astar(g::AbstractGraph{U},
H[v] = alt + heuristic(v, goal)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add description for heuristic usage in the doc string of shortest_path

Copy link
Collaborator

@captchanjack captchanjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmiller-max mmiller-max merged commit aa37ce4 into master Aug 26, 2021
@mmiller-max mmiller-max deleted the mm/shortest-path-fix branch August 26, 2021 14:33
mmiller-max pushed a commit that referenced this pull request Aug 30, 2021
mmiller-max pushed a commit that referenced this pull request Sep 17, 2021
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.

None yet

3 participants