Enable multiple edges in the same direction between two nodes

[jsfreischuetz]

Enable multiple edges in the same direction between two nodes

Description

Currently the solver does not support multiple edges of between two nodes in the same direction. This PR implements this feature for pandas and networkx.

scipy, because of the matrix representation, requires some more complex changes to support it. For now this is left out as it is somewhat obvious to the user that this would not be supported for this structure.

fixes #155

Implements based on feedback from #156

Checklist

• Implementation:
  • Implementation of the Mod in the gurobi_optimods installable package
  • Tests for the Mod implementation in tests/
  • Docstrings for public API, correctly linked using sphinx-autodoc
• Documentation page:
  • Background and problem specification
  • Example of the input data format (use gurobi_optimods.datasets for loading data)
  • Runnable code example
  • Presentation of solutions
  • Included in the mod gallery and toctree

[jsfreischuetz]

@simonbowly I have implemented with a more minimal implementation as requested.

[torressa]

@jsfreischuetz Thanks for the updated PR!
Just made some changes to keep the previous outputs the same (not always returning a MultiGraph) and some further clean up. Feel free to check that it all still works as expected

[jsfreischuetz]

Also, we can use the same functions to check the expected output by changing from a dictionary to a (sorted) list

--- a/tests/test_graph_utils.py
+++ b/tests/test_graph_utils.py
@@ -1,10 +1,18 @@
 import numpy as np


+def _sort_key(x):
+    return str(x)
+
+
 def check_solution_pandas(solution, candidates):
     # Checks whether the solution (`pd.Series`) matches any of the list of
     # candidates (containing `dict`)
-    if any(solution.to_dict() == c for c in candidates):
+    if any(
+        sorted(list(zip(solution.index.to_list(), solution.to_list())), key=_sort_key)
+        == sorted(c, key=_sort_key)
+        for c in candidates
+    ):
         return True
     return False

@@ -21,30 +29,10 @@ def check_solution_scipy(solution, candidates):
 def check_solution_networkx(solution, candidates):
     # Checks whether the solution (`nx.DiGraph`) matches any of the list of
     # candidates (containing tuples dict `{(i, j): data}`)
-    sol_dict = {(i, j): d for i, j, d in solution.edges(data=True)}
-    if any(sol_dict == c for c in candidates):
-        return True
-    return False
-
-
-def check_solution_pandas_multi(solution, candidates):
-    # Checks whether the solution (`pd.Series`) matches any of the list of
-    # candidates (containing `pd.Series`)
-    if any(solution.reset_index().equals(c.reset_index()) for c in candidates):
+    sol_list = sorted(
+        [((i, j), data["flow"]) for i, j, data in solution.edges(data=True)],
+        key=_sort_key,
+    )
+    if any(sol_list == sorted(c, key=_sort_key) for c in candidates):
         return True
     return False
-
-
-def check_solution_networkx_multi(solution, candidates):
-    # Checks whether the solution (`nx.DiGraph`) matches any of the list of
-    # candidates (containing tuples dict `{(i, j): data}`)
-    for candidate in candidates:
-
-        def edge_sort(row):
-            return (str(row[0]), str(row[1]))
-
-        if sorted(candidate, key=edge_sort) == sorted(
-            list(solution.edges(data=True)), key=edge_sort
-        ):
-            return True
-    return False
diff --git a/tests/test_min_cost_flow.py b/tests/test_min_cost_flow.py
index 33b00ca..df04752 100644
--- a/tests/test_min_cost_flow.py
+++ b/tests/test_min_cost_flow.py
@@ -15,9 +15,7 @@ import gurobi_optimods.min_cost_flow as mcf

 from .test_graph_utils import (
     check_solution_networkx,
-    check_solution_networkx_multi,
     check_solution_pandas,
-    check_solution_pandas_multi,
     check_solution_scipy,
 )

@@ -93,7 +91,13 @@ class TestMinCostFlow(unittest.TestCase):
         cost, sol = mcf.min_cost_flow_pandas(edge_data, node_data)
         sol = sol[sol > 0]
         self.assertEqual(cost, 31)
-        candidate = {(0, 1): 1.0, (0, 2): 1.0, (1, 3): 1.0, (2, 4): 2.0, (4, 5): 2.0}
+        candidate = [
+            ((0, 1), 1.0),
+            ((0, 2), 1.0),
+            ((1, 3), 1.0),
+            ((2, 4), 2.0),
+            ((4, 5), 2.0),
+        ]
         self.assertIsInstance(sol, pd.Series)
         self.assertTrue(check_solution_pandas(sol, [candidate]))

