Merge pull request #3212 from Robbybp/incidence-remove-error

Require variables and constraints to be specified separately in `IncidenceGraphInterface.remove_nodes`

Author: mrmundt

Diff for: pyomo/contrib/incidence_analysis/interface.py

@@ -28,7 +28,7 @@
     scipy as sp,
     plotly,
 )
-from pyomo.common.deprecation import deprecated
+from pyomo.common.deprecation import deprecated, deprecation_warning
 from pyomo.contrib.incidence_analysis.config import get_config_from_kwds
 from pyomo.contrib.incidence_analysis.matching import maximum_matching
 from pyomo.contrib.incidence_analysis.connected import get_independent_submatrices
@@ -453,11 +453,29 @@ def _validate_input(self, variables, constraints):
                 raise ValueError("Neither variables nor a model have been provided.")
             else:
                 variables = self.variables
+        elif self._incidence_graph is not None:
+            # If variables were provided and an incidence graph is cached,
+            # make sure the provided variables exist in the graph.
+            for var in variables:
+                if var not in self._var_index_map:
+                    raise KeyError(
+                        f"Variable {var} does not exist in the cached"
+                        " incidence graph."
+                    )
         if constraints is None:
             if self._incidence_graph is None:
                 raise ValueError("Neither constraints nor a model have been provided.")
             else:
                 constraints = self.constraints
+        elif self._incidence_graph is not None:
+            # If constraints were provided and an incidence graph is cached,
+            # make sure the provided constraints exist in the graph.
+            for con in constraints:
+                if con not in self._con_index_map:
+                    raise KeyError(
+                        f"Constraint {con} does not exist in the cached"
+                        " incidence graph."
+                    )
 
         _check_unindexed(variables + constraints)
         return variables, constraints
@@ -854,7 +872,7 @@ def dulmage_mendelsohn(self, variables=None, constraints=None):
         # Hopefully this does not get too confusing...
         return var_partition, con_partition
 
-    def remove_nodes(self, nodes, constraints=None):
+    def remove_nodes(self, variables=None, constraints=None):
         """Removes the specified variables and constraints (columns and
         rows) from the cached incidence matrix.
 
@@ -866,35 +884,76 @@ def remove_nodes(self, nodes, constraints=None):
 
         Parameters
         ----------
-        nodes: list
-            VarData or ConData objects whose columns or rows will be
-            removed from the incidence matrix.
+        variables: list
+            VarData objects whose nodes will be removed from the incidence graph
         constraints: list
-            VarData or ConData objects whose columns or rows will be
-            removed from the incidence matrix.
+            ConData objects whose nodes will be removed from the incidence graph
+
+        .. note::
+
+           **Deprecation in Pyomo v6.7.2.dev0**
+
+           The pre-6.7.2.dev0 implementation of ``remove_nodes`` allowed variables and
+           constraints to remove to be specified in a single list. This made
+           error checking difficult, and indeed, if invalid components were
+           provided, we carried on silently instead of throwing an error or
+           warning. As part of a fix to raise an error if an invalid component
+           (one that is not part of the incidence graph) is provided, we now require
+           variables and constraints to be specified separately.
 
         """
         if constraints is None:
             constraints = []
+        if variables is None:
+            variables = []
         if self._incidence_graph is None:
             raise RuntimeError(
                 "Attempting to remove variables and constraints from cached "
                 "incidence matrix,\nbut no incidence matrix has been cached."
             )
-        to_exclude = ComponentSet(nodes)
-        to_exclude.update(constraints)
-        vars_to_include = [v for v in self.variables if v not in to_exclude]
-        cons_to_include = [c for c in self.constraints if c not in to_exclude]
+
+        vars_to_validate = []
+        cons_to_validate = []
+        depr_msg = (
+            "In IncidenceGraphInterface.remove_nodes, passing variables and"
+            " constraints in the same list is deprecated. Please separate your"
+            " variables and constraints and pass them in the order variables,"
+            " constraints."
+        )
+        if any(var in self._con_index_map for var in variables) or any(
+            con in self._var_index_map for con in constraints
+        ):
+            deprecation_warning(depr_msg, version="6.7.2.dev0")
+        # If we received variables/constraints in the same list, sort them.
+        # Any unrecognized objects will be caught by _validate_input.
+        for var in variables:
+            if var in self._con_index_map:
+                cons_to_validate.append(var)
+            else:
+                vars_to_validate.append(var)
+        for con in constraints:
+            if con in self._var_index_map:
+                vars_to_validate.append(con)
+            else:
+                cons_to_validate.append(con)
+
+        variables, constraints = self._validate_input(
+            vars_to_validate, cons_to_validate
+        )
+        v_exclude = ComponentSet(variables)
+        c_exclude = ComponentSet(constraints)
+        vars_to_include = [v for v in self.variables if v not in v_exclude]
+        cons_to_include = [c for c in self.constraints if c not in c_exclude]
         incidence_graph = self._extract_subgraph(vars_to_include, cons_to_include)
         # update attributes
         self._variables = vars_to_include
         self._constraints = cons_to_include
         self._incidence_graph = incidence_graph
         self._var_index_map = ComponentMap(
-            (var, i) for i, var in enumerate(self.variables)
+            (var, i) for i, var in enumerate(vars_to_include)
         )
         self._con_index_map = ComponentMap(
-            (con, i) for i, con in enumerate(self._constraints)
+            (con, i) for i, con in enumerate(cons_to_include)
         )
 
     def plot(self, variables=None, constraints=None, title=None, show=True):

Diff for: pyomo/contrib/incidence_analysis/tests/test_interface.py

@@ -634,17 +634,15 @@ def test_exception(self):
         nlp = PyomoNLP(model)
         igraph = IncidenceGraphInterface(nlp)
 
-        with self.assertRaises(RuntimeError) as exc:
+        with self.assertRaisesRegex(KeyError, "does not exist"):
             variables = [model.P]
             constraints = [model.ideal_gas]
             igraph.maximum_matching(variables, constraints)
-        self.assertIn("must be unindexed", str(exc.exception))
 
-        with self.assertRaises(RuntimeError) as exc:
+        with self.assertRaisesRegex(KeyError, "does not exist"):
             variables = [model.P]
             constraints = [model.ideal_gas]
             igraph.block_triangularize(variables, constraints)
-        self.assertIn("must be unindexed", str(exc.exception))
 
 
 @unittest.skipUnless(networkx_available, "networkx is not available.")
@@ -885,17 +883,15 @@ def test_exception(self):
         model = make_gas_expansion_model()
         igraph = IncidenceGraphInterface(model)
 
-        with self.assertRaises(RuntimeError) as exc:
+        with self.assertRaisesRegex(KeyError, "does not exist"):
             variables = [model.P]
             constraints = [model.ideal_gas]
             igraph.maximum_matching(variables, constraints)
-        self.assertIn("must be unindexed", str(exc.exception))
 
-        with self.assertRaises(RuntimeError) as exc:
+        with self.assertRaisesRegex(KeyError, "does not exist"):
             variables = [model.P]
             constraints = [model.ideal_gas]
             igraph.block_triangularize(variables, constraints)
-        self.assertIn("must be unindexed", str(exc.exception))
 
     @unittest.skipUnless(scipy_available, "scipy is not available.")
     def test_remove(self):
@@ -923,7 +919,7 @@ def test_remove(self):
         # Say we know that these variables and constraints should
         # be matched...
         vars_to_remove = [model.F[0], model.F[2]]
-        cons_to_remove = (model.mbal[1], model.mbal[2])
+        cons_to_remove = [model.mbal[1], model.mbal[2]]
         igraph.remove_nodes(vars_to_remove, cons_to_remove)
         variable_set = ComponentSet(igraph.variables)
         self.assertNotIn(model.F[0], variable_set)
@@ -1309,7 +1305,7 @@ def test_remove(self):
         # matrix.
         vars_to_remove = [m.flow_comp[1]]
         cons_to_remove = [m.flow_eqn[1]]
-        igraph.remove_nodes(vars_to_remove + cons_to_remove)
+        igraph.remove_nodes(vars_to_remove, cons_to_remove)
         var_dmp, con_dmp = igraph.dulmage_mendelsohn()
         var_con_set = ComponentSet(igraph.variables + igraph.constraints)
         underconstrained_set = ComponentSet(
@@ -1460,6 +1456,42 @@ def test_remove_no_matrix(self):
         with self.assertRaisesRegex(RuntimeError, "no incidence matrix"):
             igraph.remove_nodes([m.v1])
 
+    def test_remove_bad_node(self):
+        m = pyo.ConcreteModel()
+        m.x = pyo.Var([1, 2, 3])
+        m.eq = pyo.Constraint(pyo.PositiveIntegers)
+        m.eq[1] = m.x[1] * m.x[2] == m.x[3]
+        m.eq[2] = m.x[1] + 2 * m.x[2] == 3 * m.x[3]
+        igraph = IncidenceGraphInterface(m)
+        with self.assertRaisesRegex(KeyError, "does not exist"):
+            # Suppose we think something like this should work. We should get
+            # an error, and not silently do nothing.
+            igraph.remove_nodes([m.x], [m.eq[1]])
+
+        with self.assertRaisesRegex(KeyError, "does not exist"):
+            igraph.remove_nodes(None, [m.eq])
+
+        with self.assertRaisesRegex(KeyError, "does not exist"):
+            igraph.remove_nodes([[m.x[1], m.x[2]], [m.eq[1]]])
+
+    def test_remove_varcon_samelist_deprecated(self):
+        m = pyo.ConcreteModel()
+        m.x = pyo.Var([1, 2, 3])
+        m.eq = pyo.Constraint(pyo.PositiveIntegers)
+        m.eq[1] = m.x[1] * m.x[2] == m.x[3]
+        m.eq[2] = m.x[1] + 2 * m.x[2] == 3 * m.x[3]
+
+        igraph = IncidenceGraphInterface(m)
+        # This raises a deprecation warning. When the deprecated functionality
+        # is removed, this will fail, and this test should be updated accordingly.
+        igraph.remove_nodes([m.eq[1], m.x[1]])
+        self.assertEqual(len(igraph.variables), 2)
+        self.assertEqual(len(igraph.constraints), 1)
+
+        igraph.remove_nodes([], [m.eq[2], m.x[2]])
+        self.assertEqual(len(igraph.variables), 1)
+        self.assertEqual(len(igraph.constraints), 0)
+
 
 @unittest.skipUnless(networkx_available, "networkx is not available.")
 @unittest.skipUnless(scipy_available, "scipy is not available.")
@@ -1840,7 +1872,7 @@ def test_var_elim(self):
         for adj_con in igraph.get_adjacent_to(m.x[1]):
             for adj_var in igraph.get_adjacent_to(m.eq4):
                 igraph.add_edge(adj_var, adj_con)
-        igraph.remove_nodes([m.x[1], m.eq4])
+        igraph.remove_nodes([m.x[1]], [m.eq4])
 
         assert ComponentSet(igraph.variables) == ComponentSet([m.x[2], m.x[3], m.x[4]])
         assert ComponentSet(igraph.constraints) == ComponentSet([m.eq1, m.eq2, m.eq3])