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

two sided dijkstra #268

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

two sided dijkstra #268

wants to merge 19 commits into from

Conversation

Dolgalad
Copy link

Implemented the Two sided Dijkstra algorithm, still requires documentation and tests

@Dolgalad
Copy link
Author

I added a very simple test, not sure if it is sufficient

@gdalle gdalle added the enhancement New feature or request label Jul 2, 2023
@gdalle
Copy link
Member

gdalle commented Jul 2, 2023

Tests are failing because of #271 which has now been merged. If we merge the JuliaGraphs:master branch of into Dolgalad:bidij this should work

gdalle
gdalle previously approved these changes Jul 2, 2023
Copy link
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

It's a good contribution, thanks! The tests will probably fail because of formatting alone, so be sure to install JuliaFormatter.jl in your global environment (@v1.9) and then to run using JuliaFormatter; format(".") from the root of your fork of Graphs.jl

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
test/shortestpaths/dijkstra.jl Show resolved Hide resolved
end
# keep weight of the best seen path and the midpoint vertex
mu, mid_v = typemax(T), -1

Copy link
Member

Choose a reason for hiding this comment

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

All that white space may not be necessary

src/shortestpaths/dijkstra.jl Outdated Show resolved Hide resolved
src/shortestpaths/dijkstra.jl Outdated Show resolved Hide resolved
uf, ub = dequeue!(Qf), dequeue!(Qb)

for v in outneighbors(g, uf)
relax(uf, v, distmx, dists_f, parents_f, visited_f, Qf)
Copy link
Member

Choose a reason for hiding this comment

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

This relax function clarifies things, would it be hard to add it to the original dijkstra too?

Copy link
Author

Choose a reason for hiding this comment

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

I was hoping you would notice =p I'll make the changes

Copy link
Author

Choose a reason for hiding this comment

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

I have a new version of the relax function that should work for both the dijkstra_shortest_paths and bidijkstra_shortest_path but it is a bit clunky. I am wondering; why the need for the parents and preds structures ? wouldn't the list of predecessors be enough ?
A cosmetic note: should we rename the function the bidijkstra_shortest_paths if it should be able to deal with multiple paths ?

src/shortestpaths/dijkstra.jl Outdated Show resolved Hide resolved
end
ds_f = DijkstraState{T,U}(parents_f, dists_f, preds_f, zeros(nvg), Vector{U}())
ds_b = DijkstraState{T,U}(parents_b, dists_b, preds_b, zeros(nvg), Vector{U}())
path = vcat(enumerate_paths(ds_f, mid_v), reverse(enumerate_paths(ds_b, mid_v)[1:end-1]))
Copy link
Member

Choose a reason for hiding this comment

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

What if there are several paths?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how to deal with such situations, as far as I can tell multiple paths with the same cost can be detected if the condition mu == ls + lt is reached. I'll look into it

Copy link
Author

Choose a reason for hiding this comment

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

Given that bidijkstra_shortest_path only returns src-dst-paths it doesn't make much sense to keep a full list of predecessors, does it ? I am not sure what the most elegant way to deal with these situations, especially when trying to share code with dijkstra_shortest_paths.
I've also noticed that there doesn't seem to be a straightforward way to retrieve multiple paths for a single (src,dst) pair. As far a I can tell the enumerate_paths routine returns a single path per destination. Should this be addressed at some point ?

@gdalle gdalle dismissed their stale review July 2, 2023 20:49

Wrong review decision

Copy link
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

The additional dependencies are not acceptable, and it needs more tests, but it's a solid start!

@Dolgalad Dolgalad requested a review from gdalle July 3, 2023 12:54
@Dolgalad
Copy link
Author

Dolgalad commented Jul 8, 2023

Not sure why tests are failing

@gdalle
Copy link
Member

gdalle commented Jul 9, 2023

I think your modification to Dijkstra somehow broke a test in the centrality part of the package.
My two propositions:

  • revert your modifications to only implement bidirectional Dijkstra without changing the original
  • investigate why you broke centrality

The error comes from the following test:

# test #1405 / #1406
g = GenericGraph(grid([50, 50]))
z = betweenness_centrality(g; normalize=false)
@test maximum(z) < nv(g) * (nv(g) - 1)

and it looks like this:

Betweenness: Test Failed at /home/runner/work/Graphs.jl/Graphs.jl/test/centrality/betweenness.jl:95
  Expression: maximum(z) < nv(g) * (nv(g) - 1)
   Evaluated: 3.193572232997055e10 < 6247500

It is related to the following issue in the old LightGraphs.jl package
sbromberger/LightGraphs.jl#1405

Copy link
Member

@simonschoelly simonschoelly left a comment

Choose a reason for hiding this comment

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

There seems to be some different behavior for the pathcount when using dijkstra_shortest_path. Might be some kind of overflow issue, as I only saw it on large graphs so far.

before:

julia> dijkstra_shortest_paths(grid([50, 50]), 1).pathcounts[50*50]
2.5477612258980867e28

after:

julia> dijkstra_shortest_paths(grid([50, 50]), 1).pathcounts[50*50]
8.581105107791177e17

I think the right solution here should be (2 * 49 \choose 49), i.e

julia> binomial(BigInt(2*49), 49)
25477612258980856902730428600

so apart from some rounding errors, the old solution looks much more correct to me.

@@ -83,7 +83,7 @@ function dijkstra_shortest_paths(
parents = zeros(U, nvg)
visited = zeros(Bool, nvg)

pathcounts = zeros(nvg)
Copy link
Member

Choose a reason for hiding this comment

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

We probably should not change this, it might overflow.

@simonschoelly
Copy link
Member

It also look to me, as if those changes might make the algorithm perform slower than before, but I only checked this single case:
before:

julia> @btime dijkstra_shortest_paths($(grid([50, 50])), 1);
  548.740 μs (81 allocations: 165.11 KiB)

after

julia> @btime dijkstra_shortest_paths($(grid([50, 50])), 1);
  585.437 μs (83 allocations: 184.72 KiB)

@gdalle gdalle self-assigned this Aug 28, 2023
@gdalle
Copy link
Member

gdalle commented Jan 29, 2024

@Dolgalad is this ready for another round of review? sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants