feat: Simpler convex checker, no longer requires &mut (#114)

ConvexChecker now uses the pre-computed node toposort to compute
traverse the subgraph nodes and their causal future in order.

This should end up processing less nodes than the previous version, and
let us avoid the temporary datastructure that required `&mut checker`
all throughout the API.

I added some simple benchmarks, but unfortunately they are not the best
at showing the improvements in this refactor (we end up traversing most
of the nodes anyway).
In more real tests on `tket2` I saw small improvements, but nothing to
write home about.

benches/benchmarks/convex.rs

@@ -1,4 +1,5 @@
 use criterion::{black_box, criterion_group, AxisScale, BenchmarkId, Criterion, PlotConfiguration};
+use itertools::Itertools;
 use portgraph::{algorithms::ConvexChecker, PortView};
 
 use super::generators::make_two_track_dag;
@@ -22,26 +23,46 @@ fn bench_convex_construction(c: &mut Criterion) {
 
 /// We benchmark the worst case scenario, where the "subgraph" is the
 /// entire graph itself.
-fn bench_convex(c: &mut Criterion) {
-    let mut g = c.benchmark_group("Runtime convexity check");
+fn bench_convex_full(c: &mut Criterion) {
+    let mut g = c.benchmark_group("Runtime convexity check. Full graph.");
     g.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic));
 
     for size in [100, 1_000, 10_000] {
         let graph = make_two_track_dag(size);
-        let mut checker = ConvexChecker::new(&graph);
+        let checker = ConvexChecker::new(&graph);
         g.bench_with_input(
-            BenchmarkId::new("check_convexity", size),
+            BenchmarkId::new("check_convexity_full", size),
             &size,
             |b, _size| b.iter(|| black_box(checker.is_node_convex(graph.nodes_iter()))),
         );
     }
     g.finish();
 }
 
+/// We benchmark the an scenario where the size of the "subgraph" is sub-linear on the size of the graph.
+fn bench_convex_sparse(c: &mut Criterion) {
+    let mut g = c.benchmark_group("Runtime convexity check. Sparse subgraph on an n^2 size graph.");
+    g.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic));
+
+    for size in [100usize, 1_000, 5_000] {
+        let graph_size = size.pow(2);
+        let graph = make_two_track_dag(graph_size);
+        let checker = ConvexChecker::new(&graph);
+        let nodes = graph.nodes_iter().step_by(graph_size / size).collect_vec();
+        g.bench_with_input(
+            BenchmarkId::new("check_convexity_sparse", size),
+            &size,
+            |b, _size| b.iter(|| black_box(checker.is_node_convex(nodes.iter().copied()))),
+        );
+    }
+    g.finish();
+}
+
 criterion_group! {
     name = benches;
     config = Criterion::default();
     targets =
-        bench_convex,
+        bench_convex_full,
+        bench_convex_sparse,
         bench_convex_construction
 }

src/algorithms/convex.rs

@@ -6,23 +6,18 @@
 
 use std::collections::BTreeSet;
 
-use bitvec::bitvec;
-use bitvec::vec::BitVec;
-
 use crate::algorithms::toposort;
 use crate::{Direction, LinkView, NodeIndex, PortIndex, SecondaryMap, UnmanagedDenseMap};
 
 use super::TopoSort;
 
-/// A pre-computed datastructure for fast convexity checking.
+/// A pre-computed data structure for fast convexity checking.
 pub struct ConvexChecker<G> {
     graph: G,
     // The nodes in topological order
     topsort_nodes: Vec<NodeIndex>,
     // The index of a node in the topological order (the inverse of topsort_nodes)
     topsort_ind: UnmanagedDenseMap<NodeIndex, usize>,
-    // A temporary datastructure used during `is_convex`
-    causal: CausalVec,
 }
 
 impl<G> ConvexChecker<G>
@@ -40,12 +35,10 @@ where
         for (i, &n) in topsort_nodes.iter().enumerate() {
             topsort_ind.set(n, i);
         }
