Fix DFS root-revisit bug in isConnected; add Prufer sequence report#81
Merged
Fix DFS root-revisit bug in isConnected; add Prufer sequence report#81
Conversation
Issue #41: AlgorithmUtils.isConnected never marked the root node as visited before calling dfs, so any neighbor of node 0 would see parent[0]==-1 and re-enter node 0, causing redundant traversal and potential StackOverflowError on large dense graphs. Fix: set parent[0]=0 before the dfs call. Issue #38: Add PruferSequence report extension under basicreports. Repeatedly removes the leaf with the smallest vertex id and appends its neighbor's id to build the n-2 length Prufer sequence of a labeled tree. Returns an error string if the graph is not a tree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review Summary by QodoFix DFS bug and add Prufer sequence report extension
WalkthroughsDescription• Fix DFS root-revisit bug in isConnected by marking root as visited • Add PruferSequence report extension for computing Prüfer sequences • Prevent StackOverflowError on large dense graphs during connectivity check • Validate tree structure and handle edge cases in sequence computation Diagramflowchart LR
A["AlgorithmUtils.isConnected"] -->|"set parent[0]=0"| B["Mark root as visited"]
B -->|"prevents re-entry"| C["Avoid StackOverflow"]
D["PruferSequence extension"] -->|"validates tree"| E["Compute n-2 sequence"]
E -->|"remove leaves"| F["Build Prüfer sequence"]
File Changes1. src/graphtea/extensions/AlgorithmUtils.java
|
Code Review by Qodo
1. Extra blank line in PruferSequence
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+12
to
+13
|
|
||
|
|
There was a problem hiding this comment.
1. Extra blank line in prufersequence 📘 Rule violation ⛯ Reliability
PruferSequence.java contains multiple consecutive blank lines after the import block, which violates the repository Checkstyle configuration and can cause ant checkstyle-strict to fail. This breaks the requirement that newly added Java files comply with Checkstyle 10 rules.
Agent Prompt
## Issue description
`PruferSequence.java` has multiple consecutive empty lines after the import section, which violates Checkstyle (`EmptyLineSeparator` with `allowMultipleEmptyLines=false`) and may fail `ant checkstyle-strict`.
## Issue Context
This is a newly added Java source file and must comply with the repository Checkstyle configuration.
## Fix Focus Areas
- src/graphtea/extensions/reports/basicreports/PruferSequence.java[8-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+64
| // degree[i] mirrors the current degree of vertex with id=i | ||
| int[] degree = new int[n]; | ||
| Vertex[] vertices = g.getVertexArray(); | ||
| for (Vertex v : vertices) { | ||
| degree[v.getId()] = g.getDegree(v); | ||
| } | ||
|
|
||
| int[] sequence = new int[n - 2]; | ||
| boolean[] removed = new boolean[n]; | ||
|
|
||
| for (int step = 0; step < n - 2; step++) { | ||
| // find the leaf with the smallest id | ||
| int leaf = -1; | ||
| for (int i = 0; i < n; i++) { | ||
| if (!removed[i] && degree[i] == 1) { | ||
| leaf = i; | ||
| break; | ||
| } | ||
| } | ||
| if (leaf == -1) { | ||
| return "Graph is not a tree (cycle detected)"; | ||
| } | ||
|
|
||
| // find the unique neighbor of this leaf | ||
| Vertex leafVertex = g.getVertex(leaf); | ||
| int neighbor = -1; | ||
| for (Vertex nb : g.getNeighbors(leafVertex)) { | ||
| if (!removed[nb.getId()]) { | ||
| neighbor = nb.getId(); | ||
| break; |
There was a problem hiding this comment.
2. Subgraph id crash 🐞 Bug ✓ Correctness
PruferSequence indexes arrays by Vertex.getId(), but in GraphModel subgraphs (subgraphIndex!=0) the local vertex index is stored separately as a subgraphId, so getId() can be outside 0..n-1 and cause ArrayIndexOutOfBoundsException or incorrect removals.
Agent Prompt
### Issue description
`PruferSequence.calculate` assumes `Vertex.getId()` is a 0..n-1 local index and uses it to index `degree[]`/`removed[]`. In GraphModel subgraphs, local indices are stored as `subgraphId` and `Vertex.getId()` may refer to the supergraph id, leading to `ArrayIndexOutOfBoundsException` and wrong behavior.
### Issue Context
Subgraphs are created via `registerSubgraph()` + `setSubGraphIndex()` and then inserting existing `Vertex` objects. In this mode, `ListGraph.setId` writes `v.setSubgraphId(...)` instead of mutating `v.id`.
### Fix Focus Areas
- src/graphtea/extensions/reports/basicreports/PruferSequence.java[35-75]
- src/graphtea/extensions/algorithms/subgraphs/InducedSubgraphs.java[20-29]
- src/graphtea/library/ListGraph.java[141-148]
- src/graphtea/library/BaseGraph.java[402-408]
- src/graphtea/graph/graph/GraphModel.java[529-536]
### Implementation sketch
- Use `Vertex[] verts = g.getVertexArray();`
- Build `IdentityHashMap<Vertex,Integer> idx` mapping each `verts[i]` to `i`.
- Maintain `degree[i] = g.getDegree(verts[i])` and `removed[i]`.
- When iterating neighbors, translate `nb` to `nbIdx = idx.get(nb)` and use `removed[nbIdx]`/`degree[nbIdx]--`.
- Store output label separately (e.g., `nb.getId()` or `verts[nbIdx].getId()`) rather than array index.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+21
to
+40
| * The graph must be an unrooted labeled tree (connected, undirected, n-1 edges). | ||
| */ | ||
| @CommandAttitude(name = "prufer_sequence", abbreviation = "_ps") | ||
| public class PruferSequence implements GraphReportExtension<String> { | ||
|
|
||
| public String calculate(GraphModel g) { | ||
| int n = g.getVerticesCount(); | ||
| if (n < 2) { | ||
| return "Graph must have at least 2 vertices"; | ||
| } | ||
| if (g.getEdgesCount() != n - 1) { | ||
| return "Graph is not a tree (edge count must equal n-1)"; | ||
| } | ||
|
|
||
| // degree[i] mirrors the current degree of vertex with id=i | ||
| int[] degree = new int[n]; | ||
| Vertex[] vertices = g.getVertexArray(); | ||
| for (Vertex v : vertices) { | ||
| degree[v.getId()] = g.getDegree(v); | ||
| } |
There was a problem hiding this comment.
3. Directed tree accepted 🐞 Bug ✓ Correctness
PruferSequence claims the input must be an undirected tree but never rejects directed graphs; on directed graphs it mixes total degree (in+out) with neighbors derived from outgoing edges only, producing incorrect sequences or misleading errors.
Agent Prompt
### Issue description
`PruferSequence.calculate` is documented for undirected trees but will run on directed graphs. In directed graphs, degree and neighbor iteration semantics do not align with the Prüfer algorithm, so results are wrong.
### Issue Context
- `BaseGraph.getDegree()` counts in+out for directed graphs.
- `getNeighbors()` depends on `lightEdgeIterator(vertex)` which, in `ListGraph`, yields outgoing edges only when `directed==true`.
### Fix Focus Areas
- src/graphtea/extensions/reports/basicreports/PruferSequence.java[26-40]
- src/graphtea/library/BaseGraph.java[433-456]
- src/graphtea/library/ListGraph.java[375-399]
### Suggested change
- Early in `calculate(GraphModel g)` add:
- `if (g.isDirected()) return "Graph is not a tree (graph must be undirected)";`
- (Optional but aligns with the docstring) `if (!AlgorithmUtils.isConnected(g)) return "Graph is not a tree (disconnected)";`
ⓘ 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
1 participant
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.
Issue #41: AlgorithmUtils.isConnected never marked the root node as visited before calling dfs, so any neighbor of node 0 would see parent[0]==-1 and re-enter node 0, causing redundant traversal and potential StackOverflowError on large dense graphs. Fix: set parent[0]=0 before the dfs call.
Issue #38: Add PruferSequence report extension under basicreports. Repeatedly removes the leaf with the smallest vertex id and appends its neighbor's id to build the n-2 length Prufer sequence of a labeled tree. Returns an error string if the graph is not a tree.