Fix and add tests for unused grapht functions

src/util/graph.h

@@ -678,6 +678,11 @@ std::vector<typename N::node_indext> grapht<N>::depth_limited_search(
   return src;
 }
 
+/// Find connected subgraphs in an undirected graph.
+/// \param [out] subgraph_nr: will be resized to graph.size() and populated
+///   to map node indices onto subgraph numbers. The subgraph numbers are dense,
+///   in the range 0 - (number of subgraphs - 1)
+/// \return Number of subgraphs found.
 template<class N>
 std::size_t grapht<N>::connected_subgraphs(
   std::vector<node_indext> &subgraph_nr)
@@ -709,12 +714,11 @@ std::size_t grapht<N>::connected_subgraphs(
 
       const nodet &node=nodes[n];
 
-      for(typename edgest::const_iterator
-          it=node.out.begin();
-          it!=node.out.end();
-          it++)
-        if(!visited[*it])
-          s.push(*it);
+      for(const auto &o : node.out)
+      {
+        if(!visited[o.first])
+          s.push(o.first);
+      }
     }
 
     nr++;
@@ -791,6 +795,10 @@ std::size_t grapht<N>::SCCs(std::vector<node_indext> &subgraph_nr) const
   return t.scc_count;
 }
 
+/// Ensure a graph is chordal (contains no 4+-cycles without an edge crossing
+/// the cycle) by adding extra edges. Note this adds more edges than are
+/// required, including to acyclic graphs or acyclic subgraphs of cyclic graphs,
+/// but does at least ensure the graph is not chordal.
 template<class N>
 void grapht<N>::make_chordal()
 {
@@ -805,20 +813,13 @@ void grapht<N>::make_chordal()
     const nodet &n=tmp[i];
 
     // connect all the nodes in n.out with each other
-
-    for(typename edgest::const_iterator
-        it1=n.out.begin();
-        it1!=n.out.end();
-        it1++)
-      for(typename edgest::const_iterator
-          it2=n.out.begin();
-          it2!=n.out.end();
-          it2++)
+    for(const auto &o1 : n.out)
+      for(const auto &o2 : n.out)
       {
-        if(*it1!=*it2)
+        if(o1.first!=o2.first)
         {
-          tmp.add_undirected_edge(*it1, *it2);
-          this->add_undirected_edge(*it1, *it2);
+          tmp.add_undirected_edge(o1.first, o2.first);
+          this->add_undirected_edge(o1.first, o2.first);
         }
       }
 

unit/Makefile

@@ -24,6 +24,7 @@ SRC += unit_tests.cpp \
        solvers/refinement/string_refinement/sparse_array.cpp \
        solvers/refinement/string_refinement/union_find_replace.cpp \
        util/expr_cast/expr_cast.cpp \
+       util/graph.cpp \
        util/irep.cpp \
        util/irep_sharing.cpp \
        util/message.cpp \

unit/util/graph.cpp

@@ -0,0 +1,312 @@
+/*******************************************************************\
+
+Module: Unit test for graph.h
+
+Author: Diffblue Ltd
+
+\*******************************************************************/
+
+#include <testing-utils/catch.hpp>
+
+#include <util/graph.h>
+
+template<class E>
+static inline bool operator==(
+  const graph_nodet<E> &gn1, const graph_nodet<E> &gn2)
+{
+  return gn1.in == gn2.in && gn1.out == gn2.out;
+}
+
+template<class N>
+static inline bool operator==(const grapht<N> &g1, const grapht<N> &g2)
+{
+  if(g1.size() != g2.size())
+    return false;
+
+  for(typename grapht<N>::node_indext i = 0; i < g1.size(); ++i)
+  {
+    if(!(g1[i] == g2[i]))
+      return false;
+  }
+
+  return true;
+}
+
+/// To verify make_chordal is working as intended: naively search for a
+/// hole (a chordless 4+-cycle)
+template<class N>
+static bool contains_hole(
+  const grapht<N> &g,
+  const std::vector<typename grapht<N>::node_indext> &cycle_so_far)
+{
+  const auto &successors_map = g[cycle_so_far.back()].out;
+
+  // If this node has a triangular edge (one leading to cycle_so_far[-3]) then
+  // this isn't a hole:
+  if(cycle_so_far.size() >= 3 &&
+     successors_map.count(cycle_so_far[cycle_so_far.size() - 3]) != 0)
+  {
+    return false;
+  }
+
+  // If this node has an edge leading to any other cycle member (except our
+  // immediate predecessor) then we've found a hole:
+  if(cycle_so_far.size() >= 4)
+  {
+    for(std::size_t i = 0; i <= cycle_so_far.size() - 4; ++i)
+    {
+      if(successors_map.count(cycle_so_far[i]) != 0)
+        return true;
+    }
+  }
+
+  // Otherwise try to extend the cycle:
+  for(const auto &successor : successors_map)
+  {
+    // The input is undirected, so a 2-cycle is always present:
+    if(cycle_so_far.size() >= 2 &&
+       successor.first == cycle_so_far[cycle_so_far.size() - 2])
+    {
+      continue;
+    }
+
+    std::vector<typename grapht<N>::node_indext> extended_cycle = cycle_so_far;
+    extended_cycle.push_back(successor.first);
+    if(contains_hole(g, extended_cycle))
+      return true;
+  }
+
+  return false;
+}
+
+template<class N>
+static bool contains_hole(const grapht<N> &g)
+{
+  // For each node in the graph, check for cycles starting at that node.
+  // This is pretty dumb, but I figure this formulation is simple enough to
+  // check by hand and the complexity isn't too bad for small examples.
+
+  for(typename grapht<N>::node_indext i = 0; i < g.size(); ++i)
+  {
+    std::vector<typename grapht<N>::node_indext> start_node {i};
+    if(contains_hole(g, start_node))
+      return true;
+  }
+
+  return false;
+}
+
+typedef grapht<graph_nodet<empty_edget>> simple_grapht;
+
+SCENARIO("graph-make-chordal",
+  "[core][util][graph]")
+{
+  GIVEN("An acyclic graph")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[5];
+
+    for(int i = 0; i < 5; ++i)
+      indices[i] = graph.add_node();
+
+    // Make a graph: 0 <-> 1 <-> 4
+    //                \->  2  <-/
+    //                 \-> 3
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[0], indices[2]);
+    graph.add_undirected_edge(indices[0], indices[3]);
+    graph.add_undirected_edge(indices[1], indices[4]);
+    graph.add_undirected_edge(indices[2], indices[4]);
+
+    WHEN("The graph is made chordal")
+    {
+      simple_grapht chordal_graph = graph;
+      chordal_graph.make_chordal();
+
+      THEN("The graph should be unchanged")
+      {
+        // This doesn't pass, as make_chordal actually adds triangular edges to
+        // *all* common neighbours, even nodes that aren't part of a cycle.
+        // REQUIRE(graph == chordal_graph);
+
+        // At least it shouldn't be chordal!
+        REQUIRE(!contains_hole(chordal_graph));
+      }
+    }
+  }
+
+  GIVEN("A cyclic graph that is already chordal")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[5];
+
+    for(int i = 0; i < 5; ++i)
+      indices[i] = graph.add_node();
+
+    // Make a graph: 0 <-> 1 <-> 2 <-> 0
+    //               3 <-> 1 <-> 2 <-> 3
+    //               4 <-> 1 <-> 2 <-> 4
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[1], indices[2]);
+    graph.add_undirected_edge(indices[2], indices[0]);
+    graph.add_undirected_edge(indices[3], indices[1]);
+    graph.add_undirected_edge(indices[2], indices[3]);
+    graph.add_undirected_edge(indices[4], indices[1]);
+    graph.add_undirected_edge(indices[2], indices[4]);
+
+    WHEN("The graph is made chordal")
+    {
+      simple_grapht chordal_graph = graph;
+      chordal_graph.make_chordal();
+
+      THEN("The graph should be unchanged")
+      {
+        // This doesn't pass, as make_chordal actually adds triangular edges to
+        // *all* common neighbours, even cycles that are already chordal.
+        // REQUIRE(graph == chordal_graph);
+
+        // At least it shouldn't be chordal!
+        REQUIRE(!contains_hole(chordal_graph));
+      }
+    }
+  }
+
+  GIVEN("A simple 4-cycle")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[4];
+
+    for(int i = 0; i < 4; ++i)
+      indices[i] = graph.add_node();
+
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[1], indices[2]);
+    graph.add_undirected_edge(indices[2], indices[3]);
+    graph.add_undirected_edge(indices[3], indices[0]);
+
+    // Check the contains_hole predicate is working as intended:
+    REQUIRE(contains_hole(graph));
+
+    WHEN("The graph is made chordal")
+    {
+      simple_grapht chordal_graph = graph;
+      chordal_graph.make_chordal();
+
+      THEN("The graph should gain a chord")
+      {
+        REQUIRE(!contains_hole(chordal_graph));
+      }
+    }
+  }
+
+  GIVEN("A more complicated graph with a hole")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[8];
+
+    for(int i = 0; i < 8; ++i)
+      indices[i] = graph.add_node();
+
+    // A 5-cycle:
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[1], indices[2]);
+    graph.add_undirected_edge(indices[2], indices[3]);
+    graph.add_undirected_edge(indices[3], indices[4]);
+    graph.add_undirected_edge(indices[4], indices[0]);
+
+    // A 3-cycle connected to the 5:
+    graph.add_undirected_edge(indices[4], indices[5]);
+    graph.add_undirected_edge(indices[5], indices[6]);
+    graph.add_undirected_edge(indices[6], indices[4]);
+
+    // Another 3-cycle joined onto the 5:
+    graph.add_undirected_edge(indices[1], indices[7]);
+    graph.add_undirected_edge(indices[3], indices[7]);
+
+    // Check we've made the input correctly:
+    REQUIRE(contains_hole(graph));
+
+    WHEN("The graph is made chordal")
+    {
+      simple_grapht chordal_graph = graph;
+      chordal_graph.make_chordal();
+
+      THEN("The graph's 5-cycle should be completed with chords")
+      {
+        REQUIRE(!contains_hole(chordal_graph));
+      }
+    }
+  }
+}
+
+SCENARIO("graph-connected-subgraphs",
+  "[core][util][graph]")
+{
+  GIVEN("A connected graph")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[5];
+
+    for(int i = 0; i < 5; ++i)
+      indices[i] = graph.add_node();
+
+    // Make a graph: 0 <-> 1 <-> 4
+    //                \->  2  <-/
+    //                 \-> 3
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[0], indices[2]);
+    graph.add_undirected_edge(indices[0], indices[3]);
+    graph.add_undirected_edge(indices[1], indices[4]);
+    graph.add_undirected_edge(indices[2], indices[4]);
+
+    WHEN("We take its connected subgraphs")
+    {
+      std::vector<simple_grapht::node_indext> subgraphs;
+      graph.connected_subgraphs(subgraphs);
+
+      REQUIRE(subgraphs.size() == graph.size());
+      simple_grapht::node_indext only_subgraph = subgraphs.at(0);
+
+      // Check everything is in one subgraph:
+      REQUIRE(
+        subgraphs ==
+        std::vector<simple_grapht::node_indext>(graph.size(), only_subgraph));
+    }
+  }
+
+  GIVEN("A graph with three unconnected subgraphs")
+  {
+    simple_grapht graph;
+    simple_grapht::node_indext indices[6];
+
+    for(int i = 0; i < 6; ++i)
+      indices[i] = graph.add_node();
+
+    graph.add_undirected_edge(indices[0], indices[1]);
+    graph.add_undirected_edge(indices[2], indices[3]);
+    graph.add_undirected_edge(indices[4], indices[5]);
+
+    WHEN("We take its connected subgraphs")
+    {
+      std::vector<simple_grapht::node_indext> subgraphs;
+      graph.connected_subgraphs(subgraphs);
+
+      REQUIRE(subgraphs.size() == graph.size());
+      simple_grapht::node_indext first_subgraph = subgraphs.at(0);
+      simple_grapht::node_indext second_subgraph = subgraphs.at(2);
+      simple_grapht::node_indext third_subgraph = subgraphs.at(4);
+
+      std::vector<simple_grapht::node_indext> expected_subgraphs
+      {
+        first_subgraph,
+        first_subgraph,
+        second_subgraph,
+        second_subgraph,
+        third_subgraph,
+        third_subgraph
+      };
+
+      REQUIRE(subgraphs == expected_subgraphs);
+    }
+  }
+}