@@ -125,13 +129,13 @@ class TestMinCostFlow(unittest.TestCase):
         G = datasets.simple_graph_networkx()
         cost, sol = mcf.min_cost_flow_networkx(G)
         self.assertEqual(cost, 31)
-        expected = {
-            (0, 1): {"flow": 1.0},
-            (0, 2): {"flow": 1.0},
-            (1, 3): {"flow": 1.0},
-            (2, 4): {"flow": 2.0},
-            (4, 5): {"flow": 2.0},
-        }
+        expected = [
+            ((0, 1), 1.0),
+            ((0, 2), 1.0),
+            ((1, 3), 1.0),
+            ((2, 4), 2.0),
+            ((4, 5), 2.0),
+        ]
         self.assertIsInstance(sol, nx.Graph)
         self.assertTrue(check_solution_networkx(sol, [expected]))

@@ -141,13 +145,13 @@ class TestMinCostFlow(unittest.TestCase):
         G = nx.relabel_nodes(G, {0: "s", 5: "t"})
         cost, sol = mcf.min_cost_flow_networkx(G)
         self.assertEqual(cost, 31)
-        expected = {
-            ("s", 1): {"flow": 1.0},
-            ("s", 2): {"flow": 1.0},
-            (1, 3): {"flow": 1.0},
-            (2, 4): {"flow": 2.0},
-            (4, "t"): {"flow": 2.0},
-        }
+        expected = [
+            (("s", 1), 1.0),
+            (("s", 2), 1.0),
+            ((1, 3), 1.0),
+            ((2, 4), 2.0),
+            ((4, "t"), 2.0),
+        ]
         self.assertIsInstance(sol, nx.Graph)
         self.assertTrue(check_solution_networkx(sol, [expected]))

@@ -158,24 +162,24 @@ class TestMinCostFlow2(unittest.TestCase):
         cost, sol = mcf.min_cost_flow_pandas(edge_data, node_data)
         sol = sol[sol > 0]
         self.assertEqual(cost, 150)
-        candidate = {
-            (0, 1): 12.0,
-            (0, 2): 8.0,
-            (1, 3): 4.0,
-            (1, 2): 8.0,
-            (2, 3): 15.0,
-            (2, 4): 1.0,
-            (3, 4): 14.0,
-        }
-        candidate2 = {
-            (0, 1): 12.0,
-            (0, 2): 8.0,
-            (1, 3): 4.0,
-            (1, 2): 8.0,
-            (2, 3): 11.0,
-            (2, 4): 5.0,
-            (3, 4): 10.0,
-        }
+        candidate = [
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 15.0),
+            ((2, 4), 1.0),
+            ((3, 4), 14.0),
+        ]
+        candidate2 = [
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 11.0),
+            ((2, 4), 5.0),
+            ((3, 4), 10.0),
+        ]
         self.assertTrue(check_solution_pandas(sol, [candidate, candidate2]))

     def test_scipy(self):
@@ -197,24 +201,24 @@ class TestMinCostFlow2(unittest.TestCase):
         G = load_graph2_networkx()
         cost, sol = mcf.min_cost_flow_networkx(G)
         self.assertEqual(cost, 150)
-        candidate = {
-            (0, 1): {"flow": 12.0},
-            (0, 2): {"flow": 8.0},
-            (1, 2): {"flow": 8.0},
-            (1, 3): {"flow": 4.0},
-            (2, 3): {"flow": 11.0},
-            (2, 4): {"flow": 5.0},
-            (3, 4): {"flow": 10.0},
-        }
-        candidate2 = {
-            (0, 1): {"flow": 12.0},
-            (0, 2): {"flow": 8.0},
-            (1, 3): {"flow": 4.0},
-            (1, 2): {"flow": 8.0},
-            (2, 3): {"flow": 15.0},
-            (2, 4): {"flow": 1.0},
-            (3, 4): {"flow": 14.0},
-        }
+        candidate = [
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 2), 8.0),
+            ((1, 3), 4.0),
+            ((2, 3), 11.0),
+            ((2, 4), 5.0),
+            ((3, 4), 10.0),
+        ]
+        candidate2 = [
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 15.0),
+            ((2, 4), 1.0),
+            ((3, 4), 14.0),
+        ]
         self.assertTrue(check_solution_networkx(sol, [candidate, candidate2]))

     @unittest.skipIf(nx is None, "networkx is not installed")
