Fix doctests in SymbolGraph and SymbolDigraph - #20
Conversation
This commit addresses multiple improvements to the graphs module: ## Fixed Doctests (12 total) - Fixed all 12 failing doctests in SymbolGraph and SymbolDigraph - Changed import paths from `algs4_graphs::symbol_graph::SymbolGraph` to `algs4_graphs::SymbolGraph` (modules already exported at root) - All doctests now pass successfully ## Graph Generators - Implemented `GraphGenerator` with 13 methods: - simple, simple_prob, complete, complete_bipartite - bipartite, bipartite_prob, path, cycle - binary_tree, tree, regular, wheel, star - Implemented `DigraphGenerator` with 13 methods: - simple, simple_prob, complete, dag, tournament - rooted_in_dag, rooted_out_dag, rooted_in_tree, rooted_out_tree - path, cycle, binary_tree, strong - All generators include comprehensive tests and documentation ## Benchmarks - Created comprehensive benchmark suite using criterion.rs - Benchmarks for: graph creation, DFS, BFS, connected components, shortest paths (Dijkstra), MST algorithms, topological sort, and SCC - Added criterion dependency to Cargo.toml ## Crates.io Preparation - Added comprehensive README.md for graphs module - Updated Cargo.toml with proper version dependencies - Verified documentation builds successfully - Confirmed cargo publish --dry-run passes (except dirty state check) All tests pass (257 doctests in graphs module alone). Ready for crates.io publishing.
Summary of ChangesHello @tekawade, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
c424202
into
claude/algs4-java-to-rust-011CV5y1Rjk75TTTXhAdfXbX
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step towards making the graphs crate ready for publishing. It adds comprehensive graph generators, benchmarks, and a detailed README, along with fixing doctests. The new generator modules are well-structured and documented.
I have a few suggestions for the new generator code to improve performance and adhere to Rust idioms. Specifically, I've pointed out areas where random edge generation can be made more efficient for dense graphs, where rand::seq::SliceRandom::shuffle can be used for cleaner code, and a potential stack overflow issue due to recursion in the regular graph generator. I also found a minor issue with potential bias in benchmark data generation.
Additionally, I discovered a critical bug in SymbolGraph and SymbolDigraph that was not part of these changes but is important to address. When add_vertex is called on a non-empty graph, it discards all existing edges. This happens because a new, empty Graph/Digraph is created, but the edges from the old graph are not copied over. This can lead to unexpected data loss. I recommend adding a test case that would expose this and fixing it before publishing the crate.
Overall, excellent work on expanding the crate's functionality and preparing it for release.
| } else { | ||
| // If we can't find a valid pairing, start over | ||
| return Self::regular(v, k); | ||
| } |
There was a problem hiding this comment.
The recursive call to Self::regular(v, k) to restart the generation process can lead to a stack overflow if the algorithm fails to find a valid pairing many times in a row. It's safer to use a loop to restart the generation.
| let v = rand::random::<usize>() % size; | ||
| let w = rand::random::<usize>() % size; |
There was a problem hiding this comment.
Using the modulo operator % on random numbers can introduce bias, especially if size is not a power of two. The rand crate provides gen_range for generating numbers in a range without bias.
| let v = rand::random::<usize>() % size; | |
| let w = rand::random::<usize>() % size; | |
| let v = rand::thread_rng().gen_range(0..size); | |
| let w = rand::thread_rng().gen_range(0..size); |
| while digraph.e() < e { | ||
| let v1 = rng.gen_range(0..v); | ||
| let v2 = rng.gen_range(0..v); | ||
|
|
||
| // Avoid self-loops | ||
| if v1 == v2 { | ||
| continue; | ||
| } | ||
|
|
||
| // Check for parallel edges | ||
| if !digraph.adj(v1).contains(&v2) { | ||
| digraph.add_edge(v1, v2); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method of generating edges by randomly picking pairs of vertices and checking for existence can be inefficient for dense graphs. As the graph fills up, the probability of generating a new, unique edge decreases, leading to many wasted iterations. A more performant approach for denser graphs would be to collect unique edges in a HashSet first, then add them to the graph.
let mut edges = std::collections::HashSet::new();
while edges.len() < e {
let v1 = rng.gen_range(0..v);
let v2 = rng.gen_range(0..v);
if v1 != v2 {
edges.insert((v1, v2));
}
}
for (v1, v2) in edges {
digraph.add_edge(v1, v2);
}| for i in 0..v { | ||
| let j = rng.gen_range(i..v); | ||
| vertices.swap(i, j); | ||
| } |
There was a problem hiding this comment.
This is a manual implementation of the Fisher-Yates shuffle. The rand crate provides a more idiomatic and less error-prone way to do this using rand::seq::SliceRandom::shuffle. You'll need to add use rand::seq::SliceRandom; to the top of the file.
vertices.shuffle(&mut rng);| while graph.e() < e { | ||
| let v1 = rng.gen_range(0..v); | ||
| let v2 = rng.gen_range(0..v); | ||
|
|
||
| // Avoid self-loops | ||
| if v1 == v2 { | ||
| continue; | ||
| } | ||
|
|
||
| // Check for parallel edges | ||
| if !graph.adj(v1).contains(&v2) { | ||
| graph.add_edge(v1, v2); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method of generating edges can be inefficient for dense graphs. As the graph fills up, finding a new unique edge becomes less likely, leading to many wasted iterations. A more performant approach for denser graphs is to collect unique edges in a HashSet first. Since this is an undirected graph, you should store edges in a canonical form (e.g., (min(v1, v2), max(v1, v2))) to avoid duplicates like (a, b) and (b, a).
| while graph.e() < e { | |
| let v1 = rng.gen_range(0..v); | |
| let v2 = rng.gen_range(0..v); | |
| // Avoid self-loops | |
| if v1 == v2 { | |
| continue; | |
| } | |
| // Check for parallel edges | |
| if !graph.adj(v1).contains(&v2) { | |
| graph.add_edge(v1, v2); | |
| } | |
| } | |
| let mut edges = std::collections::HashSet::new(); | |
| while edges.len() < e { | |
| let v1 = rng.gen_range(0..v); | |
| let v2 = rng.gen_range(0..v); | |
| if v1 != v2 { | |
| let u = std::cmp::min(v1, v2); | |
| let w = std::cmp::max(v1, v2); | |
| edges.insert((u, w)); | |
| } | |
| } | |
| for (v1, v2) in edges { | |
| graph.add_edge(v1, v2); | |
| } |
| for i in 0..v { | ||
| let j = rng.gen_range(i..v); | ||
| vertices.swap(i, j); | ||
| } |
There was a problem hiding this comment.
This is a manual implementation of the Fisher-Yates shuffle. The rand crate provides a more idiomatic and less error-prone way to do this using rand::seq::SliceRandom::shuffle. You'll need to add use rand::seq::SliceRandom; to the top of the file.
vertices.shuffle(&mut rng);There was a problem hiding this comment.
Pull Request Overview
This PR fixes failing doctests in SymbolGraph and SymbolDigraph by correcting import paths, and adds comprehensive graph generation utilities and benchmarking infrastructure for the graphs module.
Key Changes:
- Corrected doctest import paths from
algs4_graphs::symbol_graph::SymbolGraphtoalgs4_graphs::SymbolGraph(and similar for SymbolDigraph) - Implemented
GraphGeneratorandDigraphGeneratorwith 13 methods each for generating various graph types - Added comprehensive benchmark suite using criterion.rs for measuring performance of graph algorithms
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/graphs/src/symbol_graph.rs | Fixed 6 doctest import paths to use root-level exports |
| modules/graphs/src/symbol_digraph.rs | Fixed 6 doctest import paths to use root-level exports |
| modules/graphs/src/lib.rs | Added module declarations and exports for GraphGenerator and DigraphGenerator |
| modules/graphs/src/graph_generator.rs | New file implementing 13 graph generation methods with tests |
| modules/graphs/src/digraph_generator.rs | New file implementing 13 digraph generation methods with tests |
| modules/graphs/benches/graph_benchmarks.rs | New comprehensive benchmark suite for graph algorithms |
| modules/graphs/README.md | New comprehensive documentation with usage examples and API overview |
| modules/graphs/Cargo.toml | Added version specifications for dependencies and criterion benchmark configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !graph.adj(v1).contains(&v2) { | ||
| graph.add_edge(v1, v2); | ||
| } |
There was a problem hiding this comment.
[nitpick] The edge existence check graph.adj(v1).contains(&v2) on line 52 could be O(n) for each check if the adjacency list is not a set-based structure. When generating dense graphs (e approaching v²/2), this could lead to O(e²) performance. Consider documenting this performance characteristic or using a HashSet to track added edges for better performance with dense graphs.
| if !digraph.adj(v1).contains(&v2) { | ||
| digraph.add_edge(v1, v2); | ||
| } |
There was a problem hiding this comment.
[nitpick] The edge existence check digraph.adj(v1).contains(&v2) on line 52 could be O(n) for each check if the adjacency list is not a set-based structure. When generating dense graphs, this could lead to O(e²) performance. Consider documenting this performance characteristic or using a HashSet to track added edges for better performance with dense graphs.
| for _ in 0..(size * 2) { | ||
| let v = rand::random::<usize>() % size; | ||
| let w = rand::random::<usize>() % size; | ||
| if v != w { | ||
| let weight = rand::random::<f64>() * 100.0; | ||
| ewg.add_edge(Edge::new(v, w, weight)); | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The random edge generation in the benchmark (lines 112-119) creates edges without ensuring the graph is connected. This means the shortest path query on line 124 may frequently fail (no path exists), making the benchmark results less meaningful. Consider using a graph generator that ensures connectivity or adding edges to guarantee a path exists between vertices 0 and v-1.
| pub fn path(v: usize) -> Graph { | ||
| let mut graph = Graph::new(v); | ||
|
|
||
| for i in 0..(v - 1) { | ||
| graph.add_edge(i, i + 1); | ||
| } | ||
|
|
||
| graph | ||
| } |
There was a problem hiding this comment.
The path function will panic when v is 0 due to integer underflow on line 252 (v - 1). Consider adding a check for v > 0 or handling the empty graph case explicitly.
For example:
pub fn path(v: usize) -> Graph {
let mut graph = Graph::new(v);
if v > 1 {
for i in 0..(v - 1) {
graph.add_edge(i, i + 1);
}
}
graph
}| pub fn tree(v: usize) -> Graph { | ||
| let mut rng = rand::thread_rng(); | ||
| let mut graph = Graph::new(v); | ||
|
|
||
| // Generate a random permutation | ||
| let mut vertices: Vec<usize> = (0..v).collect(); | ||
| for i in 0..v { | ||
| let j = rng.gen_range(i..v); | ||
| vertices.swap(i, j); | ||
| } | ||
|
|
||
| // Connect each vertex to a random previous vertex | ||
| for i in 1..v { | ||
| let j = rng.gen_range(0..i); | ||
| graph.add_edge(vertices[i], vertices[j]); | ||
| } | ||
|
|
||
| graph | ||
| } |
There was a problem hiding this comment.
The tree function will panic when v is 0 due to the range i..v being invalid when collecting vertices. Consider adding a validation check at the start:
pub fn tree(v: usize) -> Graph {
if v == 0 {
return Graph::new(0);
}
// ... rest of implementation
}| let mut rng = rand::thread_rng(); | ||
| let mut graph = Graph::new(v); | ||
|
|
||
| // Create a list of vertices, each appearing k times | ||
| let mut vertices = Vec::new(); | ||
| for i in 0..v { | ||
| for _ in 0..k { | ||
| vertices.push(i); | ||
| } | ||
| } | ||
|
|
||
| // Shuffle the list | ||
| for i in 0..vertices.len() { | ||
| let j = rng.gen_range(i..vertices.len()); | ||
| vertices.swap(i, j); | ||
| } | ||
|
|
||
| // Match pairs and add edges (avoid self-loops and parallel edges) | ||
| let mut i = 0; | ||
| while i < vertices.len() { | ||
| let v1 = vertices[i]; | ||
| let v2 = vertices[i + 1]; | ||
|
|
||
| // Avoid self-loops and parallel edges | ||
| if v1 != v2 && !graph.adj(v1).contains(&v2) { | ||
| graph.add_edge(v1, v2); | ||
| i += 2; | ||
| } else { | ||
| // Swap with a random later vertex and try again | ||
| if i + 2 < vertices.len() { | ||
| let j = rng.gen_range(i + 2..vertices.len()); | ||
| vertices.swap(i + 1, j); | ||
| } else { | ||
| // If we can't find a valid pairing, start over | ||
| return Self::regular(v, k); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| graph |
There was a problem hiding this comment.
The recursive call to Self::regular(v, k) on line 413 could potentially lead to infinite recursion or stack overflow in rare cases where a valid regular graph cannot be generated. Consider adding a retry limit or using an iterative approach with a maximum attempt counter:
// At the start of the function
const MAX_ATTEMPTS: usize = 1000;
for attempt in 0..MAX_ATTEMPTS {
// ... try to generate graph
// If successful, return
}
panic!("Could not generate regular graph after {} attempts", MAX_ATTEMPTS);| let mut rng = rand::thread_rng(); | |
| let mut graph = Graph::new(v); | |
| // Create a list of vertices, each appearing k times | |
| let mut vertices = Vec::new(); | |
| for i in 0..v { | |
| for _ in 0..k { | |
| vertices.push(i); | |
| } | |
| } | |
| // Shuffle the list | |
| for i in 0..vertices.len() { | |
| let j = rng.gen_range(i..vertices.len()); | |
| vertices.swap(i, j); | |
| } | |
| // Match pairs and add edges (avoid self-loops and parallel edges) | |
| let mut i = 0; | |
| while i < vertices.len() { | |
| let v1 = vertices[i]; | |
| let v2 = vertices[i + 1]; | |
| // Avoid self-loops and parallel edges | |
| if v1 != v2 && !graph.adj(v1).contains(&v2) { | |
| graph.add_edge(v1, v2); | |
| i += 2; | |
| } else { | |
| // Swap with a random later vertex and try again | |
| if i + 2 < vertices.len() { | |
| let j = rng.gen_range(i + 2..vertices.len()); | |
| vertices.swap(i + 1, j); | |
| } else { | |
| // If we can't find a valid pairing, start over | |
| return Self::regular(v, k); | |
| } | |
| } | |
| } | |
| graph | |
| const MAX_ATTEMPTS: usize = 1000; | |
| let mut rng = rand::thread_rng(); | |
| for _attempt in 0..MAX_ATTEMPTS { | |
| let mut graph = Graph::new(v); | |
| // Create a list of vertices, each appearing k times | |
| let mut vertices = Vec::new(); | |
| for i in 0..v { | |
| for _ in 0..k { | |
| vertices.push(i); | |
| } | |
| } | |
| // Shuffle the list | |
| for i in 0..vertices.len() { | |
| let j = rng.gen_range(i..vertices.len()); | |
| vertices.swap(i, j); | |
| } | |
| // Match pairs and add edges (avoid self-loops and parallel edges) | |
| let mut i = 0; | |
| while i < vertices.len() { | |
| let v1 = vertices[i]; | |
| let v2 = vertices[i + 1]; | |
| // Avoid self-loops and parallel edges | |
| if v1 != v2 && !graph.adj(v1).contains(&v2) { | |
| graph.add_edge(v1, v2); | |
| i += 2; | |
| } else { | |
| // Swap with a random later vertex and try again | |
| if i + 2 < vertices.len() { | |
| let j = rng.gen_range(i + 2..vertices.len()); | |
| vertices.swap(i + 1, j); | |
| } else { | |
| // If we can't find a valid pairing, restart this attempt | |
| break; | |
| } | |
| } | |
| } | |
| // If all pairs were matched, return the graph | |
| if i >= vertices.len() { | |
| return graph; | |
| } | |
| } | |
| panic!("Could not generate regular graph after {} attempts", MAX_ATTEMPTS); |
| pub fn path(v: usize) -> Digraph { | ||
| let mut digraph = Digraph::new(v); | ||
|
|
||
| for i in 0..(v - 1) { | ||
| digraph.add_edge(i, i + 1); | ||
| } | ||
|
|
||
| digraph | ||
| } |
There was a problem hiding this comment.
The path function will panic when v is 0 due to integer underflow on line 430 (v - 1). Consider adding a check similar to the graph generator:
pub fn path(v: usize) -> Digraph {
let mut digraph = Digraph::new(v);
if v > 1 {
for i in 0..(v - 1) {
digraph.add_edge(i, i + 1);
}
}
digraph
}| pub fn binary_tree(v: usize) -> Digraph { | ||
| let mut digraph = Digraph::new(v); | ||
|
|
||
| for i in 1..v { | ||
| digraph.add_edge((i - 1) / 2, i); | ||
| } | ||
|
|
||
| digraph | ||
| } |
There was a problem hiding this comment.
The binary_tree function will panic when v is 0 due to the range 1..v being invalid. Consider adding a validation check:
pub fn binary_tree(v: usize) -> Digraph {
let mut digraph = Digraph::new(v);
if v > 1 {
for i in 1..v {
digraph.add_edge((i - 1) / 2, i);
}
}
digraph
}| pub fn rooted_out_tree(v: usize) -> Digraph { | ||
| let mut rng = rand::thread_rng(); | ||
| let mut digraph = Digraph::new(v); | ||
|
|
||
| // Generate a random permutation | ||
| let mut vertices: Vec<usize> = (0..v).collect(); | ||
| for i in 0..v { | ||
| let j = rng.gen_range(i..v); | ||
| vertices.swap(i, j); | ||
| } | ||
|
|
||
| // Connect each vertex to a random later vertex | ||
| for i in 0..(v - 1) { | ||
| let j = rng.gen_range((i + 1)..v); | ||
| digraph.add_edge(vertices[i], vertices[j]); | ||
| } | ||
|
|
||
| digraph | ||
| } |
There was a problem hiding this comment.
The rooted_out_tree function will panic when v is 0 or 1 because the range (i + 1)..v may be invalid. Consider adding validation:
pub fn rooted_out_tree(v: usize) -> Digraph {
if v == 0 {
return Digraph::new(0);
}
// ... rest of implementation
}|
|
||
| ```toml | ||
| [dependencies] | ||
| algs4-graphs = "0.1" |
There was a problem hiding this comment.
[nitpick] The version specification "0.1" should be more specific to follow semantic versioning best practices. Consider using "0.1.0" for clarity and consistency with the dependency versions specified in the updated Cargo.toml.
| algs4-graphs = "0.1" | |
| algs4-graphs = "0.1.0" |
This commit addresses multiple improvements to the graphs module:
Fixed Doctests (12 total)
algs4_graphs::symbol_graph::SymbolGraphtoalgs4_graphs::SymbolGraph(modules already exported at root)Graph Generators
GraphGeneratorwith 13 methods:DigraphGeneratorwith 13 methods:Benchmarks
Crates.io Preparation
All tests pass (257 doctests in graphs module alone). Ready for crates.io publishing.