Fix knn_graph self-loops with coincident points and shared network nodes

city2graph/proximity.py

@@ -876,14 +876,15 @@ def knn_graph(
 
     if metric.name == "network":
         # Per-source distance rows instead of a dense matrix.
-        # Skip the first sorted entry (self)
+        # Sources snapped to the same network node share one row and are all at distance 0,
+        # so a source is not necessarily first in the ordering: drop it by index, not position.
         n = len(builder.coords)
         selections: list[npt.NDArray[np.intp]] = [np.empty(0, dtype=np.intp)] * n
         for indices, row in metric._iter_network_rows(builder.coords):
-            order = np.argsort(row)[1 : k + 1]
-            sel = order[row[order] < np.inf]
+            nearest = np.argsort(row, kind="stable")[: k + 1]
             for i in indices:
-                selections[i] = sel
+                neighbours = nearest[nearest != i][:k]
+                selections[i] = neighbours[row[neighbours] < np.inf]
         edges = [
             (builder.node_ids[i], builder.node_ids[j]) for i in range(n) for j in selections[i]
         ]
@@ -893,10 +894,12 @@ def knn_graph(
         tree = cKDTree(builder.coords)
         _, idxs = tree.query(builder.coords, k=n_neigh, p=p_norm)
         idxs = idxs.reshape(len(builder.coords), -1)
+        # Coincident points are all at distance 0, so a point is not necessarily its own
+        # first neighbour: drop it by index, not position.
         edges = [
             (builder.node_ids[i], builder.node_ids[j])
             for i, neigh in enumerate(idxs)
-            for j in neigh[1:]
+            for j in neigh[neigh != i][:k]
         ]
 
     builder.add_edges(edges)

tests/test_proximity.py

@@ -93,6 +93,48 @@ def test_knn_non_string_metric_defaults(small_points: gpd.GeoDataFrame) -> None:
     assert_valid_proximity_result(nodes, edges, len(small_points), allow_empty_edges=False)
 
 
+@pytest.mark.parametrize("metric", ["euclidean", "manhattan"])
+def test_knn_graph_coincident_points_have_no_self_loops(metric: str) -> None:
+    """Coincident points must neighbour each other, never themselves.
+
+    With several nodes at exactly the same location every distance to the query
+    point is zero, so the query point is not necessarily returned first by the
+    neighbour search. The graph must still link the coincident nodes together.
+    """
+    pts = make_points_simple([(0, 0), (0, 0), (0, 0), (5, 5)])
+
+    nodes, edges = knn_graph(pts, k=2, distance_metric=metric)
+
+    assert_valid_proximity_result(nodes, edges, len(pts))
+    pairs = [frozenset(pair) for pair in edges.index]
+    assert all(len(pair) == 2 for pair in pairs), "self-loop found"
+    assert {frozenset({0, 1}), frozenset({0, 2}), frozenset({1, 2})} <= set(pairs)
+    assert sum(3 in pair for pair in pairs) >= 2
+
+
+def test_knn_graph_network_metric_points_sharing_node_have_no_self_loops() -> None:
+    """Points snapped to the same network node must neighbour each other, not themselves."""
+    net = make_network_edges(
+        src_ids=[0, 1, 2],
+        dst_ids=[1, 2, 3],
+        geometries=[
+            LineString([(0, 0), (10, 0)]),
+            LineString([(10, 0), (20, 0)]),
+            LineString([(20, 0), (30, 0)]),
+        ],
+    )
+    # Points 0-2 all snap to the network node at (0, 0); 3 and 4 snap to the next nodes
+    pts = make_points_simple([(0.5, 0.3), (0.6, -0.2), (0.4, 0.1), (10, 1), (20, 1)])
+
+    nodes, edges = knn_graph(pts, k=2, distance_metric="network", network_gdf=net)
+
+    assert_valid_proximity_result(nodes, edges, len(pts))
+    pairs = [frozenset(pair) for pair in edges.index]
+    assert all(len(pair) == 2 for pair in pairs), "self-loop found"
+    # Each co-located point has the other two co-located points as its 2 nearest neighbours
+    assert {frozenset({0, 1}), frozenset({0, 2}), frozenset({1, 2})} <= set(pairs)
+
+
 def test_network_metric_requires_network_gdf(small_points: gpd.GeoDataFrame) -> None:
     """Requesting network metric without network_gdf triggers clear error."""
     assert_network_metric_requires_network_gdf(knn_graph, small_points, k=1)