diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index 50cb84daaf5..0f47e03d0a2 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/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 --git a/pyomo/contrib/incidence_analysis/tests/test_interface.py b/pyomo/contrib/incidence_analysis/tests/test_interface.py index 4b77d60d8ba..b0a9661aa54 100644 --- a/pyomo/contrib/incidence_analysis/tests/test_interface.py +++ b/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])