Skip to content

Fix Dijkstra predecessor tracking, complement edge removal, and banana tree positioning #83

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.

Sign up for GitHub

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

Merged
rostam merged 1 commit into from
Mar 25, 2026
+24 −14
Merged

Fix Dijkstra predecessor tracking, complement edge removal, and banana tree positioning #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/graphtea/extensions/algorithms/shortestpath/algs/Dijkstra.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public int compare(Vertex o1, Vertex o2) {
target.setMark(true);
target.setColor(5);
Q.add(target);
prev.set(edge.source.getId(), edge.target);
prev.set(target.getId(), vMin);
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 25, 2026

Choose a reason for hiding this comment

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

Action required

2. Directed edges traversed backwards 🐞 Bug ✓ Correctness

Dijkstra.getShortestPath() iterates *all incident edges* via graph.edgeIterator(vMin) and then
picks the “other endpoint”, which causes incoming edges in directed graphs to be relaxed in reverse
direction and can produce paths that do not exist in the directed graph.
Agent Prompt 
### Issue description
`algs/Dijkstra#getShortestPath` currently traverses directed graphs as undirected by iterating all incident edges (`edgeIterator(vMin)`) and selecting the opposite endpoint, which can relax incoming edges backwards.

### Issue Context
`ListGraph.edgeIterator(v)` for directed graphs returns both outgoing and incoming edges, so Dijkstra must explicitly restrict traversal to outgoing edges (or filter by `edge.source == vMin`) when `graph.isDirected()`.

### Fix Focus Areas
- src/graphtea/extensions/algorithms/shortestpath/algs/Dijkstra.java[84-103]
- src/graphtea/library/ListGraph.java[220-244]

### Implementation notes
- Prefer `graph.edgeIterator(vMin, true)` (source/outgoing) for directed graphs and use `target = edge.target`.
- Keep the current undirected behavior for `!graph.isDirected()` (incident edges + opposite endpoint).
- Ensure `prev.set(target.getId(), vMin)` is only applied for valid traversal direction in directed graphs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/graphtea/extensions/generators/BananaTreeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ private static GraphModel generateBananaTree(int n, int k) {

GPoint[] sR = PositionGenerators.circle(1000, center.x, center.y, k);
for (int j = 0; j < k; j++) {
if(j == (i + k/2)%n) continue;
if (j == (i * k / n + k / 2) % k) continue;
Copy link
Copy Markdown

@qodo-code-review qodo-code-review bot Mar 25, 2026

Choose a reason for hiding this comment

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

Action required

1. if without braces 📘 Rule violation ⚙ Maintainability

The modified if statement uses a single-line body without braces, which violates the NeedBraces
requirement and increases risk of future control-flow mistakes during edits.
Agent Prompt 
## Issue description
A modified `if` statement omits braces (NeedBraces violation).

## Issue Context
Single-line control statements without braces are disallowed by the compliance checklist and can lead to accidental logic changes when additional lines are added later.

## Fix Focus Areas
- src/graphtea/extensions/generators/BananaTreeGenerator.java[79-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

curv = new Vertex();
g.insertVertex(curv);
setloc(curv, sR[j]);
Expand Down
14 changes: 10 additions & 4 deletions src/graphtea/plugins/main/select/MakeSelectionComplementGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,20 @@ public void action(GraphData gd) {

protected void doEdgeOperation(GraphModel g, HashSet<Vertex> v) {
boolean directed = g.isDirected();

for (Vertex v1 : v) {
for (Vertex v2 : v) {
if (!directed)
if (v1.getId() < v2.getId())
continue;
if (v1 == v2) {
continue;
}
if (!directed && v1.getId() < v2.getId()) {
continue;
}
if (g.isEdge(v1, v2)) {
g.removeAllEdges(v1, v2);
if (!directed) {
g.removeAllEdges(v2, v1);
}
} else {
g.insertEdge(new Edge(v1, v2));
}
Expand Down
20 changes: 12 additions & 8 deletions test/AlgorithmsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class AlgorithmsTest {
// -------------------------------------------------------------------------

/** Build a simple directed weighted path 0→1(w=1)→2(w=2)→3(w=3) and verify
* that Dijkstra from vertex 0 encodes the correct successor chain. */
* that Dijkstra from vertex 0 encodes the correct predecessor chain. */
@Test
public void testDijkstraLinearPath() throws Exception {
GraphModel g = new GraphModel(true);
Expand All @@ -40,10 +40,11 @@ public void testDijkstraLinearPath() throws Exception {
List<Vertex> prev = new Dijkstra().getShortestPath(g, v0);

assertNotNull(prev);
// prev[i] is the successor of vertex i on the shortest path from v0
assertEquals(v1, prev.get(v0.getId()));
assertEquals(v2, prev.get(v1.getId()));
assertEquals(v3, prev.get(v2.getId()));
// prev[i] is the predecessor of vertex i on the shortest path from v0
assertNull(prev.get(v0.getId()), "Source vertex has no predecessor");
assertEquals(v0, prev.get(v1.getId()), "Predecessor of v1 is v0");
assertEquals(v1, prev.get(v2.getId()), "Predecessor of v2 is v1");
assertEquals(v2, prev.get(v3.getId()), "Predecessor of v3 is v2");
}

/** Dijkstra on a complete K4 graph should return a prev-array of size 4
Expand Down Expand Up @@ -71,9 +72,12 @@ public void testDijkstraDiamond() throws Exception {

List<Vertex> prev = new Dijkstra().getShortestPath(g, v0);
assertNotNull(prev);
// The path to v1 should go via v2, not directly
assertEquals(v2, prev.get(v0.getId()),
"Dijkstra should prefer short path 0→2 over long path 0→1");
// prev[i] is the predecessor of vertex i on the shortest path from v0
assertNull(prev.get(v0.getId()), "Source vertex has no predecessor");
assertEquals(v0, prev.get(v2.getId()),
"Predecessor of v2 is v0 (direct path 0→2)");
assertEquals(v2, prev.get(v1.getId()),
"Predecessor of v1 is v2 (shortest path 0→2→1)");
}

// -------------------------------------------------------------------------
Expand Down
Loading