-
-
Notifications
You must be signed in to change notification settings - Fork 630
PNC k shortest simple path (for directed graphs) #40284
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
base: develop
Are you sure you want to change the base?
Conversation
…o this implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to define PathCandidate
in a .h file ? can't it be done in file path_enumeration.pxd
?
src/sage/graphs/path_enumeration.pyx
Outdated
G.delete_edges(G.incoming_edges(source, labels=False)) | ||
G.delete_edges(G.outgoing_edges(target, labels=False)) | ||
|
||
if weight_function is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done by the call to _get_weight_function
. No need to duplicate.
src/sage/graphs/path_enumeration.pyx
Outdated
if self.has_loops() or self.allows_multiple_edges(): | ||
G = self.to_simple(to_undirected=False, keep_label='min', immutable=False) | ||
else: | ||
G = self.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be on the safe side, use self.copy(immutable=False)
src/sage/graphs/path_enumeration.pyx
Outdated
reverse_graph = G.reverse() | ||
dist, successor = shortest_paths(reverse_graph, target, weight_function=reverse_weight_function, | ||
algorithm='Dijkstra_Boost') | ||
cdef set unnecessary_vertices = set(G) - set(dist) # no path to target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always add 2 spaces before a comment at the end of a line, so set(list) # vertices with no path to target
src/sage/graphs/path_enumeration.pyx
Outdated
|
||
# ancestor_idx_dict[v] := the first vertex reachable by edges of | ||
# first shortest path tree from v | ||
ancestor_idx_dict = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ancestor_idx_dict = {v: i for I, v in enumerate(path)}
Because I want to use priority_queue of C++ standard library, the type of PathCandidate must be struct of C++. Thus, I think I need definition of PathCandidate in .h file. However, There may be another way of doing so without .h file. I am looking for it. |
isn't it possible to use a tuple instead of PathCandidate ? or may be |
src/sage/graphs/path_enumeration.pyx
Outdated
@@ -36,6 +36,7 @@ from sage.misc.misc_c import prod | |||
from libcpp.queue cimport priority_queue | |||
from libcpp.pair cimport pair | |||
from sage.rings.integer_ring import ZZ | |||
from sage.graphs.path_enumeration cimport PathCandidate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line. Otherwise it fails to compile.
sage: list(pnc_k_shortest_simple_paths(g, 1, 5, by_weight=True)) | ||
[[1, 3, 5], [1, 2, 5], [1, 4, 5]] | ||
TESTS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add another test to compare the results with Yen, at least the sequence of weights. Paths with the same weight may be reported in different orders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, the key to the paper should be ACN2023
(Al Zoobi, Coudert, Nisse)
something goes wrong in the algorithm. Paths are not reported in increasing order of weight sage: g = DiGraph(graphs.Grid2dGraph(2, 6).relabel(inplace=False))
sage: for u, v in g.edge_iterator(labels=False):
....: g.set_edge_label(u, v, 1)
sage: %time X = [w for w, P in pnc_k_shortest_simple_paths(g, 5, 1, by_weight=True, report_weight=True, labels=True, report_edges=True)]
CPU times: user 1.39 ms, sys: 0 ns, total: 1.39 ms
Wall time: 1.4 ms
sage: print(X)
[4.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 10.0, 10.0, 10.0, 10.0, 8.0] |
src/sage/graphs/path_enumeration.pyx
Outdated
[4.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 6.0, 8.0, 8.0, | ||
8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 8.0, 10.0, 10.0, 10.0, 10.0] | ||
|
||
Same tests as yen_k_shortest_simple_paths:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 spaces less ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done.
You should rebase the branch on the last beta. If you do another commit, you can add the doi to the paper |
It's not fully fixed sage: from sage.graphs.path_enumeration import yen_k_shortest_simple_paths as yen
sage: from sage.graphs.path_enumeration import feng_k_shortest_simple_paths as feng
sage: from sage.graphs.path_enumeration import pnc_k_shortest_simple_paths as pnc
sage: D = graphs.Grid2dGraph(5, 5).relabel(inplace=False).to_directed()
sage: %time len(list(yen(D, 0, 24)))
CPU times: user 14 s, sys: 408 µs, total: 14 s
Wall time: 14.1 s
8512
sage: %time len(list(feng(D, 0, 24)))
CPU times: user 1.23 s, sys: 988 µs, total: 1.23 s
Wall time: 1.24 s
8512
sage: %time len(list(pnc(D, 0, 24)))
CPU times: user 427 ms, sys: 995 µs, total: 428 ms
Wall time: 428 ms
8530 |
Implement PNC k shortest simple path for directed graphs.
I implemented for only directed graphs for simplicity first.
📝 Checklist
⌛ Dependencies
#40248
#40217