From 01f7ebe58af5ded0325b028c462906d8aa2c4179 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Tue, 26 Mar 2024 16:14:05 -0600 Subject: [PATCH 1/7] require variables and constraints to be specified separately in `remove_nodes`; update to raise error on invalid components --- pyomo/contrib/incidence_analysis/interface.py | 55 +++++++++++++++---- .../tests/test_interface.py | 33 +++++++---- 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index 50cb84daaf5..0ed9b34b0f8 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/pyomo/contrib/incidence_analysis/interface.py @@ -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,48 @@ 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:: + + **Breaking change in Pyomo vTBD** + + The pre-TBD 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] + variables, constraints = self._validate_input(variables, constraints) + 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..3b2439ed2af 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,21 @@ 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]) + + with self.assertRaisesRegex(KeyError, "does not exist"): + igraph.remove_nodes([[m.x[1], m.x[2]], [m.eq[1]]]) + @unittest.skipUnless(networkx_available, "networkx is not available.") @unittest.skipUnless(scipy_available, "scipy is not available.") @@ -1840,7 +1851,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]) From 9a9c6843296b30bd45c0641c626736317cc785e2 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 3 Apr 2024 15:20:35 -0600 Subject: [PATCH 2/7] allow variables/constraints to be specified in same list in remove_nodes, but raise deprecation warning --- pyomo/contrib/incidence_analysis/interface.py | 28 +++++++++++++++++-- .../tests/test_interface.py | 18 ++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index 0ed9b34b0f8..f8d7ea855d4 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 @@ -911,7 +911,31 @@ def remove_nodes(self, variables=None, constraints=None): "Attempting to remove variables and constraints from cached " "incidence matrix,\nbut no incidence matrix has been cached." ) - variables, constraints = self._validate_input(variables, constraints) + + 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." + ) + for var in variables: + if var in self._con_index_map: + deprecation_warning(depr_msg, version="TBD") + cons_to_validate.append(var) + else: + vars_to_validate.append(var) + for con in constraints: + if con in self._var_index_map: + deprecation_warning(depr_msg, version="TBD") + 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] diff --git a/pyomo/contrib/incidence_analysis/tests/test_interface.py b/pyomo/contrib/incidence_analysis/tests/test_interface.py index 3b2439ed2af..816c8cbe3d3 100644 --- a/pyomo/contrib/incidence_analysis/tests/test_interface.py +++ b/pyomo/contrib/incidence_analysis/tests/test_interface.py @@ -1471,6 +1471,24 @@ def test_remove_bad_node(self): 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.") From 025e429ec66af22c0fa1fbaeae51f8d54a1d4421 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 3 Apr 2024 15:23:39 -0600 Subject: [PATCH 3/7] rephrase "breaking change" as "deprecation" --- pyomo/contrib/incidence_analysis/interface.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index f8d7ea855d4..64551788a8b 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/pyomo/contrib/incidence_analysis/interface.py @@ -891,7 +891,7 @@ def remove_nodes(self, variables=None, constraints=None): .. note:: - **Breaking change in Pyomo vTBD** + **Deprecation in Pyomo vTBD** The pre-TBD implementation of ``remove_nodes`` allowed variables and constraints to remove to be specified in a single list. This made From c7db40f70332d047fbb9197f8e2a9e694b4b34e6 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 3 Apr 2024 16:32:41 -0600 Subject: [PATCH 4/7] update remove_nodes tests for better coverage --- pyomo/contrib/incidence_analysis/tests/test_interface.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pyomo/contrib/incidence_analysis/tests/test_interface.py b/pyomo/contrib/incidence_analysis/tests/test_interface.py index 816c8cbe3d3..b0a9661aa54 100644 --- a/pyomo/contrib/incidence_analysis/tests/test_interface.py +++ b/pyomo/contrib/incidence_analysis/tests/test_interface.py @@ -1466,7 +1466,10 @@ def test_remove_bad_node(self): 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]) + 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]]]) From d6af9b3b2d809ed45796fee016c2961acc9ecd89 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 3 Apr 2024 20:42:18 -0600 Subject: [PATCH 5/7] only log deprecation warning once --- pyomo/contrib/incidence_analysis/interface.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index 64551788a8b..ce6c3633e8d 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/pyomo/contrib/incidence_analysis/interface.py @@ -920,15 +920,20 @@ def remove_nodes(self, variables=None, constraints=None): " 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: - deprecation_warning(depr_msg, version="TBD") cons_to_validate.append(var) else: vars_to_validate.append(var) for con in constraints: if con in self._var_index_map: - deprecation_warning(depr_msg, version="TBD") vars_to_validate.append(con) else: cons_to_validate.append(con) From ac75f8380b59b9981e6853a37879d753a8676ef4 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Wed, 3 Apr 2024 20:50:18 -0600 Subject: [PATCH 6/7] reformat --- pyomo/contrib/incidence_analysis/interface.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index ce6c3633e8d..8dee0539cb3 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/pyomo/contrib/incidence_analysis/interface.py @@ -920,9 +920,8 @@ def remove_nodes(self, variables=None, constraints=None): " 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) + 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. From 24d3baf9b174beab9f34f39e02010fc87194a193 Mon Sep 17 00:00:00 2001 From: Robert Parker Date: Thu, 4 Apr 2024 09:03:56 -0600 Subject: [PATCH 7/7] replace TBD with dev version in docstring --- pyomo/contrib/incidence_analysis/interface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyomo/contrib/incidence_analysis/interface.py b/pyomo/contrib/incidence_analysis/interface.py index 8dee0539cb3..0f47e03d0a2 100644 --- a/pyomo/contrib/incidence_analysis/interface.py +++ b/pyomo/contrib/incidence_analysis/interface.py @@ -891,9 +891,9 @@ def remove_nodes(self, variables=None, constraints=None): .. note:: - **Deprecation in Pyomo vTBD** + **Deprecation in Pyomo v6.7.2.dev0** - The pre-TBD implementation of ``remove_nodes`` allowed variables and + 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