-        let causal = CausalVec::new(topsort_nodes.len());
         Self {
             graph,
             topsort_nodes,
             topsort_ind,
-            causal,
         }
     }
 
@@ -60,7 +53,7 @@ where
     /// past and in the future of another node of the subgraph.
     ///
     /// This function requires mutable access to `self` because it uses a
-    /// temporary datastructure within the object.
+    /// temporary data structure within the object.
     ///
     /// ## Arguments
     ///
@@ -77,32 +70,58 @@ where
     /// that are in the interval between the first and last node of the subgraph
     /// in some topological order. In the worst case this will traverse every
     /// node in the graph and can be improved on in the future.
-    pub fn is_node_convex(&mut self, nodes: impl IntoIterator<Item = NodeIndex>) -> bool {
+    pub fn is_node_convex(&self, nodes: impl IntoIterator<Item = NodeIndex>) -> bool {
+        // The nodes in the subgraph, in topological order.
         let nodes: BTreeSet<_> = nodes.into_iter().map(|n| self.topsort_ind[n]).collect();
+        if nodes.is_empty() {
+            return true;
+        }
+
+        // The range of considered nodes, as positions in the toposorted vector.
+        // Since the nodes are ordered, any node outside of this range will
+        // necessarily be outside the convex hull.
         let min_ind = *nodes.first().unwrap();
         let max_ind = *nodes.last().unwrap();
-        for ind in min_ind..=max_ind {
-            let n = self.topsort_nodes[ind];
-            let mut in_inds = {
-                let in_neighs = self.graph.input_neighbours(n);
-                in_neighs
+        let node_range = min_ind..=max_ind;
+
+        let mut node_iter = nodes.iter().copied().peekable();
+
+        // Nodes in the causal future of `nodes` (inside `node_range`).
+        let mut other_nodes = BTreeSet::new();
+
+        loop {
+            if node_iter.peek().is_none() {
+                break;
+            }
+            if other_nodes.is_empty() || node_iter.peek() < other_nodes.first() {
+                let current = node_iter.next().unwrap();
+                let current_node = self.topsort_nodes[current];
+                for neighbour in self
+                    .graph
+                    .output_neighbours(current_node)
                     .map(|n| self.topsort_ind[n])
-                    .filter(|&ind| ind >= min_ind)
-            };
-            if nodes.contains(&ind) {
-                if in_inds.any(|ind| self.causal.get(ind) == Causal::Future) {
-                    // There is a node in the past that is also in the future!
-                    return false;
+                    .filter(|ind| node_range.contains(ind))
+                {
+                    if !nodes.contains(&neighbour) {
+                        other_nodes.insert(neighbour);
+                    }
                 }
-                self.causal.set(ind, Causal::Past);
             } else {
-                let ind_causal = match in_inds
-                    .any(|ind| nodes.contains(&ind) || self.causal.get(ind) == Causal::Future)
+                let current = other_nodes.pop_first().unwrap();
+                let current_node = self.topsort_nodes[current];
+                for neighbour in self
+                    .graph
+                    .output_neighbours(current_node)
+                    .map(|n| self.topsort_ind[n])
+                    .filter(|ind| node_range.contains(ind))
                 {
-                    true => Causal::Future,
-                    false => Causal::Past,
-                };
-                self.causal.set(ind, ind_causal);
+                    if nodes.contains(&neighbour) {
+                        // A non-subgraph node in the causal future of the subgraph has an output neighbour in the subgraph.
+                        return false;
+                    } else {
+                        other_nodes.insert(neighbour);
+                    }
+                }
             }
         }
         true
@@ -111,15 +130,15 @@ where
     /// Whether a subgraph is convex.
     ///
     /// A subgraph is convex if there is no path between two nodes of the
-    /// sugraph that has an edge outside of the subgraph.
+    /// subgraph that has an edge outside of the subgraph.
     ///
     /// Equivalently, we check the following two conditions:
     ///  - There is no node that is both in the past and in the future of
     ///    another node of the subgraph (convexity on induced subgraph),
     ///  - There is no edge from an output port to an input port.
     ///
     /// This function requires mutable access to `self` because it uses a
-    /// temporary datastructure within the object.
+    /// temporary data structure within the object.
     ///
     /// ## Arguments
     ///
@@ -132,7 +151,7 @@ where
     /// Any edge between two nodes of the subgraph that does not have an explicit
     /// input or output port is considered within the subgraph.
     pub fn is_convex(
-        &mut self,
+        &self,
         nodes: impl IntoIterator<Item = NodeIndex>,
         inputs: impl IntoIterator<Item = PortIndex>,
         outputs: impl IntoIterator<Item = PortIndex>,
@@ -148,49 +167,6 @@ where
     }
 }
 
-/// Whether a node is in the past or in the future of a subgraph.
-#[derive(Default, Clone, Debug, PartialEq, Eq)]
-enum Causal {
-    #[default]
-    Past,
-    Future,
-}
-
-/// A memory-efficient substitute for `Vec<Causal>`.
-struct CausalVec(BitVec);
-
-impl From<bool> for Causal {
-    fn from(b: bool) -> Self {
-        match b {
-            true => Self::Future,
-            false => Self::Past,
-        }
-    }
-}
-
-impl From<Causal> for bool {
-    fn from(c: Causal) -> Self {
-        match c {
-            Causal::Past => false,
-            Causal::Future => true,
-        }
-    }
-}
-
-impl CausalVec {
-    fn new(len: usize) -> Self {
-        Self(bitvec![0; len])
-    }
-
-    fn set(&mut self, index: usize, causal: Causal) {
-        self.0.set(index, causal.into());
-    }
-
-    fn get(&self, index: usize) -> Causal {
-        self.0[index].into()
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::{LinkMut, NodeIndex, PortGraph, PortMut, PortView};
@@ -225,7 +201,7 @@ mod tests {
     #[test]
     fn induced_convexity_test() {
         let (g, [i1, i2, i3, n1, n2, o1, o2]) = graph();
-        let mut checker = ConvexChecker::new(&g);
+        let checker = ConvexChecker::new(&g);
 
         assert!(checker.is_node_convex([i1, i2, i3]));
         assert!(checker.is_node_convex([i1, n2]));
@@ -240,7 +216,7 @@ mod tests {
     #[test]
     fn edge_convexity_test() {
         let (g, [i1, i2, _, n1, n2, _, o2]) = graph();
-        let mut checker = ConvexChecker::new(&g);
+        let checker = ConvexChecker::new(&g);
 
         assert!(checker.is_convex(
             [i1, n2],
@@ -273,7 +249,7 @@ mod tests {
     fn dangling_input() {
         let mut g = PortGraph::new();
         let n = g.add_node(1, 1);
-        let mut checker = ConvexChecker::new(&g);
+        let checker = ConvexChecker::new(&g);
         assert!(checker.is_node_convex([n]));
     }
 
@@ -282,7 +258,7 @@ mod tests {
         let mut g = PortGraph::new();
         let n = g.add_node(1, 1);
         g.add_node(1, 1);
-        let mut checker = ConvexChecker::new(&g);
+        let checker = ConvexChecker::new(&g);
         assert!(checker.is_node_convex([n]));
     }
 }

src/view/subgraph.rs

@@ -96,12 +96,12 @@ where
 
     /// Whether the subgraph is convex.
     pub fn is_convex(&self) -> bool {
-        let mut checker = ConvexChecker::new(self.graph());
-        self.is_convex_with_checker(&mut checker)
+        let checker = ConvexChecker::new(self.graph());
+        self.is_convex_with_checker(&checker)
     }
 
     /// Whether the subgraph is convex, using a pre-existing checker.
-    pub fn is_convex_with_checker(&self, checker: &mut ConvexChecker<G>) -> bool {
+    pub fn is_convex_with_checker(&self, checker: &ConvexChecker<G>) -> bool {
         checker.is_convex(
             self.nodes_iter(),
             self.context().inputs.iter().copied(),