Repository: arup-group/genet

Issue:
Reference to stop name changes during snapping+routing on network
The step to [process outputs of pyomo model](https://github.com/arup-group/genet/blob/main/genet/max_stable_set.py#L301) strips too much of the string. It changes the name of the stop, if the stop name begins with an 'x' - this leads to routing issues because the resulting stop doesn't actually exist in the network.

```python

2024-01-25 11:32:24,827 - Passing problem to solver
--
2024-01-25 11:32:24,854 -
Routing Service: `some_route_id` resulted in the following Exception:
Traceback (most recent call last):
File "/src/genet/core.py", line 1827, in route_schedule
mss = modify_schedule.route_pt_graph(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/src/genet/modify/schedule.py", line 64, in route_pt_graph
mss.solve(solver=solver)
File "/src/genet/max_stable_set.py", line 306, in solve
self.solution = {
^
File "/src/genet/max_stable_set.py", line 307, in <dictcomp>
self.problem_graph.nodes[node]["id"]: self.problem_graph.nodes[node]["link_id"]
~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
File "/usr/local/lib/python3.11/site-packages/networkx/classes/reportviews.py", line 194, in __getitem__
return self._nodes[n]
~~~~~~~~~~~^^^
KeyError: '_stop.link:1234'
```
with some stop originally named `x_stop`

Discussion:
brynpickering: How about `pd.Series(variable.extract_values())`? that's [what we use in calliope](https://github.com/calliope-project/calliope/blob/aad664ff1202d298e3265cd8994ca5e9a57788e9/calliope/backend/pyomo/util.py#L141)
KasiaKoz: I pulled out the entire code block that processes the model for solution variables - I didn't want to do it initially, because I really didn't want to mock loads of objects, but it turned out to not be that bad at all (no need to even use mocker). I noticed there was a dedicated method to get the index of the variable, which is much cleaner than processing the string representation of the variable.
I hope the method and test names look a lot better now too - let me know what you think @mfitz !

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 13315de..4954692 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## [Unreleased]
 
 ### Fixed
+* Fixed problem snapping and routing PT services with stops whose names started with 'x' [#225](https://github.com/arup-group/genet/pull/225)
 * 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)
   * Node/Links numbers were reported incorrectly (switched) [#207](https://github.com/arup-group/genet/pull/207)
diff --git a/genet/max_stable_set.py b/genet/max_stable_set.py
index 5eb3ecb..6253511 100644
--- a/genet/max_stable_set.py
+++ b/genet/max_stable_set.py
@@ -19,6 +19,14 @@ def has_attrib(attrib_value):
     return True
 
 
+def get_indices_of_chosen_problem_graph_nodes(model: pe.Model):
+    return [
+        v.index()
+        for v in model.component_data_objects(pe.Var)
+        if v.value is not None and float(v.value) == 1.0
+    ]
+
+
 class MaxStableSet:
     def __init__(self, pt_graph, network_spatial_tree, modes, distance_threshold=30, step_size=10):
         self.service_modes = modes
@@ -297,11 +305,8 @@ class MaxStableSet:
             # Solution parse
             # --------------------------------------------------------
 
-            selected = [
-                str(v).strip("x[\\']")
-                for v in model.component_data_objects(pe.Var)
-                if v.value is not None and float(v.value) == 1.0
-            ]
+            selected = get_indices_of_chosen_problem_graph_nodes(model)
+
             # solution maps Stop IDs to Link IDs
             self.solution = {
                 self.problem_graph.nodes[node]["id"]: self.problem_graph.nodes[node]["link_id"]


Test patch:
diff --git a/tests/test_max_stable_set.py b/tests/test_max_stable_set.py
index 066ae04..37f8206 100644
--- a/tests/test_max_stable_set.py
+++ b/tests/test_max_stable_set.py
@@ -1,11 +1,13 @@
 import shutil
 
 import networkx as nx
+import pyomo.environ as pe
 import pytest
 from pandas import DataFrame
 
 import genet.utils.spatial as spatial
 from genet import MaxStableSet, Network, Route, Schedule, Service, Stop
+from genet.max_stable_set import get_indices_of_chosen_problem_graph_nodes
 
 
 @pytest.fixture()
@@ -1522,3 +1524,46 @@ def test_combining_two_changesets_with_overlap(assert_semantically_equal, partia
             {"routes": {"service_1_route_2"}, "services": {"bus_service"}},
         ),
     ]
+
+
+@pytest.mark.parametrize(
+    "case_name,stop_ids",
+    [
+        ("simple stops", ["stop_3.link:link_7_8_car", "stop_1.link:link_4_5_car"]),
+        (
+            "stops with x in name",
+            ["x_stop.link:1234", "stopx.link:1234", "stxop.link:1234", "stop.link:1234x"],
+        ),
+    ],
+)
+def test_indices_of_chosen_variables_remain_unchanged_during_solution_extraction(
+    case_name, stop_ids
+):
+    model = pe.ConcreteModel()
+    model.x = pe.Var(stop_ids, within=pe.Binary)
+    # set all variables as chosen, we only care about their indices being correct
+    for v in model.component_data_objects(pe.Var):
+        v.value = 1.0
+
+    assert get_indices_of_chosen_problem_graph_nodes(model) == stop_ids
+
+
+def test_indices_of_expected_solution_nodes_are_extracted_from_the_model():
+    solution_nodes = ["stop_3.link:link_7_8_car", "bababooey"]
+    zero_value_nodes = ["some_zero_node"]
+    none_value_nodes = ["some_none_node"]
+
+    model = pe.ConcreteModel()
+    model.x = pe.Var(solution_nodes + zero_value_nodes + none_value_nodes, within=pe.Binary)
+    # set expected variable values (fake a solution)
+    for v in model.component_data_objects(pe.Var):
+        if v.index() in solution_nodes:
+            v.value = 1.0
+        elif v.index() in zero_value_nodes:
+            v.value = 0.0
+        elif v.index() in none_value_nodes:
+            v.value = None
+        else:
+            raise RuntimeError(f"Unrecognised variable: {v.index()}")
+
+    assert get_indices_of_chosen_problem_graph_nodes(model) == solution_nodes