Committed by
Gerrit Code Review
Fixed an infinite loop bug in Suurballe graph path search.
Triggered by three edges between two vertices. More work needed to address the remaining issues in the class implementation. Change-Id: Ice1fa85f1b9213680e063e362db4d734d212f6f0
Showing
2 changed files
with
136 additions
and
86 deletions
... | @@ -279,9 +279,12 @@ public abstract class AbstractGraphPathSearch<V extends Vertex, E extends Edge<V | ... | @@ -279,9 +279,12 @@ public abstract class AbstractGraphPathSearch<V extends Vertex, E extends Edge<V |
279 | while (edges.hasNext()) { | 279 | while (edges.hasNext()) { |
280 | E edge = edges.next(); | 280 | E edge = edges.next(); |
281 | boolean isLast = !edges.hasNext(); | 281 | boolean isLast = !edges.hasNext(); |
282 | - DefaultMutablePath<V, E> pendingPath = isLast ? path : new DefaultMutablePath<>(path); | 282 | + // Exclude any looping paths |
283 | - pendingPath.insertEdge(edge); | 283 | + if (!isInPath(edge, path)) { |
284 | - frontier.add(pendingPath); | 284 | + DefaultMutablePath<V, E> pendingPath = isLast ? path : new DefaultMutablePath<>(path); |
285 | + pendingPath.insertEdge(edge); | ||
286 | + frontier.add(pendingPath); | ||
287 | + } | ||
285 | } | 288 | } |
286 | } | 289 | } |
287 | } | 290 | } |
... | @@ -291,6 +294,18 @@ public abstract class AbstractGraphPathSearch<V extends Vertex, E extends Edge<V | ... | @@ -291,6 +294,18 @@ public abstract class AbstractGraphPathSearch<V extends Vertex, E extends Edge<V |
291 | } | 294 | } |
292 | } | 295 | } |
293 | 296 | ||
297 | + /** | ||
298 | + * Indicates whether or not the specified edge source is already visited | ||
299 | + * in the specified path. | ||
300 | + * | ||
301 | + * @param edge edge to test | ||
302 | + * @param path path to test | ||
303 | + * @return true if the edge.src() is a vertex in the path already | ||
304 | + */ | ||
305 | + private boolean isInPath(E edge, DefaultMutablePath<V, E> path) { | ||
306 | + return path.edges().stream().anyMatch(e -> edge.src().equals(e.dst())); | ||
307 | + } | ||
308 | + | ||
294 | // Returns the first vertex of the specified path. This is either the source | 309 | // Returns the first vertex of the specified path. This is either the source |
295 | // of the first edge or, if there are no edges yet, the given destination. | 310 | // of the first edge or, if there are no edges yet, the given destination. |
296 | private V firstVertex(Path<V, E> path, V dst) { | 311 | private V firstVertex(Path<V, E> path, V dst) { | ... | ... |
... | @@ -16,6 +16,8 @@ | ... | @@ -16,6 +16,8 @@ |
16 | 16 | ||
17 | package org.onlab.graph; | 17 | package org.onlab.graph; |
18 | 18 | ||
19 | +import com.google.common.collect.Sets; | ||
20 | + | ||
19 | import java.util.ArrayList; | 21 | import java.util.ArrayList; |
20 | import java.util.Set; | 22 | import java.util.Set; |
21 | import java.util.List; | 23 | import java.util.List; |
... | @@ -34,13 +36,20 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D | ... | @@ -34,13 +36,20 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D |
34 | @Override | 36 | @Override |
35 | public Result<V, E> search(Graph<V, E> graph, V src, V dst, | 37 | public Result<V, E> search(Graph<V, E> graph, V src, V dst, |
36 | EdgeWeight<V, E> weight, int maxPaths) { | 38 | EdgeWeight<V, E> weight, int maxPaths) { |
39 | + // FIXME: This method needs to be refactored as it is difficult to follow and debug. | ||
40 | + | ||
41 | + // FIXME: There is a defect here triggered by 3+ edges between the same vertices, | ||
42 | + // which makes an attempt to produce looping paths. Protection against | ||
43 | + // this was added to AbstractGraphPathSearch, but the root issue remains here. | ||
37 | 44 | ||
45 | + // FIXME: There is a defect here where not all paths are truly disjoint. | ||
46 | + // This class needs to filter its own results to make sure that the | ||
47 | + // paths are indeed disjoint. Temporary fix for this is provided, but | ||
48 | + // the issue needs to be addressed through refactoring. | ||
38 | if (weight == null) { | 49 | if (weight == null) { |
39 | weight = edge -> 1; | 50 | weight = edge -> 1; |
40 | } | 51 | } |
41 | 52 | ||
42 | - List<DisjointPathPair<V, E>> dpps = new ArrayList<>(); | ||
43 | - | ||
44 | final EdgeWeight weightf = weight; | 53 | final EdgeWeight weightf = weight; |
45 | DefaultResult firstDijkstraS = (DefaultResult) super.search(graph, src, dst, weight, ALL_PATHS); | 54 | DefaultResult firstDijkstraS = (DefaultResult) super.search(graph, src, dst, weight, ALL_PATHS); |
46 | DefaultResult firstDijkstra = (DefaultResult) super.search(graph, src, null, weight, ALL_PATHS); | 55 | DefaultResult firstDijkstra = (DefaultResult) super.search(graph, src, null, weight, ALL_PATHS); |
... | @@ -50,49 +59,37 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D | ... | @@ -50,49 +59,37 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D |
50 | if (firstDijkstraS.paths().size() == 0) { | 59 | if (firstDijkstraS.paths().size() == 0) { |
51 | return firstDijkstraS; | 60 | return firstDijkstraS; |
52 | } | 61 | } |
62 | + | ||
63 | + DisjointPathResult result = new DisjointPathResult(firstDijkstra, src, dst, maxPaths); | ||
64 | + | ||
53 | for (Path p: firstDijkstraS.paths()) { | 65 | for (Path p: firstDijkstraS.paths()) { |
54 | shortPath = p; | 66 | shortPath = p; |
55 | //transforms the graph so tree edges have 0 weight | 67 | //transforms the graph so tree edges have 0 weight |
56 | - EdgeWeight<V, Edge<V>> modified = edge -> { | 68 | + EdgeWeight<V, E> modified = edge -> |
57 | - if (classE().isInstance(edge)) { | 69 | + edge instanceof ReverseEdge ? 0 : |
58 | - return weightf.weight((E) (edge)) + firstDijkstra.cost(edge.src()) | 70 | + weightf.weight(edge) + firstDijkstra.cost(edge.src()) - firstDijkstra.cost(edge.dst()); |
59 | - - firstDijkstra.cost(edge.dst()); | ||
60 | - } | ||
61 | - return 0; | ||
62 | - }; | ||
63 | EdgeWeight<V, E> modified2 = edge -> | 71 | EdgeWeight<V, E> modified2 = edge -> |
64 | weightf.weight(edge) + firstDijkstra.cost(edge.src()) - firstDijkstra.cost(edge.dst()); | 72 | weightf.weight(edge) + firstDijkstra.cost(edge.src()) - firstDijkstra.cost(edge.dst()); |
65 | 73 | ||
66 | //create a residual graph g' by removing all src vertices and reversing 0 length path edges | 74 | //create a residual graph g' by removing all src vertices and reversing 0 length path edges |
67 | - MutableGraph<V, Edge<V>> gt = mutableCopy(graph); | 75 | + MutableGraph<V, E> gt = mutableCopy(graph); |
68 | 76 | ||
69 | - Map<Edge<V>, E> revToEdge = new HashMap<>(); | 77 | + Map<E, E> revToEdge = new HashMap<>(); |
70 | graph.getEdgesTo(src).forEach(gt::removeEdge); | 78 | graph.getEdgesTo(src).forEach(gt::removeEdge); |
71 | for (E edge: shortPath.edges()) { | 79 | for (E edge: shortPath.edges()) { |
72 | gt.removeEdge(edge); | 80 | gt.removeEdge(edge); |
73 | - Edge<V> reverse = new Edge<V>() { | 81 | + Edge<V> reverse = new ReverseEdge<V>(edge); |
74 | - final Edge<V> orig = edge; | 82 | + revToEdge.put((E) reverse, edge); |
75 | - public V src() { | 83 | + gt.addEdge((E) reverse); |
76 | - return orig.dst(); | ||
77 | - } | ||
78 | - public V dst() { | ||
79 | - return orig.src(); | ||
80 | - } | ||
81 | - public String toString() { | ||
82 | - return "ReversedEdge " + "src=" + src() + " dst=" + dst(); | ||
83 | - } | ||
84 | - }; | ||
85 | - revToEdge.put(reverse, edge); | ||
86 | - gt.addEdge(reverse); | ||
87 | } | 84 | } |
88 | 85 | ||
89 | //rerun dijkstra on the temporary graph to get a second path | 86 | //rerun dijkstra on the temporary graph to get a second path |
90 | - Result<V, Edge<V>> secondDijkstra; | 87 | + Result<V, E> secondDijkstra = new DijkstraGraphSearch<V, E>() |
91 | - secondDijkstra = new DijkstraGraphSearch<V, Edge<V>>().search(gt, src, dst, modified, ALL_PATHS); | 88 | + .search(gt, src, dst, modified, ALL_PATHS); |
92 | 89 | ||
93 | - Path<V, Edge<V>> residualShortPath = null; | 90 | + Path<V, E> residualShortPath = null; |
94 | if (secondDijkstra.paths().size() == 0) { | 91 | if (secondDijkstra.paths().size() == 0) { |
95 | - dpps.add(new DisjointPathPair<V, E>(shortPath, null)); | 92 | + result.dpps.add(new DisjointPathPair<>(shortPath, null)); |
96 | continue; | 93 | continue; |
97 | } | 94 | } |
98 | 95 | ||
... | @@ -109,85 +106,123 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D | ... | @@ -109,85 +106,123 @@ public class SuurballeGraphSearch<V extends Vertex, E extends Edge<V>> extends D |
109 | 106 | ||
110 | if (residualShortPath != null) { | 107 | if (residualShortPath != null) { |
111 | for (Edge<V> edge: residualShortPath.edges()) { | 108 | for (Edge<V> edge: residualShortPath.edges()) { |
112 | - if (classE().isInstance(edge)) { | 109 | + if (edge instanceof ReverseEdge) { |
113 | - roundTrip.addEdge((E) edge); | ||
114 | - } else { | ||
115 | roundTrip.removeEdge(revToEdge.get(edge)); | 110 | roundTrip.removeEdge(revToEdge.get(edge)); |
111 | + } else { | ||
112 | + roundTrip.addEdge((E) edge); | ||
116 | } | 113 | } |
117 | } | 114 | } |
118 | } | 115 | } |
119 | //Actually build the final result | 116 | //Actually build the final result |
120 | DefaultResult lastSearch = (DefaultResult) super.search(roundTrip, src, dst, weight, ALL_PATHS); | 117 | DefaultResult lastSearch = (DefaultResult) super.search(roundTrip, src, dst, weight, ALL_PATHS); |
121 | - Path<V, E> path1 = lastSearch.paths().iterator().next(); | 118 | + Path<V, E> primary = lastSearch.paths().iterator().next(); |
122 | - path1.edges().forEach(roundTrip::removeEdge); | 119 | + primary.edges().forEach(roundTrip::removeEdge); |
123 | 120 | ||
124 | - Set<Path<V, E>> bckpaths = super.search(roundTrip, src, dst, weight, ALL_PATHS).paths(); | 121 | + Set<Path<V, E>> backups = super.search(roundTrip, src, dst, weight, ALL_PATHS).paths(); |
125 | - Path<V, E> backup = null; | ||
126 | - if (bckpaths.size() != 0) { | ||
127 | - backup = bckpaths.iterator().next(); | ||
128 | - } | ||
129 | 122 | ||
130 | - dpps.add(new DisjointPathPair<>(path1, backup)); | 123 | + // Find first backup path that does not share any nodes with the primary |
124 | + for (Path<V, E> backup : backups) { | ||
125 | + if (isDisjoint(primary, backup)) { | ||
126 | + result.dpps.add(new DisjointPathPair<>(primary, backup)); | ||
127 | + break; | ||
128 | + } | ||
129 | + } | ||
131 | } | 130 | } |
132 | } | 131 | } |
133 | 132 | ||
134 | - for (int i = dpps.size() - 1; i > 0; i--) { | 133 | + for (int i = result.dpps.size() - 1; i > 0; i--) { |
135 | - if (dpps.get(i).size() <= 1) { | 134 | + if (result.dpps.get(i).size() <= 1) { |
136 | - dpps.remove(i); | 135 | + result.dpps.remove(i); |
137 | } | 136 | } |
138 | } | 137 | } |
139 | 138 | ||
140 | - return new Result<V, E>() { | 139 | + result.buildPaths(); |
141 | - final DefaultResult search = firstDijkstra; | 140 | + return result; |
142 | - | ||
143 | - public V src() { | ||
144 | - return src; | ||
145 | - } | ||
146 | - public V dst() { | ||
147 | - return dst; | ||
148 | - } | ||
149 | - public Set<Path<V, E>> paths() { | ||
150 | - Set<Path<V, E>> pathsD = new HashSet<>(); | ||
151 | - int paths = 0; | ||
152 | - for (DisjointPathPair<V, E> path: dpps) { | ||
153 | - pathsD.add((Path<V, E>) path); | ||
154 | - paths++; | ||
155 | - if (paths == maxPaths) { | ||
156 | - break; | ||
157 | - } | ||
158 | - } | ||
159 | - return pathsD; | ||
160 | - } | ||
161 | - public Map<V, Double> costs() { | ||
162 | - return search.costs(); | ||
163 | - } | ||
164 | - public Map<V, Set<E>> parents() { | ||
165 | - return search.parents(); | ||
166 | - } | ||
167 | - }; | ||
168 | } | 141 | } |
169 | 142 | ||
170 | - private Class<?> clazzV; | 143 | + private boolean isDisjoint(Path<V, E> a, Path<V, E> b) { |
171 | - | 144 | + return Sets.intersection(vertices(a), vertices(b)).isEmpty(); |
172 | - public Class<?> classV() { | ||
173 | - return clazzV; | ||
174 | } | 145 | } |
175 | 146 | ||
176 | - private Class<?> clazzE; | 147 | + private Set<V> vertices(Path<V, E> p) { |
177 | - | 148 | + Set<V> set = new HashSet<>(); |
178 | - public Class<?> classE() { | 149 | + p.edges().forEach(e -> set.add(e.src())); |
179 | - return clazzE; | 150 | + set.remove(p.src()); |
151 | + return set; | ||
180 | } | 152 | } |
153 | + | ||
181 | /** | 154 | /** |
182 | * Creates a mutable copy of an immutable graph. | 155 | * Creates a mutable copy of an immutable graph. |
183 | * | 156 | * |
184 | * @param graph immutable graph | 157 | * @param graph immutable graph |
185 | * @return mutable copy | 158 | * @return mutable copy |
186 | */ | 159 | */ |
187 | - public MutableGraph mutableCopy(Graph<V, E> graph) { | 160 | + private MutableGraph<V, E> mutableCopy(Graph<V, E> graph) { |
188 | - clazzV = graph.getVertexes().iterator().next().getClass(); | 161 | + return new MutableAdjacencyListsGraph<>(graph.getVertexes(), graph.getEdges()); |
189 | - clazzE = graph.getEdges().iterator().next().getClass(); | 162 | + } |
190 | - return new MutableAdjacencyListsGraph<V, E>(graph.getVertexes(), graph.getEdges()); | 163 | + |
164 | + private static final class ReverseEdge<V extends Vertex> extends AbstractEdge<V> { | ||
165 | + private ReverseEdge(Edge<V> edge) { | ||
166 | + super(edge.dst(), edge.src()); | ||
167 | + } | ||
168 | + | ||
169 | + @Override | ||
170 | + public String toString() { | ||
171 | + return "ReversedEdge " + "src=" + src() + " dst=" + dst(); | ||
172 | + } | ||
173 | + } | ||
174 | + | ||
175 | + // Auxiliary result for disjoint path search | ||
176 | + private final class DisjointPathResult implements AbstractGraphPathSearch.Result<V, E> { | ||
177 | + | ||
178 | + private final Result<V, E> searchResult; | ||
179 | + private final V src, dst; | ||
180 | + private final int maxPaths; | ||
181 | + private final List<DisjointPathPair<V, E>> dpps = new ArrayList<>(); | ||
182 | + private final Set<Path<V, E>> disjointPaths = new HashSet<>(); | ||
183 | + | ||
184 | + private DisjointPathResult(Result<V, E> searchResult, V src, V dst, int maxPaths) { | ||
185 | + this.searchResult = searchResult; | ||
186 | + this.src = src; | ||
187 | + this.dst = dst; | ||
188 | + this.maxPaths = maxPaths; | ||
189 | + } | ||
190 | + | ||
191 | + @Override | ||
192 | + public V src() { | ||
193 | + return src; | ||
194 | + } | ||
195 | + | ||
196 | + @Override | ||
197 | + public V dst() { | ||
198 | + return dst; | ||
199 | + } | ||
200 | + | ||
201 | + @Override | ||
202 | + public Set<Path<V, E>> paths() { | ||
203 | + return disjointPaths; | ||
204 | + } | ||
205 | + | ||
206 | + private void buildPaths() { | ||
207 | + int paths = 0; | ||
208 | + for (DisjointPathPair<V, E> path: dpps) { | ||
209 | + disjointPaths.add(path); | ||
210 | + paths++; | ||
211 | + if (paths == maxPaths) { | ||
212 | + break; | ||
213 | + } | ||
214 | + } | ||
215 | + } | ||
216 | + | ||
217 | + @Override | ||
218 | + public Map<V, Set<E>> parents() { | ||
219 | + return searchResult.parents(); | ||
220 | + } | ||
221 | + | ||
222 | + @Override | ||
223 | + public Map<V, Double> costs() { | ||
224 | + return searchResult.costs(); | ||
225 | + } | ||
191 | } | 226 | } |
192 | } | 227 | } |
193 | 228 | ... | ... |
-
Please register or login to post a comment