Merge branch 'main' into fix-pyros-subsolver-time-limits

Author: mrmundt

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):

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])

pyomo/contrib/pynumero/interfaces/pyomo_nlp.py

@@ -22,6 +22,7 @@
 import pyomo.core.base as pyo
 from pyomo.common.collections import ComponentMap
 from pyomo.common.env import CtypesEnviron
+from pyomo.solvers.amplfunc_merge import amplfunc_merge
 from ..sparse.block_matrix import BlockMatrix
 from pyomo.contrib.pynumero.interfaces.ampl_nlp import AslNLP
 from pyomo.contrib.pynumero.interfaces.nlp import NLP
@@ -92,15 +93,8 @@ def __init__(self, pyomo_model, nl_file_options=None):
             # The NL writer advertises the external function libraries
             # through the PYOMO_AMPLFUNC environment variable; merge it
             # with any preexisting AMPLFUNC definitions
-            amplfunc = "\n".join(
-                filter(
-                    None,
-                    (
-                        os.environ.get('AMPLFUNC', None),
-                        os.environ.get('PYOMO_AMPLFUNC', None),
-                    ),
-                )
-            )
+            amplfunc = amplfunc_merge(os.environ)
+
             with CtypesEnviron(AMPLFUNC=amplfunc):
                 super(PyomoNLP, self).__init__(nl_file)
 

pyomo/solvers/amplfunc_merge.py

@@ -0,0 +1,32 @@
+#  ___________________________________________________________________________
+#
+#  Pyomo: Python Optimization Modeling Objects
+#  Copyright (c) 2008-2024
+#  National Technology and Engineering Solutions of Sandia, LLC
+#  Under the terms of Contract DE-NA0003525 with National Technology and
+#  Engineering Solutions of Sandia, LLC, the U.S. Government retains certain
+#  rights in this software.
+#  This software is distributed under the 3-clause BSD License.
+#  ___________________________________________________________________________
+
+
+def amplfunc_string_merge(amplfunc, pyomo_amplfunc):
+    """Merge two AMPLFUNC variable strings eliminating duplicate lines"""
+    # Assume that the strings amplfunc and pyomo_amplfunc don't contain duplicates
+    # Assume that the path separator is correct for the OS so we don't need to
+    # worry about comparing Unix and Windows paths.
+    amplfunc_lines = amplfunc.split("\n")
+    existing = set(amplfunc_lines)
+    for line in pyomo_amplfunc.split("\n"):
+        # Skip lines we already have
+        if line not in existing:
+            amplfunc_lines.append(line)
+    # Remove empty lines which could happen if one or both of the strings is
+    # empty or there are two new lines in a row for whatever reason.
+    amplfunc_lines = [s for s in amplfunc_lines if s != ""]
+    return "\n".join(amplfunc_lines)
+
+
+def amplfunc_merge(env):
+    """Merge AMPLFUNC and PYOMO_AMPLFUNC in an environment var dict"""
+    return amplfunc_string_merge(env.get("AMPLFUNC", ""), env.get("PYOMO_AMPLFUNC", ""))

pyomo/solvers/plugins/solvers/ASL.py

@@ -23,6 +23,7 @@
 from pyomo.opt.solver import SystemCallSolver
 from pyomo.core.kernel.block import IBlock
 from pyomo.solvers.mockmip import MockMIP
+from pyomo.solvers.amplfunc_merge import amplfunc_merge
 from pyomo.core import TransformationFactory
 
 import logging
@@ -158,11 +159,9 @@ def create_command_line(self, executable, problem_files):
         # Pyomo/Pyomo) with any user-specified external function
         # libraries
         #
-        if 'PYOMO_AMPLFUNC' in env:
-            if 'AMPLFUNC' in env:
-                env['AMPLFUNC'] += "\n" + env['PYOMO_AMPLFUNC']
-            else:
-                env['AMPLFUNC'] = env['PYOMO_AMPLFUNC']
+        amplfunc = amplfunc_merge(env)
+        if amplfunc:
+            env['AMPLFUNC'] = amplfunc
 
         cmd = [executable, problem_files[0], '-AMPL']
         if self._timer:

pyomo/solvers/plugins/solvers/IPOPT.py

@@ -21,6 +21,8 @@
 from pyomo.opt.results import SolverStatus, SolverResults, TerminationCondition
 from pyomo.opt.solver import SystemCallSolver
 
+from pyomo.solvers.amplfunc_merge import amplfunc_merge
+
 import logging
 
 logger = logging.getLogger('pyomo.solvers')
@@ -119,11 +121,9 @@ def create_command_line(self, executable, problem_files):
         # Pyomo/Pyomo) with any user-specified external function
         # libraries
         #
-        if 'PYOMO_AMPLFUNC' in env:
-            if 'AMPLFUNC' in env:
-                env['AMPLFUNC'] += "\n" + env['PYOMO_AMPLFUNC']
-            else:
-                env['AMPLFUNC'] = env['PYOMO_AMPLFUNC']
+        amplfunc = amplfunc_merge(env)
+        if amplfunc:
+            env['AMPLFUNC'] = amplfunc
 
         cmd = [executable, problem_files[0], '-AMPL']
         if self._timer:

pyomo/solvers/plugins/solvers/gurobi_direct.py

@@ -493,9 +493,8 @@ def _add_constraint(self, con):
         if not con.active:
             return None
 
-        if is_fixed(con.body):
-            if self._skip_trivial_constraints:
-                return None
+        if self._skip_trivial_constraints and is_fixed(con.body):
+            return None
 
         conname = self._symbol_map.getSymbol(con, self._labeler)
 

pyomo/solvers/plugins/solvers/xpress_direct.py

@@ -667,9 +667,8 @@ def _add_constraint(self, con):
         if not con.active:
             return None
 
-        if is_fixed(con.body):
-            if self._skip_trivial_constraints:
-                return None
+        if self._skip_trivial_constraints and is_fixed(con.body):
+            return None
 
         conname = self._symbol_map.getSymbol(con, self._labeler)