Fix NetworkX motif matching correctness

.github/workflows/python-package.yml

@@ -11,7 +11,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        python-version: ['3.9', '3.10', '3.11', '3.12', '3.13', '3.14']
+        python-version: ['3.10', '3.11', '3.12', '3.13', '3.14']
 
     steps:
       - uses: actions/checkout@v3
@@ -36,10 +36,7 @@ jobs:
 
       - name: Set up uv venv and install dependencies
         run: |
-          uv venv
-          source .venv/bin/activate
-          uv pip install ruff pytest pytest-cov pytest-codspeed
-          uv pip install -e .
+          uv sync --locked --group dev
 
       - name: Lint with ruff
         run: |
@@ -49,4 +46,4 @@ jobs:
       - name: Test with pytest
         run: |
           source .venv/bin/activate
-          pytest
+          pytest

dotmotif/executors/NetworkXExecutor.py

@@ -302,8 +302,8 @@ def _validate_multigraph_all_edge_constraints(
             graph_u = node_isomorphism_map[motif_U]
             graph_v = node_isomorphism_map[motif_V]
 
-            # Check each edge in graph for constraints
-            for _, _, edge_attrs in graph.edges((graph_u, graph_v), data=True):
+            # Check each parallel edge between the mapped endpoints.
+            for edge_attrs in graph.get_edge_data(graph_u, graph_v).values():
                 if not _edge_satisfies_constraints(edge_attrs, constraint_list):
                     # Fail fast
                     return False
@@ -333,7 +333,7 @@ def _validate_multigraph_any_edge_constraints(
 
             # Check each edge in graph for constraints
             constraint_list_copy = copy.deepcopy(constraint_list)
-            for _, _, edge_attrs in graph.edges((graph_u, graph_v), data=True):
+            for edge_attrs in graph.get_edge_data(graph_u, graph_v).values():
                 matched_constraints = (
                     _edge_satisfies_many_constraints_for_muligraph_any_edges(
                         edge_attrs, constraint_list_copy
@@ -380,45 +380,36 @@ def find(self, motif: "dotmotif.Motif", limit: Optional[int] = None):
         # filter them out later on. Though this reduces the speed of the graph-
         # matching, NetworkX does not seem to support this out of the box.
 
-        if motif.ignore_direction or not self.graph.is_directed:
+        if motif.ignore_direction:
+            graph_constructor = nx.Graph
+            graph_matcher = nx.algorithms.isomorphism.GraphMatcher
+            host_graph = self.graph.to_undirected(as_view=True)
+        elif not self.graph.is_directed():
             graph_constructor = nx.Graph
             graph_matcher = nx.algorithms.isomorphism.GraphMatcher
+            host_graph = self.graph
         else:
             graph_constructor = nx.DiGraph
             graph_matcher = nx.algorithms.isomorphism.DiGraphMatcher
+            host_graph = self.graph
 
         only_positive_edges_motif = graph_constructor()
+        only_positive_edges_motif.add_nodes_from(motif.to_nx().nodes(data=True))
         must_not_exist_edges = []
         for u, v, attrs in motif.to_nx().edges(data=True):
             if attrs["exists"] is True:
                 only_positive_edges_motif.add_edge(u, v, **attrs)
             elif attrs["exists"] is False:
                 # Collect a list of neg-edges to check for again in a moment
                 must_not_exist_edges.append((u, v))
-        gm = graph_matcher(self.graph, only_positive_edges_motif)
+        gm = graph_matcher(host_graph, only_positive_edges_motif)
 
         def _doesnt_have_any_of_motifs_negative_edges(mapping):
             for u, v in must_not_exist_edges:
-                if self.graph.has_edge(mapping[u], mapping[v]):
+                if host_graph.has_edge(mapping[u], mapping[v]):
                     return False
             return True
 
-        unfiltered_results = [
-            # Here, `mapping` has keys of self.graph node IDs and values of
-            # motif node names. We need the reverse for pretty much everything
-            # we do from here out, so we reverse the pairs.
-            {v: k for k, v in mapping.items()}
-            # TODO: Use isomorphism here if requested
-            for mapping in gm.subgraph_monomorphisms_iter()
-        ]
-
-        # Now, filter out those that have edges they should not:
-        results = [
-            mapping
-            for mapping in unfiltered_results
-            if _doesnt_have_any_of_motifs_negative_edges(mapping)
-        ]
-
         _edge_constraint_validator = (
             self._validate_edge_constraints
             if not self._host_is_multigraph
@@ -429,29 +420,38 @@ def _doesnt_have_any_of_motifs_negative_edges(mapping):
             )
         )
         _edge_dynamic_constraint_validator = self._validate_dynamic_edge_constraints
-        # Now, filter on attributes:
-        res = [
-            r
-            for r in results
+        effective_limit = motif.limit if limit is None else limit
+        res = []
+        for mapping in gm.subgraph_monomorphisms_iter():
+            # NetworkX maps host node IDs to motif names; validators use the
+            # reverse mapping.
+            r = {v: k for k, v in mapping.items()}
             if (
-                _edge_constraint_validator(r, self.graph, motif.list_edge_constraints())
-                and _edge_dynamic_constraint_validator(
-                    r, self.graph, motif.list_dynamic_edge_constraints()
-                )
-                and self._validate_node_constraints(
-                    r, self.graph, motif.list_node_constraints()
-                )
-                and self._validate_dynamic_node_constraints(
-                    r, self.graph, motif.list_dynamic_node_constraints()
-                )
-                # by default, networkx returns the automorphism that is left-
-                # sorted, so this comparison is _opposite_ the check that we
-                # use in the other executors. In other words, we usually check
-                # that A >= B; here we check A <= B.
+                _doesnt_have_any_of_motifs_negative_edges(r)
                 and (
-                    (not motif.exclude_automorphisms)
-                    or all(r[a] <= r[b] for (a, b) in motif.list_automorphisms())
+                    _edge_constraint_validator(
+                        r, self.graph, motif.list_edge_constraints()
+                    )
+                    and _edge_dynamic_constraint_validator(
+                        r, self.graph, motif.list_dynamic_edge_constraints()
+                    )
+                    and self._validate_node_constraints(
+                        r, self.graph, motif.list_node_constraints()
+                    )
+                    and self._validate_dynamic_node_constraints(
+                        r, self.graph, motif.list_dynamic_node_constraints()
+                    )
+                    # NetworkX returns the left-sorted automorphism, so this
+                    # comparison is opposite the other executors' check.
+                    and (
+                        (not motif.exclude_automorphisms)
+                        or all(
+                            r[a] <= r[b] for (a, b) in motif.list_automorphisms()
+                        )
+                    )
                 )
-            )
-        ]
-        return res[:limit] if limit is not None else res
+            ):
+                res.append(r)
+                if effective_limit is not None and len(res) >= effective_limit:
+                    break
+        return res

dotmotif/executors/test_networkxexecutor.py

@@ -195,6 +195,32 @@ def test_in_string(self):
 
 
 class TestSmallMotifs(unittest.TestCase):
+    def test_ignore_direction_with_directed_host(self):
+        motif = dotmotif.Motif("A -> B", ignore_direction=True)
+        host = nx.DiGraph([("x", "y")])
+
+        self.assertEqual(len(NetworkXExecutor(graph=host).find(motif)), 2)
+
+    def test_negative_only_motif(self):
+        motif = dotmotif.Motif("A !> B")
+        host = nx.DiGraph()
+        host.add_nodes_from(["x", "y"])
+
+        self.assertEqual(
+            {tuple(sorted(result.items())) for result in NetworkXExecutor(graph=host).find(motif)},
+            {
+                (("A", "x"), ("B", "y")),
+                (("A", "y"), ("B", "x")),
+            },
+        )
+
+    def test_find_stops_at_limit(self):
+        motif = dotmotif.Motif("A -> B")
+        host = nx.DiGraph((str(i), str(i + 1)) for i in range(10))
+        executor = NetworkXExecutor(graph=host)
+
+        self.assertEqual(len(executor.find(motif, limit=1)), 1)
+
     def test_edgecount_motif(self):
         dm = dotmotif.Motif("""A->B""")
 

dotmotif/tests/test_multigraphs.py

@@ -95,6 +95,25 @@ def test_multigraph_basic(executor):
     assert len(results) == 1
 
 
+def test_multigraph_constraints_ignore_reverse_and_incident_edges():
+    haystack = nx.MultiDiGraph()
+    haystack.add_edge("A", "B", size=10)
+    haystack.add_edge("A", "B", size=20)
+    haystack.add_edge("B", "A", size=0)
+    haystack.add_edge("A", "C", size=0)
+    motif = Motif("a -> b [size > 0]")
+
+    all_results = NetworkXExecutor(
+        graph=haystack, multigraph_edge_match="all"
+    ).find(motif)
+    any_results = NetworkXExecutor(
+        graph=haystack, multigraph_edge_match="any"
+    ).find(motif)
+
+    assert all_results == [{"a": "A", "b": "B"}]
+    assert any_results == [{"a": "A", "b": "B"}]
+
+
 @pytest.mark.parametrize("executor", [NetworkXExecutor, GrandIsoExecutor])
 def test_impossible_constraint_works_on_multigraph(executor):
     """
@@ -146,7 +165,7 @@ def test_complex_multigraph(executor):
     )
 
     results = executor(graph=haystack, multigraph_edge_match="any").find(motif)
-    assert len(results) == 2
+    assert len(results) == 1
 
     results = executor(graph=haystack, multigraph_edge_match="all").find(motif)
     assert len(results) == 0