Conversation
…, banana tree positioning (#15) - Dijkstra: Fix prev.set(edge.source.getId(), edge.target) → prev.set(target.getId(), vMin) to correctly track predecessors in shortest path tree - Complement: Skip self-loops, remove undirected edges in both directions since removeAllEdges only removes source→target direction - BananaTree: Fix skip condition (i+k/2)%n → (i*k/n+k/2)%k to correctly calculate which leaf position points toward root Co-authored-by: rostam <1497363+rostam@users.noreply.github.com> Agent-Logs-Url: https://github.com/rostam/GraphTea/sessions/3b924362-279a-41f3-9237-c722508d2703
Copilot created this pull request from a session on behalf of
rostam
March 25, 2026 19:33
View session
Review Summary by QodoFix Dijkstra, complement graph, and banana tree bugs
WalkthroughsDescription• Fix Dijkstra predecessor tracking in shortest path tree - Changed prev.set(edge.source.getId(), edge.target) to prev.set(target.getId(), vMin) - Correctly tracks predecessors instead of edge endpoints for undirected graphs • Fix complement graph edge removal for undirected graphs - Skip self-loops when processing vertex pairs - Remove edges in both directions since removeAllEdges only removes source→target • Fix banana tree leaf positioning calculation - Changed skip condition from (i + k/2) % n to (i * k / n + k / 2) % k - Correctly identifies leaf position pointing toward root for each star • Update Dijkstra tests to verify predecessor semantics - Changed assertions from successor to predecessor chain validation - Added null checks and descriptive assertion messages Diagramflowchart LR
A["Dijkstra Algorithm"] -->|Fix predecessor tracking| B["Correct shortest path tree"]
C["Complement Graph"] -->|Skip self-loops & remove both directions| D["Correct edge removal"]
E["Banana Tree Generator"] -->|Fix leaf position formula| F["Correct positioning"]
B -->|Update tests| G["Predecessor semantics validation"]
File Changes1. src/graphtea/extensions/algorithms/shortestpath/algs/Dijkstra.java
|
Code Review by Qodo
1. if without braces
|
rostam
approved these changes
Mar 25, 2026
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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; |
There was a problem hiding this comment.
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
| target.setColor(5); | ||
| Q.add(target); | ||
| prev.set(edge.source.getId(), edge.target); | ||
| prev.set(target.getId(), vMin); |
There was a problem hiding this comment.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses three long-standing bugs reported in issues #53/#70, #48, and #15.
Dijkstra shortest path returns wrong results (#53, #70)
prev.set(edge.source.getId(), edge.target)uses the edge's stored source/target fields, which don't correspond to traversal direction in undirected graphs. Changed to standard predecessor tracking:Tests updated from (incorrect) successor semantics to predecessor semantics, consistent with how
Johnson.javaalready consumes the result.Complement selection doesn't delete existing edges (#48)
Two bugs in
MakeSelectionComplementGraph.doEdgeOperation():ListGraph.removeAllEdges(source, target)only removes the source→target direction. For undirected graphs, the reverse must also be removed.v1 == v2were not skipped, so complement incorrectly added self-loops.Banana tree leaf positioning broken when k ≠ n (#15)
The skip condition determining which leaf position overlaps the root edge used
(i + k/2) % n— modulon(star count) instead ofk(leaf count), and didn't scale the star index. Fixed to(i * k / n + k / 2) % kwhich correctly identifies the leaf position pointing toward root for each star.