Merge pull request #8 from mitchmindtree/add_edge_heuristics

Add edge heuristic to avoid full cycle detection in common cases.

Author: mitchmindtree

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "daggy"
-version = "0.3.0"
+version = "0.4.0"
 authors = ["mitchmindtree <[email protected]>"]
 description = "A directed acyclic graph data structure library. It is Implemented on top of petgraph's Graph data structure and attempts to follow similar conventions where suitable."
 readme = "README.md"
@@ -9,6 +9,5 @@ license = "MIT/Apache-2.0"
 repository = "https://github.com/mitchmindtree/daggy.git"
 homepage = "https://github.com/mitchmindtree/daggy"
 
-
 [dependencies]
-petgraph = "0.1.15"
+petgraph = "0.2.2"

src/lib.rs

@@ -14,17 +14,15 @@
 #![forbid(unsafe_code)]
 #![warn(missing_docs)]
 
-
 extern crate petgraph as pg;
 
-
-pub use pg as petgraph;
-pub use pg::graph::{EdgeIndex, NodeIndex, EdgeWeightsMut, NodeWeightsMut};
-pub use walker::Walker;
 use pg::graph::{DefIndex, GraphIndex, IndexType};
 use std::marker::PhantomData;
 use std::ops::{Index, IndexMut};
 
+pub use pg as petgraph;
+pub use pg::graph::{EdgeIndex, NodeIndex, EdgeWeightsMut, NodeWeightsMut};
+pub use walker::Walker;
 
 pub mod walker;
 
@@ -37,6 +35,7 @@ pub type RawNodes<'a, N, Ix> = &'a [pg::graph::Node<N, Ix>];
 /// Read only access into a **Dag**'s internal edge array.
 pub type RawEdges<'a, E, Ix> = &'a [pg::graph::Edge<E, Ix>];
 
+
 /// A Directed acyclic graph (DAG) data structure.
 ///
 /// Dag is a thin wrapper around petgraph's `Graph` data structure, providing a refined API for