@@ -223,23 +227,23 @@ class TestMinCostFlow2(unittest.TestCase):
         G = nx.relabel_nodes(G, {0: "s", 4: "t"})
         cost, sol = mcf.min_cost_flow_networkx(G)
         self.assertEqual(cost, 150)
-        candidate = {
-            ("s", 1): {"flow": 12.0},
-            ("s", 2): {"flow": 8.0},
-            (1, 2): {"flow": 8.0},
-            (1, 3): {"flow": 4.0},
-            (2, 3): {"flow": 11.0},
-            (2, "t"): {"flow": 5.0},
-            (3, "t"): {"flow": 10.0},
-        }
+        candidate = [
+            (("s", 1), 12.0),
+            (("s", 2), 8.0),
+            ((1, 2), 8.0),
+            ((1, 3), 4.0),
+            ((2, 3), 11.0),
+            ((2, "t"), 5.0),
+            ((3, "t"), 10.0),
+        ]
         candidate2 = {
-            ("s", 1): {"flow": 12.0},
-            ("s", 2): {"flow": 8.0},
-            (1, 3): {"flow": 4.0},
-            (1, 2): {"flow": 8.0},
-            (2, 3): {"flow": 15.0},
-            (2, "t"): {"flow": 1.0},
-            (3, "t"): {"flow": 14.0},
+            (("s", 1), 12.0),
+            (("s", 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 15.0),
+            ((2, "t"), 1.0),
+            ((3, "t"), 14.0),
         }
         self.assertTrue(check_solution_networkx(sol, [candidate, candidate2]))

@@ -251,15 +255,17 @@ class TestMinCostFlow3(unittest.TestCase):
         sol = sol[sol > 0]
         self.assertEqual(cost, 49.0)

-        candidate = pd.DataFrame(
-            {
-                "source": [0, 0, 1, 1, 2, 2, 3, 2],
-                "target": [1, 2, 3, 2, 3, 4, 4, 3],
-                "flow": [12.0, 8.0, 4.0, 8.0, 10.0, 5.0, 10.0, 1.0],
-            }
-        ).set_index(["source", "target"])
-
-        self.assertTrue(check_solution_pandas_multi(sol, [candidate]))
+        candidate = [
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 10.0),
+            ((2, 4), 5.0),
+            ((3, 4), 10.0),
+            ((2, 3), 1.0),
+        ]
+        self.assertTrue(check_solution_pandas(sol, [candidate]))

     @unittest.skipIf(nx is None, "networkx is not installed")
     def test_networkx(self):
@@ -267,14 +273,14 @@ class TestMinCostFlow3(unittest.TestCase):
         cost, sol = mcf.min_cost_flow_networkx(G)
         self.assertEqual(cost, 49.0)
         candidate = [
-            (0, 1, {"flow": 12.0}),
-            (0, 2, {"flow": 8.0}),
-            (1, 3, {"flow": 4.0}),
-            (1, 2, {"flow": 8.0}),
-            (2, 3, {"flow": 10.0}),
-            (2, 3, {"flow": 1.0}),
-            (2, 4, {"flow": 5.0}),
-            (3, 4, {"flow": 10.0}),
+            ((0, 1), 12.0),
+            ((0, 2), 8.0),
+            ((1, 3), 4.0),
+            ((1, 2), 8.0),
+            ((2, 3), 10.0),
+            ((2, 3), 1.0),
+            ((2, 4), 5.0),
+            ((3, 4), 10.0),
         ]

-        self.assertTrue(check_solution_networkx_multi(sol, [candidate]))
+        self.assertTrue(check_solution_networkx(sol, [candidate]))

I had this code in a previous commit, but was trying to limit changes, but thanks!

[jsfreischuetz]

@jsfreischuetz Thanks for the updated PR! Just made some changes to keep the previous outputs the same (not always returning a MultiGraph) and some further clean up. Feel free to check that it all still works as expected

Everything seems to be working properly

[torressa]

I had this code in a previous commit, but was trying to limit changes, but thanks!

Ah cool, sorry, did not realise. I think it makes sense, it is safer than dictionaries as the order can sometimes differ leading to randomly failing tests. I can revert it if Simon is not happy with it.

@simonbowly please review, sorry about the numerous changes to the tests but I think it is better like this

[simonbowly]

I have a couple of comments.

Changing the test structure is ok by me, as long as it doesn't change the meaning of the tests. I simplified things a bit further in 2536cbf

[simonbowly]

Thanks @jsfreischuetz and @torressa! I'll release the changes as version 2.2, just want to take a look at another open PR first and hopefully merge and release that in the same go.