From f58c53ccb74b8b9a2fcc1255b5fc526a45d72ee5 Mon Sep 17 00:00:00 2001 From: Dynisious Date: Sat, 23 Mar 2019 18:25:20 +1100 Subject: [PATCH 1/5] Fixing `cyclic` (WIP) I've writen a new solution for cyclic and am now testing it. --- src/iterators/dfs.rs | 95 ++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 56 deletions(-) diff --git a/src/iterators/dfs.rs b/src/iterators/dfs.rs index 5458718..b2dfb78 100644 --- a/src/iterators/dfs.rs +++ b/src/iterators/dfs.rs @@ -3,7 +3,7 @@ use crate::graph::Graph; use crate::vertex_id::VertexId; -use hashbrown::HashMap; +use hashbrown::{HashMap, HashSet,}; use std::sync::Arc; #[derive(Debug)] @@ -13,6 +13,8 @@ pub struct Dfs<'a, T> { color_map: HashMap, Color>, roots_stack: Vec>, iterable: &'a Graph, + /// A cached answer to the question: does this Graph contain cycles. + cached_cyclic: Option, } #[derive(Debug)] @@ -47,70 +49,51 @@ impl<'a, T> Dfs<'a, T> { recursion_stack: Vec::with_capacity(graph.vertex_count()), roots_stack, iterable: graph, + cached_cyclic: None, } } /// Returns true if the iterated graph has a cycle. - pub fn is_cyclic(&mut self) -> bool { - while !self.roots_stack.is_empty() { - let root = self.roots_stack[self.roots_stack.len() - 1].clone(); - - // No vertices have been visited yet, - // so we begin from the current root. - if self.recursion_stack.is_empty() { - self.recursion_stack.push(root.clone()); - self.color_map.insert(root.clone(), Color::Grey); - } - - let mut current = self.recursion_stack.pop().unwrap(); - - loop { - if self.iterable.out_neighbors_count(current.as_ref()) == 0 - && !self.recursion_stack.is_empty() - { - // Mark as processed - self.color_map.insert(current.clone(), Color::Black); - - // Set new current as popped value from recursion stack - current = self.recursion_stack.pop().unwrap(); - continue; - } - - break; - } - - let mut all_are_black = true; - - // Traverse current neighbors - for n in self.iterable.out_neighbors(current.as_ref()) { - let reference = Arc::from(*n); - - if let Some(Color::White) = self.color_map.get(&reference) { - self.recursion_stack.push(current.clone()); - self.recursion_stack.push(reference.clone()); - self.color_map.insert(reference, Color::Grey); - all_are_black = false; - break; - } - - // This means there is a cycle - if let Some(Color::Grey) = self.color_map.get(&reference) { - return true; + pub fn is_cyclic(&mut self,) -> bool { + //Check for a cached answer. + if let Some(cyclic) = self.cached_cyclic { return cyclic } + + //Calculate the answer. + let cyclic = (|| { + //The vertices pending processing. + let mut pending_stack = Vec::new(); + //The ids of all the visited vertices. + let mut visited = HashSet::new(); + + //Iterate all roots to check all paths. + for root in self.iterable.roots() { + //This indicates that we have not encountered an already visited vertex + let mut root_visited = HashSet::new(); + + pending_stack.push(*root); + //Process all pending vertices. + while let Some(v) = pending_stack.pop() { + //If true This path has found a cycle. + if !root_visited.insert(v) { return true } + + //Add all of its outbound neibours to be processed. + for &v in self.iterable.out_neighbors(&v) { + //If this vertex exists in visited then we have already checked it for cycles. + if !visited.contains(&v) { + pending_stack.push(v) + } + } } - } - if all_are_black { - self.color_map.insert(current.clone(), Color::Black); + //Move all of the vertices visited in this check into the previously visited pool. + visited.extend(root_visited); } - // Begin traversing from next root if the - // recursion stack is empty. - if self.recursion_stack.is_empty() { - self.roots_stack.pop(); - } - } + false + })(); - false + self.cached_cyclic = Some(cyclic); + return cyclic; } } From e65bd047709ae58e2369d28bb69e312993ed671c Mon Sep 17 00:00:00 2001 From: Dynisious Date: Sat, 23 Mar 2019 19:03:24 +1100 Subject: [PATCH 2/5] Bug Fix Fixed edge case: previously if there were no roots in the graph then `is_cyclic` was `false` but if there are no roots then there *must* be a cycle. --- src/graph.rs | 4 ++-- src/iterators/dfs.rs | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 85df75f..f5fe1b1 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -522,9 +522,9 @@ impl Graph { /// graph.add_edge(&v3, &v4).unwrap(); /// /// assert!(!graph.is_cyclic()); - /// + /// /// graph.add_edge(&v3, &v1); - /// + /// /// assert!(graph.is_cyclic()); /// ``` pub fn is_cyclic(&self) -> bool { diff --git a/src/iterators/dfs.rs b/src/iterators/dfs.rs index b2dfb78..e615f38 100644 --- a/src/iterators/dfs.rs +++ b/src/iterators/dfs.rs @@ -60,23 +60,26 @@ impl<'a, T> Dfs<'a, T> { //Calculate the answer. let cyclic = (|| { + //If there are no roots then there must be cycles. + if self.iterable.roots_count() == 0 { return true } + //The vertices pending processing. let mut pending_stack = Vec::new(); //The ids of all the visited vertices. let mut visited = HashSet::new(); + //This is all the vertices which have been visited by the current root. + let mut root_visited = HashSet::new(); //Iterate all roots to check all paths. for root in self.iterable.roots() { - //This indicates that we have not encountered an already visited vertex - let mut root_visited = HashSet::new(); - pending_stack.push(*root); + //Process all pending vertices. while let Some(v) = pending_stack.pop() { - //If true This path has found a cycle. + //If true there is a cycle. if !root_visited.insert(v) { return true } - - //Add all of its outbound neibours to be processed. + + //Add all of this vertexes outbound neibours to be processed. for &v in self.iterable.out_neighbors(&v) { //If this vertex exists in visited then we have already checked it for cycles. if !visited.contains(&v) { @@ -85,8 +88,8 @@ impl<'a, T> Dfs<'a, T> { } } - //Move all of the vertices visited in this check into the previously visited pool. - visited.extend(root_visited); + //Forget all the vertexes visited from this root specifically. + visited.extend(root_visited.drain()); } false From bcb9d0325dbbd43c020258078ecd2d1ad5ec232d Mon Sep 17 00:00:00 2001 From: Dynisious Date: Sat, 23 Mar 2019 23:28:12 +1100 Subject: [PATCH 3/5] Rewrite `Dfs` * The previous version of `Dfs` worked but was far removed functionally from the original implementation. I've rewritten `Dfs` to be logically similar. * I've added a test for the edge case which caused the original implementation of `is_cyclic` to fail. * I've created a new macro `set` and have used it to update some tests which were failing on semantics instead of logic. --- src/graph.rs | 74 +++++++-------- src/iterators/dfs.rs | 222 +++++++++++++++++++------------------------ src/macros.rs | 22 +++++ 3 files changed, 152 insertions(+), 166 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index f5fe1b1..1c642a6 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -831,30 +831,27 @@ impl Graph { /// /// ## Example /// ```rust + /// # #[macro_use] extern crate graphlib; fn main() { /// use graphlib::Graph; - /// + /// use std::collections::HashSet; + /// /// let mut graph: Graph = Graph::new(); - /// let mut vertices = vec![]; - /// + /// /// let v1 = graph.add_vertex(0); /// let v2 = graph.add_vertex(1); /// let v3 = graph.add_vertex(2); /// let v4 = graph.add_vertex(3); - /// + /// /// graph.add_edge(&v1, &v2).unwrap(); /// graph.add_edge(&v3, &v1).unwrap(); /// graph.add_edge(&v1, &v4).unwrap(); - /// - /// // Iterate over vertices - /// for v in graph.dfs() { - /// vertices.push(v); - /// } - /// - /// assert_eq!(vertices.len(), 4); - /// assert_eq!(vertices[0], &v3); - /// assert_eq!(vertices[1], &v1); - /// assert_eq!(vertices[2], &v2); - /// assert_eq!(vertices[3], &v4); + /// + /// let mut dfs = graph.dfs(); + /// + /// assert_eq!(dfs.next(), Some(&v3)); + /// assert_eq!(dfs.next(), Some(&v1)); + /// assert!(set![&v2, &v4] == dfs.collect()); + /// # } /// ``` pub fn dfs(&self) -> Dfs<'_, T> { Dfs::new(self) @@ -920,8 +917,7 @@ mod tests { #[test] fn dfs() { let mut graph: Graph = Graph::new(); - let mut vertices = vec![]; - + let v1 = graph.add_vertex(0); let v2 = graph.add_vertex(1); let v3 = graph.add_vertex(2); @@ -931,47 +927,45 @@ mod tests { graph.add_edge(&v3, &v1).unwrap(); graph.add_edge(&v1, &v4).unwrap(); - // Iterate over vertices - for v in graph.dfs() { - vertices.push(v); - } + let mut dfs = graph.dfs(); - assert_eq!(vertices.len(), 4); - assert_eq!(vertices[0], &v3); - assert_eq!(vertices[1], &v1); - assert_eq!(vertices[2], &v2); - assert_eq!(vertices[3], &v4); + assert_eq!(dfs.next(), Some(&v3)); + assert_eq!(dfs.next(), Some(&v1)); + assert!(set![&v2, &v4] == dfs.collect()); } #[test] fn dfs_mul_roots() { let mut graph: Graph = Graph::new(); - let mut vertices = vec![]; - + let v1 = graph.add_vertex(0); let v2 = graph.add_vertex(1); let v3 = graph.add_vertex(2); let v4 = graph.add_vertex(3); - let v5 = graph.add_vertex(4); - let v6 = graph.add_vertex(5); - graph.add_edge(&v1, &v2).unwrap(); graph.add_edge(&v3, &v1).unwrap(); graph.add_edge(&v1, &v4).unwrap(); + + let v5 = graph.add_vertex(4); + let v6 = graph.add_vertex(5); + graph.add_edge(&v5, &v6).unwrap(); // Iterate over vertices - for v in graph.dfs() { - vertices.push(v); + let mut dfs = graph.dfs(); + + for _ in 0..2 { + let v = dfs.next(); + + if v == Some(&v3) { + assert_eq!(dfs.next(), Some(&v1)); + assert!(set![&v2, &v4] == (&mut dfs).take(2).collect()); + } else if v == Some(&v5) { + assert_eq!(dfs.next(), Some(&v6)); + } else { panic!("Not a root node") } } - assert_eq!(vertices.len(), 6); - assert_eq!(vertices[0], &v5); - assert_eq!(vertices[1], &v6); - assert_eq!(vertices[2], &v3); - assert_eq!(vertices[3], &v1); - assert_eq!(vertices[4], &v2); - assert_eq!(vertices[5], &v4); + assert_eq!(dfs.count(), 0, "There were remaining nodes"); } } diff --git a/src/iterators/dfs.rs b/src/iterators/dfs.rs index e615f38..bf407b8 100644 --- a/src/iterators/dfs.rs +++ b/src/iterators/dfs.rs @@ -2,172 +2,142 @@ use crate::graph::Graph; use crate::vertex_id::VertexId; +use crate::iterators::VertexIter; -use hashbrown::{HashMap, HashSet,}; -use std::sync::Arc; +use hashbrown::HashSet; +use std::iter::{Cloned, Chain, Peekable,}; #[derive(Debug)] /// Depth-First Iterator pub struct Dfs<'a, T> { - recursion_stack: Vec>, - color_map: HashMap, Color>, - roots_stack: Vec>, + /// All the vertices to be checked with the roots coming first. + unchecked: Peekable, VertexIter<'a>>>>, + /// All previously visited vertices. + visited: HashSet, + /// All vertices pending processing. + pending_stack: Vec, + /// The Graph being iterated. iterable: &'a Graph, /// A cached answer to the question: does this Graph contain cycles. - cached_cyclic: Option, -} - -#[derive(Debug)] -enum Color { - White, - Grey, - Black, + cached_cyclic: bool, } impl<'a, T> Dfs<'a, T> { pub fn new(graph: &'a Graph) -> Dfs<'_, T> { - let mut roots_stack = Vec::with_capacity(graph.roots_count()); - let color_map: HashMap, Color> = graph - .vertices() - .map(|v| (Arc::from(*v), Color::White)) - .collect(); - - if graph.roots_count() == 0 && graph.vertex_count() != 0 { - // Pick random vertex as first root - for (random_vertex, _) in color_map.iter() { - roots_stack.push(random_vertex.clone()); - break; - } - } else { - for v in graph.roots() { - roots_stack.push(Arc::from(*v)); - } - } + let unchecked = graph.roots() + .chain(graph.vertices()) + .cloned().peekable(); Dfs { - color_map, - recursion_stack: Vec::with_capacity(graph.vertex_count()), - roots_stack, + unchecked, iterable: graph, - cached_cyclic: None, + cached_cyclic: false, + visited: HashSet::new(), + pending_stack: Vec::new(), } } /// Returns true if the iterated graph has a cycle. + /// + /// # Warning + /// + /// It is a logic error to use this iterator after calling this function. pub fn is_cyclic(&mut self,) -> bool { //Check for a cached answer. - if let Some(cyclic) = self.cached_cyclic { return cyclic } - - //Calculate the answer. - let cyclic = (|| { - //If there are no roots then there must be cycles. - if self.iterable.roots_count() == 0 { return true } - - //The vertices pending processing. - let mut pending_stack = Vec::new(); - //The ids of all the visited vertices. - let mut visited = HashSet::new(); - //This is all the vertices which have been visited by the current root. - let mut root_visited = HashSet::new(); + if self.cached_cyclic { return self.cached_cyclic } + + //Search until an answer is found. + while self.process_vertex().is_some() {} + + self.cached_cyclic + } + + /// Processes the next vertex. + /// + /// Will return None if: + /// + /// * No vertices are left. + /// * The next vertex forms a cycle. + fn process_vertex(&mut self,) -> Option<&'a VertexId> { + //We have traversed this partition of the graph, move on. + if self.pending_stack.is_empty() { + //Spliting the borrows for the borrow checker. + let unchecked = &mut self.unchecked; + let visited = &self.visited; + + //Search for an unprocessed vertex. + let next = unchecked.find(move |v,| !visited.contains(v)); - //Iterate all roots to check all paths. - for root in self.iterable.roots() { - pending_stack.push(*root); - - //Process all pending vertices. - while let Some(v) = pending_stack.pop() { - //If true there is a cycle. - if !root_visited.insert(v) { return true } - - //Add all of this vertexes outbound neibours to be processed. - for &v in self.iterable.out_neighbors(&v) { - //If this vertex exists in visited then we have already checked it for cycles. - if !visited.contains(&v) { - pending_stack.push(v) - } - } - } - - //Forget all the vertexes visited from this root specifically. - visited.extend(root_visited.drain()); + //We found a new vertex. + if let Some(v) = next { + self.pending_stack.push(v); + } + } + + //Get the next pending vertex. + self.pending_stack.pop().iter() + //Filter cycles. + .filter_map(|v,| { + //If this vertex forms a cycle do not return it. + if !self.visited.insert(*v) { + self.cached_cyclic = true; + + return None } - false - })(); + //Add all of its neighbours to be processed. + for v in self.iterable.out_neighbors(v) { + //This neighbour forms a cycle don't process it. + if self.visited.contains(v) { self.cached_cyclic = true } + else { self.pending_stack.push(*v) } + } - self.cached_cyclic = Some(cyclic); - return cyclic; + self.iterable.fetch_id_ref(v) + }).next() } } impl<'a, T> Iterator for Dfs<'a, T> { type Item = &'a VertexId; + fn size_hint(&self) -> (usize, Option) { + let remaining = self.iterable.vertex_count() - self.visited.len(); + + (remaining, Some(remaining)) + } + #[inline] fn next(&mut self) -> Option { - while !self.roots_stack.is_empty() { - let root = self.roots_stack[self.roots_stack.len() - 1].clone(); + (0..self.size_hint().0).filter_map(move |_,| self.process_vertex()).next() + } +} - // No vertices have been visited yet, - // so we begin from the current root. - if self.recursion_stack.is_empty() { - self.recursion_stack.push(root.clone()); - self.color_map.insert(root.clone(), Color::Grey); +#[cfg(test)] +mod tests { + use super::*; - return self.iterable.fetch_id_ref(root.as_ref()); - } + #[test] + fn is_cyclic() { + /* + A previous version of the function would fail if the iterator had passed through the last cycle. - // Check if the topmost item on the recursion stack - // has outbound neighbors. If it does, we traverse - // them until we find one that is unvisited. - // - // If either the topmost item on the recursion stack - // doesn't have neighbors or all of its neighbors - // are visited, we pop it from the stack. - let mut current = self.recursion_stack.pop().unwrap(); - - loop { - if self.iterable.out_neighbors_count(current.as_ref()) == 0 - && !self.recursion_stack.is_empty() - { - // Mark as processed - self.color_map.insert(current.clone(), Color::Black); - - // Pop from recursion stack - current = self.recursion_stack.pop().unwrap(); - - continue; - } - - break; - } + The current version written 2019-03-23 caches if any cycles have been found as it + iterates to resolve this issue. + */ - let mut mark = true; + for _ in 0..100 { + let mut graph = Graph::new(); - // Traverse current neighbors - for n in self.iterable.out_neighbors(current.as_ref()) { - let reference = Arc::from(*n); + let v = graph.add_vertex(0); - if let Some(Color::White) = self.color_map.get(&reference) { - self.recursion_stack.push(current); - self.recursion_stack.push(reference.clone()); - self.color_map.insert(reference, Color::Grey); - mark = false; + assert!(graph.add_edge(&v, &v).is_ok(), "Failed to create cycle"); - return Some(n); - } - } + for _ in 0..100 { graph.add_vertex(0); } - if mark { - self.color_map.insert(current.clone(), Color::Black); - } + let mut dfs = graph.dfs(); - // Begin traversing from next root if the - // recursion stack is empty. - if self.recursion_stack.is_empty() { - self.roots_stack.pop(); - } - } + for _ in 0..99 { dfs.next(); } - None + assert!(dfs.is_cyclic()); + } } } diff --git a/src/macros.rs b/src/macros.rs index 2eb7ab1..6e1a611 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -4,3 +4,25 @@ macro_rules! min { ($x: expr) => ($x); ($x: expr, $($z: expr),+) => (::std::cmp::min($x, min!($($z),*))); } + +/// Returns a HashSet containing the passed values. +#[macro_export] +macro_rules! set { + ($fst:expr $(, $v:expr)*) => ({ + let mut set = HashSet::with_capacity(count!($fst $(, $v)*)); + + set.insert($fst); + $(set.insert($v);)* + + set + }); +} + +/// Counts the number of values passed to it. +#[macro_export] +macro_rules! count { + () => (0); + ($fst:expr) => (1); + ($fst:expr, $snd:expr) => (2); + ($fst:expr, $snd:expr $(, $v:expr)*) => (1 + count!($snd $(, $v)*)); +} From d7ca9fbb942db31a55b4d995a48937c32c1a227b Mon Sep 17 00:00:00 2001 From: Dynisious Date: Sun, 24 Mar 2019 00:22:52 +1100 Subject: [PATCH 4/5] Improved `VertexIter` I've added improvements to `VertexIter` so that it uses a Trait-Object instead of allocating a `Vec` making it smaller and lazy. --- src/graph.rs | 57 ++++++++++++--------------------------- src/iterators/vertices.rs | 34 +++++++---------------- 2 files changed, 27 insertions(+), 64 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 1c642a6..a30996b 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -666,13 +666,10 @@ impl Graph { /// assert_eq!(neighbors[0], &v3); /// ``` pub fn in_neighbors(&self, id: &VertexId) -> VertexIter<'_> { - let mut collection: Vec<&VertexId> = vec![]; - - if let Some(inbounds) = self.inbound_table.get(id) { - collection = inbounds.iter().map(|v| v.as_ref()).collect(); - }; - - VertexIter::new(collection) + match self.inbound_table.get(id) { + Some(neighbors) => VertexIter(Box::new(neighbors.iter().map(AsRef::as_ref,),),), + None => VertexIter(Box::new(std::iter::empty(),),), + } } /// Returns an iterator over the outbound neighbors @@ -704,16 +701,13 @@ impl Graph { /// assert_eq!(neighbors[1], &v4); /// ``` pub fn out_neighbors(&self, id: &VertexId) -> VertexIter<'_> { - let mut collection: Vec<&VertexId> = vec![]; - - if let Some(outbounds) = self.outbound_table.get(id) { - collection = outbounds.iter().map(|v| v.as_ref()).collect(); - }; - - VertexIter::new(collection) + match self.outbound_table.get(id,) { + Some(iter) => VertexIter(Box::new(iter.iter().map(AsRef::as_ref,),),), + None => VertexIter(Box::new(std::iter::empty(),),), + } } - /// Returns an iterator over the outbound neighbors + /// Returns an iterator over the inbound and outbound neighbors /// of the vertex with the given id. /// /// ## Example @@ -743,26 +737,13 @@ impl Graph { /// assert_eq!(neighbors[2], &v3); /// ``` pub fn neighbors(&self, id: &VertexId) -> VertexIter<'_> { - let mut collection: Vec<&VertexId> = vec![]; + let mut visited = HashSet::new(); + let neighbors = self.out_neighbors(id,) + .chain(self.in_neighbors(id,),) + //Remove duplicates. + .filter(move |&&v,| visited.insert(v,),); - match (self.outbound_table.get(id), self.inbound_table.get(id)) { - (Some(outbounds), None) => { - collection = outbounds.iter().map(|v| v.as_ref()).collect(); - } - (None, Some(inbounds)) => { - collection = inbounds.iter().map(|v| v.as_ref()).collect(); - } - (Some(outbounds), Some(inbounds)) => { - collection = outbounds.iter().map(|v| v.as_ref()).collect(); - - let inbounds: Vec<&VertexId> = inbounds.iter().map(|v| v.as_ref()).collect(); - - collection.extend_from_slice(&inbounds); - } - (None, None) => {} // Do nothing - }; - - VertexIter::new(collection) + VertexIter(Box::new(neighbors,),) } /// Returns an iterator over the root vertices @@ -793,9 +774,7 @@ impl Graph { /// assert_eq!(roots[0], &v3); /// ``` pub fn roots(&self) -> VertexIter<'_> { - let collection: Vec<&VertexId> = self.roots.iter().map(|v| v.as_ref()).collect(); - - VertexIter::new(collection) + VertexIter(Box::new(self.roots.iter().map(AsRef::as_ref,),),) } /// Returns an iterator over all of the @@ -821,9 +800,7 @@ impl Graph { /// assert_eq!(vertices.len(), 4); /// ``` pub fn vertices(&self) -> VertexIter<'_> { - let collection: Vec<&VertexId> = self.vertices.iter().map(|(v, _)| v.as_ref()).collect(); - - VertexIter::new(collection) + VertexIter(Box::new(self.vertices.keys().map(AsRef::as_ref,),),) } /// Returns an iterator over the vertices diff --git a/src/iterators/vertices.rs b/src/iterators/vertices.rs index 911114d..5a40168 100644 --- a/src/iterators/vertices.rs +++ b/src/iterators/vertices.rs @@ -1,34 +1,20 @@ // Copyright 2019 Octavian Oncescu use crate::vertex_id::VertexId; +use std::fmt::Debug; -#[derive(Debug)] -/// Generic Vertex Iterator -pub struct VertexIter<'a> { - current: usize, - iterable: Vec<&'a VertexId>, -} +pub(crate) trait MergedTrait<'a,>: Iterator + Debug {} -impl<'a> VertexIter<'a> { - pub fn new(neighbors: Vec<&'a VertexId>) -> VertexIter<'a> { - VertexIter { - current: 0, - iterable: neighbors, - } - } -} +impl<'a, T,> MergedTrait<'a,> for T + where T: Iterator + Debug {} + +/// Generic Vertex Iterator. +#[derive(Debug)] +pub struct VertexIter<'a>(pub(crate) Box>,); impl<'a> Iterator for VertexIter<'a> { type Item = &'a VertexId; - fn next(&mut self) -> Option { - if self.current == self.iterable.len() { - return None; - } - - let result = self.iterable[self.current]; - self.current += 1; - - Some(result) - } + #[inline] + fn next(&mut self) -> Option { self.0.next() } } From ed519d48761226b7cc2268a175fadf19a5ec0121 Mon Sep 17 00:00:00 2001 From: Dynisious Date: Sun, 24 Mar 2019 01:10:59 +1100 Subject: [PATCH 5/5] Bug Fix Noticed a big bug where I would classify any retraversal as a cycle because I forgot colour and there were no negative test cases. I've fixed the issue with colouring and added a test case. --- src/iterators/dfs.rs | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/iterators/dfs.rs b/src/iterators/dfs.rs index bf407b8..ba9096d 100644 --- a/src/iterators/dfs.rs +++ b/src/iterators/dfs.rs @@ -12,8 +12,10 @@ use std::iter::{Cloned, Chain, Peekable,}; pub struct Dfs<'a, T> { /// All the vertices to be checked with the roots coming first. unchecked: Peekable, VertexIter<'a>>>>, - /// All previously visited vertices. - visited: HashSet, + /// All black vertices. + black: HashSet, + /// All grey vertices. + grey: HashSet, /// All vertices pending processing. pending_stack: Vec, /// The Graph being iterated. @@ -32,7 +34,8 @@ impl<'a, T> Dfs<'a, T> { unchecked, iterable: graph, cached_cyclic: false, - visited: HashSet::new(), + grey: HashSet::new(), + black: HashSet::new(), pending_stack: Vec::new(), } } @@ -61,12 +64,15 @@ impl<'a, T> Dfs<'a, T> { fn process_vertex(&mut self,) -> Option<&'a VertexId> { //We have traversed this partition of the graph, move on. if self.pending_stack.is_empty() { + //Mark all the grey vertices black. + self.black.extend(self.grey.drain(),); + //Spliting the borrows for the borrow checker. let unchecked = &mut self.unchecked; - let visited = &self.visited; + let black = &self.black; //Search for an unprocessed vertex. - let next = unchecked.find(move |v,| !visited.contains(v)); + let next = unchecked.find(move |v,| !black.contains(v)); //We found a new vertex. if let Some(v) = next { @@ -79,7 +85,7 @@ impl<'a, T> Dfs<'a, T> { //Filter cycles. .filter_map(|v,| { //If this vertex forms a cycle do not return it. - if !self.visited.insert(*v) { + if !self.grey.insert(*v) { self.cached_cyclic = true; return None @@ -88,7 +94,7 @@ impl<'a, T> Dfs<'a, T> { //Add all of its neighbours to be processed. for v in self.iterable.out_neighbors(v) { //This neighbour forms a cycle don't process it. - if self.visited.contains(v) { self.cached_cyclic = true } + if self.grey.contains(v) { self.cached_cyclic = true } else { self.pending_stack.push(*v) } } @@ -101,13 +107,12 @@ impl<'a, T> Iterator for Dfs<'a, T> { type Item = &'a VertexId; fn size_hint(&self) -> (usize, Option) { - let remaining = self.iterable.vertex_count() - self.visited.len(); + let remaining = self.iterable.vertex_count() - self.black.len(); - (remaining, Some(remaining)) + (0, Some(remaining)) } - #[inline] fn next(&mut self) -> Option { - (0..self.size_hint().0).filter_map(move |_,| self.process_vertex()).next() + (0..self.size_hint().1.unwrap()).filter_map(move |_,| self.process_vertex()).next() } } @@ -140,4 +145,19 @@ mod tests { assert!(dfs.is_cyclic()); } } + #[test] + fn not_cyclic() { + let mut graph = Graph::new(); + + let v1 = graph.add_vertex(()); + let v2 = graph.add_vertex(()); + let v3 = graph.add_vertex(()); + + graph.add_edge(&v1, &v2,); + graph.add_edge(&v3, &v2,); + + graph.add_vertex(()); + + assert!(graph.is_cyclic() == false,); + } }