@@ -166,10 +165,9 @@ impl<N, E, Ix = DefIndex> Dag<N, E, Ix> where Ix: IndexType {
     /// If adding the edge **would** cause the graph to cycle, the edge will not be added and
     /// instead a `WouldCycle<E>` error with the given weight will be returned.
     ///
-    /// Computes in **O(t)** time where "t" is the time taken to check if adding the edge would
-    /// cause a cycle in the graph. See petgraph's [`is_cyclic_directed`]
+    /// In the worst case, petgraph's [`is_cyclic_directed`]
     /// (http://bluss.github.io/petulant-avenger-graphlibrary/doc/petgraph/algo/fn.is_cyclic_directed.html)
-    /// function for more details.
+    /// function is used to check whether or not adding the edge would create a cycle.
     ///
     /// **Note:** Dag allows adding parallel ("duplicate") edges. If you want to avoid this, use
     /// [`update_edge`](./struct.Dag.html#method.update_edge) instead.
@@ -178,18 +176,24 @@ impl<N, E, Ix = DefIndex> Dag<N, E, Ix> where Ix: IndexType {
     /// some other node, consider using the [add_child](./struct.Dag.html#method.add_child) or
     /// [add_parent](./struct.Dag.html#method.add_parent) methods instead for better performance.
     ///
-    /// **Panics** if the Graph is at the maximum number of nodes for its index type.
+    /// **Panics** if either `a` or `b` do not exist within the **Dag**.
+    ///
+    /// **Panics** if the Graph is at the maximum number of edges for its index type.
     pub fn add_edge(&mut self, a: NodeIndex<Ix>, b: NodeIndex<Ix>, weight: E)
         -> Result<EdgeIndex<Ix>, WouldCycle<E>>
     {
+        let should_check_for_cycle = must_check_for_cycle(self, a, b);
+
         let idx = self.graph.add_edge(a, b, weight);
 
         // Check if adding the edge has created a cycle.
-        // TODO: Once petgraph adds support for re-using visit stack/maps, use that so that we
-        // don't have to re-allocate every time `add_edge` is called.
-        if pg::algo::is_cyclic_directed(&self.graph) {
+        //
+        // TODO: Re-use a `pg::visit::Topo` for this. Perhaps store this in the `Dag` itself?
+        if should_check_for_cycle && pg::algo::is_cyclic_directed(&self.graph) {
             let weight = self.graph.remove_edge(idx).expect("No edge for index");
             Err(WouldCycle(weight))
+
+        // If no cycles were found, we're done.
         } else {
             Ok(idx)
         }
@@ -223,26 +227,34 @@ impl<N, E, Ix = DefIndex> Dag<N, E, Ix> where Ix: IndexType {
     ///
     /// **Note:** If you're adding a series of new nodes and edges to a single node, consider using
     ///  the [add_child](./struct.Dag.html#method.add_child) or [add_parent]
-    ///  (./struct.Dag.html#method.add_parent) methods instead for better performance. These
-    ///  perform better as there is no need to check for cycles.
+    ///  (./struct.Dag.html#method.add_parent) methods instead for greater convenience.
     ///
     /// **Panics** if the Graph is at the maximum number of nodes for its index type.
     pub fn add_edges<I>(&mut self, edges: I) -> Result<EdgeIndices<Ix>, WouldCycle<Vec<E>>> where
-        I: ::std::iter::IntoIterator<Item=(NodeIndex<Ix>, NodeIndex<Ix>, E)>,
+        I: IntoIterator<Item=(NodeIndex<Ix>, NodeIndex<Ix>, E)>,
     {
         let mut num_edges = 0;
+        let mut should_check_for_cycle = false;
+
         for (a, b, weight) in edges {
+            // Check whether or not we'll need to check for cycles.
+            if !should_check_for_cycle {
+                if must_check_for_cycle(self, a, b) {
+                    should_check_for_cycle = true;
+                }
+            }
+
             self.graph.add_edge(a, b, weight);
             num_edges += 1;
         }
 
         let total_edges = self.edge_count();
-        let new_edges_range = total_edges-num_edges .. total_edges;
+        let new_edges_range = total_edges-num_edges..total_edges;
 
         // Check if adding the edges has created a cycle.
-        // TODO: Once petgraph adds support for re-using visit stack/maps, use that so that we
-        // don't have to re-allocate every time `add_edges` is called.
-        if pg::algo::is_cyclic_directed(&self.graph) {
+        //
+        // TODO: Re-use a `pg::visit::Topo` for this. Perhaps store this in the `Dag` itself?
+        if should_check_for_cycle && pg::algo::is_cyclic_directed(&self.graph) {
             let removed_edges = new_edges_range.rev().filter_map(|i| {
                 let idx = EdgeIndex::new(i);
                 self.graph.remove_edge(idx)
@@ -269,7 +281,7 @@ impl<N, E, Ix = DefIndex> Dag<N, E, Ix> where Ix: IndexType {
     ///
     /// **Note:** If you're adding a new node and immediately adding a single edge to that node from
     /// some parent node, consider using the [`add_child`](./struct.Dag.html#method.add_child)
-    /// method instead for better performance.
+    /// method instead for greater convenience.
     ///
     /// **Panics** if the Graph is at the maximum number of nodes for its index type.
     pub fn update_edge(&mut self, a: NodeIndex<Ix>, b: NodeIndex<Ix>, weight: E)
@@ -469,6 +481,19 @@ impl<N, E, Ix = DefIndex> Dag<N, E, Ix> where Ix: IndexType {
 }
 
 
+/// After adding a new edge to the graph, we use this function immediately after to check whether
+/// or not we need to check for a cycle.
+/// 
+/// If our parent *a* has no parents or our child *b* has no children, or if there was already an
+/// edge connecting *a* to *b*, we know that adding this edge has not caused the graph to cycle.
+fn must_check_for_cycle<N, E, Ix>(dag: &Dag<N, E, Ix>, a: NodeIndex<Ix>, b: NodeIndex<Ix>) -> bool
+    where Ix: IndexType,
+{
+    dag.parents(a).next(dag).is_some() && dag.children(b).next(dag).is_some()
+    && dag.find_edge(a, b).is_none()
+}
+
+
 impl<N, E, Ix> Index<NodeIndex<Ix>> for Dag<N, E, Ix> where Ix: IndexType {
     type Output = N;
     fn index(&self, index: NodeIndex<Ix>) -> &N {