From 0f7c5319e4ea5b10fd5414789aff4f75951a6de7 Mon Sep 17 00:00:00 2001
From: Kasia Kozlowska <36536946+KasiaKoz@users.noreply.github.com>
Date: Fri, 15 Nov 2024 09:27:27 +0000
Subject: [PATCH] Bugfix: Multi-indexing fails in a very specific case (#249)

* add failing test

* update indexing of the groupby output

* update CHANGELOG.md

* address PR comments: test naming, make assertions more clear

* address PR comments: add more to the assertion message
---
 CHANGELOG.md               |  1 +
 src/genet/core.py          |  1 +
 tests/test_core_network.py | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f41341db..5b0e8e37 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+* Issue when adding edges to fill spaces left by removing other edges (failure in resolving multi indexing) [#248](https://github.com/arup-group/genet/issues/248)
 * Fixed generating standard outputs by highway tag which were broken after moving to storing additional attributes in short form [#217](https://github.com/arup-group/genet/pull/217)
 * Fixed summary report:
   * Intermodal Access/Egress reporting is more general (not expecting just car and bike mode access to PT) [#204](https://github.com/arup-group/genet/pull/204)
diff --git a/src/genet/core.py b/src/genet/core.py
index 68c26d50..1b535522 100644
--- a/src/genet/core.py
+++ b/src/genet/core.py
@@ -1308,6 +1308,7 @@ def generate_unique_multi_idx(group):
                 df_links[df_links["id"].isin(clashing_multi_idxs)]
                 .groupby(["from", "to"])
                 .apply(generate_unique_multi_idx)
+                .set_index("id", drop=False)
             )
 
             # generate unique indices if not
diff --git a/tests/test_core_network.py b/tests/test_core_network.py
index 3e290aa8..239c044d 100644
--- a/tests/test_core_network.py
+++ b/tests/test_core_network.py
@@ -1938,6 +1938,29 @@ def test_adding_multiple_links_with_id_and_multi_idx_clashes(assert_semantically
     )
 
 
+def test_avoids_destructive_index_clashes_when_adding_replacement_links(mocker):
+    n = Network("epsg:27700")
+    for i in range(3):
+        n.add_link(str(i), 1, 2)
+    n.remove_link(str(1))
+    # this method will sometimes output the smallest multiindex and sometimes just the next
+    # one after the highest used one. We fix it's return value to 1, an available index
+    # inbetween which caused this issue: https://github.com/arup-group/genet/issues/248
+    # I'm mocking this method here and not at the top of this test because adding the initial links
+    # to the graph would be impacted by it
+    mocker.patch.object(nx.MultiDiGraph, "new_edge_key", return_value=1)
+    links_to_add = {f"new-{i}": {"id": f"new-{i}", "from": 1, "to": 2} for i in range(3)}
+
+    reindexing_dict, links_and_attribs = n.add_links(links_to_add)
+
+    assert (
+        len(list(n.links())) == 5
+    ), "Network does not have the expected number of links: 5 = (3 original - 1 removed) + 3 new links"
+    assert (
+        len({v["multi_edge_idx"] for v in n.link_id_mapping.values()}) == 5
+    ), "Some multi-indices are not unique, the number of multi-indeces should match the number of links"
+
+
 def test_adding_multiple_links_missing_some_from_nodes():
     n = Network("epsg:27700")
     with pytest.raises(RuntimeError) as error_info: