Fix bug in check_hypergraph_acyclicity

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Removed
 ### Deprecated
 ### Fixed
+ - Fixed a bug in `check_hypergraph_acyclicity` reported by @abcorrea.
 
 
 ## [0.6.0]- 2020-09-18

src/tarski/analysis/csp.py

@@ -37,21 +37,44 @@ def compute_primal_graph_from_hypergraph(hypergraph):
 
 
 def _remove_ear_if_exists(edges, node_counts):
-    """ A helper to find a so-called ear of the hypergraph """
-    for e1, e2 in itertools.product(edges, edges):
-        diff = e1-e2
-        if diff and all(node_counts[node] == 1 for node in diff):
-            # e1 is an ear and can be removed. We update node count as well
-            for node in e1:
-                node_counts[node] -= 1
-            edges.remove(e1)
+    """ Find and remove from the given set of edges a so-called "ear" and return True.
+    If no such ear exists, return False.
+
+    From Abiteboul, S., Hull, R. and Vianu, V (1995). Foundations of Databases:
+        An ear of hypergraph F = (V, F) is an edge f ∈ F such that for some distinct f' ∈ F,
+        no vertex of f − f' is in any other edge [...]. In this case, f' is called a witness that f is an ear.
+        As a special case, if there is an edge f of F that intersects no other edge, then f is also considered an ear.
+
+    Note that this implementation is possibly not the most efficient, but it should work fine for the kind of small
+    hypergraphs we're interested in.
+    """
+    def remove_edge(edge):
+        for node in edge:
+            node_counts[node] -= 1
+        edges.remove(edge)
+
+    for f in edges:
+        # Check first the "special case"
+        if all(node_counts[node] == 1 for node in f):
+            remove_edge(f)
+            # print(f'Edge {f} removed because it intersects no other edge')
+            return True
+
+    for f, fprime in itertools.permutations(edges, 2):
+        # Note that this includes the case where f \subseteq f'.
+        # Abiteboul et al. assume that hypergraph is "reduced" and f has thus been compiled into f' at preprocessing;
+        # we simply compile it away here on the fly by assuming it is indeed an ear and can be removed.
+        if all(node_counts[node] == 1 for node in f-fprime):
+            remove_edge(f)
+            # print(f'Edge {f} removed in favor of {fprime}')
             return True
+    # print(f'No more ears found in {edges}')
     return False
 
 
 def check_hypergraph_acyclicity(hypergraph):
     """ Check whether the given hypergraph is acyclic by applying the GYO reduction as described in
-        Jeffrey D. Ullman, Principles of Database and Knowledge-Base Systems, Vol. II
+        Abiteboul, S., Hull, R. and Vianu, V (1995). Foundations of Databases, pp.131-132.
     """
     nodes = set(itertools.chain.from_iterable(hypergraph))
     edges = set(frozenset(x) for x in hypergraph)  # simply convert the tuple into frozensets
@@ -64,10 +87,11 @@ def check_hypergraph_acyclicity(hypergraph):
         for n in edge:
             node_counts[n] += 1
 
+    # This is essentially the GYO algorithm:
     while _remove_ear_if_exists(edges, node_counts):
         pass
 
-    return len(edges) <= 1
+    return len(edges) == 0
 
 
 def _collect_hyperedges(phi, edges):

tests/analysis/test_csp.py

@@ -5,23 +5,34 @@
 from tests.io.common import collect_strips_benchmarks, reader
 
 
-def test_hypergraph_generation():
-    def compute_acyclicity(instance):
-        instance_file, domain_file = collect_strips_benchmarks([instance])[0]
-        problem = reader(case_insensitive=True).read_problem(domain_file, instance_file)
-        hgraphs = {a.name: compute_schema_constraint_hypergraph(a) for a in problem.actions.values()}
-        return {name: check_hypergraph_acyclicity(h) for name, h in hgraphs.items()}
-
-    acyclicity = compute_acyclicity("pipesworld-notankage:p01-net1-b6-g2.pddl")
-    assert acyclicity == {'PUSH-START': False, 'PUSH-END': True, 'POP-START': False,
-                          'POP-END': True, 'PUSH-UNITARYPIPE': False, 'POP-UNITARYPIPE': False}
-
-    acyclicity = compute_acyclicity("ged-opt14-strips:d-1-2.pddl")
-    assert all(x is True for x in acyclicity.values())
-
-
-def check_acyclicity(domain_file, instance_file):
-    problem = reader(case_insensitive=True, strict_with_requirements=False).read_problem(domain_file, instance_file)
-    for a in problem.actions.values():
-        acyclic = check_hypergraph_acyclicity(compute_schema_constraint_hypergraph(a))
-        print(f'Action {a.name} is {"acyclic" if acyclic else "cyclic"}.')
+def test_acyclicity_detection():
+    # assert check_hypergraph_acyclicity({('room',), ('obj', 'room'), ('gripper',), ('obj',)}) is True
+    # assert check_hypergraph_acyclicity({('gripper', 'obj'), ('gripper',), ('obj',), ('room',)}) is True
+
+    # A hyperedge contained in another hyperedge is correctly marked as acyclic
+    assert check_hypergraph_acyclicity({('X', ), ('X', 'Y')}) is True
+
+    # Two repeated hyperedges are dealt with adequately
+    assert check_hypergraph_acyclicity({('X', ), ('X', 'Y'), ('Y', 'X')}) is True
+
+    # Two non-intersecting hyperedges are acyclic
+    assert check_hypergraph_acyclicity({('S', 'T'), ('X', 'Y')}) is True
+
+    # A simple cyclic hypergraph
+    assert check_hypergraph_acyclicity({('X', 'Y'), ('Y', 'Z'), ('Z', 'X')}) is False
+
+    # Now compute acyclicity over a few domain schemas
+    assert compute_acyclicity("pipesworld-notankage:p01-net1-b6-g2.pddl") ==\
+           {'PUSH-START': False, 'PUSH-END': True, 'POP-START': False,
+            'POP-END': True, 'PUSH-UNITARYPIPE': False, 'POP-UNITARYPIPE': False}
+
+    assert all(x is True for x in compute_acyclicity("ged-opt14-strips:d-1-2.pddl").values())
+
+    assert compute_acyclicity("gripper:prob01.pddl") == {'move': True, 'pick': True, 'drop': True}
+
+
+def compute_acyclicity(instance):
+    instance_file, domain_file = collect_strips_benchmarks([instance])[0]
+    problem = reader(case_insensitive=True).read_problem(domain_file, instance_file)
+    hgraphs = {a.name: compute_schema_constraint_hypergraph(a) for a in problem.actions.values()}
+    return {name: check_hypergraph_acyclicity(h) for name, h in hgraphs.items()}