From 1bccb1699bd504af1e851eb8ffdb61e152af6578 Mon Sep 17 00:00:00 2001 From: Miranda Mundt Date: Wed, 21 Feb 2024 08:03:57 -0700 Subject: [PATCH] Address comments from @jsiirola, @blnicho --- pyomo/common/tests/test_config.py | 1 - pyomo/contrib/appsi/base.py | 1 + pyomo/contrib/solver/base.py | 12 ++- pyomo/contrib/solver/config.py | 22 +++--- pyomo/contrib/solver/gurobi.py | 4 +- pyomo/contrib/solver/ipopt.py | 79 ++++--------------- pyomo/contrib/solver/persistent.py | 16 ++-- .../tests/solvers/test_gurobi_persistent.py | 2 +- .../solver/tests/solvers/test_solvers.py | 31 +++----- pyomo/contrib/solver/tests/unit/test_base.py | 16 ++-- pyomo/contrib/solver/tests/unit/test_ipopt.py | 20 +---- .../contrib/solver/tests/unit/test_results.py | 6 +- 12 files changed, 68 insertions(+), 142 deletions(-) diff --git a/pyomo/common/tests/test_config.py b/pyomo/common/tests/test_config.py index 02f4fc88251..0bbed43423d 100644 --- a/pyomo/common/tests/test_config.py +++ b/pyomo/common/tests/test_config.py @@ -25,7 +25,6 @@ # ___________________________________________________________________________ import argparse -import datetime import enum import os import os.path diff --git a/pyomo/contrib/appsi/base.py b/pyomo/contrib/appsi/base.py index 201e5975ac9..d028bdc4fde 100644 --- a/pyomo/contrib/appsi/base.py +++ b/pyomo/contrib/appsi/base.py @@ -597,6 +597,7 @@ def __init__( class Solver(abc.ABC): class Availability(enum.IntEnum): + """Docstring""" NotFound = 0 BadVersion = -1 BadLicense = -2 diff --git a/pyomo/contrib/solver/base.py b/pyomo/contrib/solver/base.py index 3bfa83050ad..f3d60bef03d 100644 --- a/pyomo/contrib/solver/base.py +++ b/pyomo/contrib/solver/base.py @@ -125,7 +125,7 @@ def solve(self, model: _BlockData, **kwargs) -> Results: """ @abc.abstractmethod - def available(self): + def available(self) -> bool: """Test if the solver is available on this system. Nominally, this will return True if the solver interface is @@ -159,7 +159,7 @@ def version(self) -> Tuple: A tuple representing the version """ - def is_persistent(self): + def is_persistent(self) -> bool: """ Returns ------- @@ -178,9 +178,7 @@ class PersistentSolverBase(SolverBase): Example usage can be seen in the Gurobi interface. """ - CONFIG = PersistentSolverConfig() - - @document_kwargs_from_configdict(CONFIG) + @document_kwargs_from_configdict(PersistentSolverConfig()) @abc.abstractmethod def solve(self, model: _BlockData, **kwargs) -> Results: super().solve(model, kwargs) @@ -312,7 +310,7 @@ def remove_variables(self, variables: List[_GeneralVarData]): """ @abc.abstractmethod - def remove_params(self, params: List[_ParamData]): + def remove_parameters(self, params: List[_ParamData]): """ Remove parameters from the model """ @@ -336,7 +334,7 @@ def update_variables(self, variables: List[_GeneralVarData]): """ @abc.abstractmethod - def update_params(self): + def update_parameters(self): """ Update parameters on the model """ diff --git a/pyomo/contrib/solver/config.py b/pyomo/contrib/solver/config.py index c91eb603b32..e60219a74b5 100644 --- a/pyomo/contrib/solver/config.py +++ b/pyomo/contrib/solver/config.py @@ -23,6 +23,7 @@ NonNegativeInt, ADVANCED_OPTION, Bool, + Path, ) from pyomo.common.log import LogStream from pyomo.common.numeric_types import native_logical_types @@ -74,17 +75,17 @@ def __init__( ConfigValue( domain=TextIO_or_Logger, default=False, - description="""`tee` accepts :py:class:`bool`, + description="""``tee`` accepts :py:class:`bool`, :py:class:`io.TextIOBase`, or :py:class:`logging.Logger` (or a list of these types). ``True`` is mapped to ``sys.stdout``. The solver log will be printed to each of - these streams / destinations. """, + these streams / destinations.""", ), ) - self.working_dir: Optional[str] = self.declare( + self.working_dir: Optional[Path] = self.declare( 'working_dir', ConfigValue( - domain=str, + domain=Path(), default=None, description="The directory in which generated files should be saved. " "This replaces the `keepfiles` option.", @@ -134,7 +135,8 @@ def __init__( self.time_limit: Optional[float] = self.declare( 'time_limit', ConfigValue( - domain=NonNegativeFloat, description="Time limit applied to the solver." + domain=NonNegativeFloat, + description="Time limit applied to the solver (in seconds).", ), ) self.solver_options: ConfigDict = self.declare( @@ -201,7 +203,7 @@ class AutoUpdateConfig(ConfigDict): check_for_new_objective: bool update_constraints: bool update_vars: bool - update_params: bool + update_parameters: bool update_named_expressions: bool update_objective: bool treat_fixed_vars_as_params: bool @@ -257,7 +259,7 @@ def __init__( description=""" If False, new/old parameters will not be automatically detected on subsequent solves. Use False only when manually updating the solver with opt.add_parameters() and - opt.remove_params() or when you are certain parameters are not being added to / + opt.remove_parameters() or when you are certain parameters are not being added to / removed from the model.""", ), ) @@ -297,15 +299,15 @@ def __init__( opt.update_variables() or when you are certain variables are not being modified.""", ), ) - self.update_params: bool = self.declare( - 'update_params', + self.update_parameters: bool = self.declare( + 'update_parameters', ConfigValue( domain=bool, default=True, description=""" If False, changes to parameter values will not be automatically detected on subsequent solves. Use False only when manually updating the solver with - opt.update_params() or when you are certain parameters are not being modified.""", + opt.update_parameters() or when you are certain parameters are not being modified.""", ), ) self.update_named_expressions: bool = self.declare( diff --git a/pyomo/contrib/solver/gurobi.py b/pyomo/contrib/solver/gurobi.py index 63387730c45..d0ac0d80f45 100644 --- a/pyomo/contrib/solver/gurobi.py +++ b/pyomo/contrib/solver/gurobi.py @@ -747,7 +747,7 @@ def _remove_variables(self, variables: List[_GeneralVarData]): self._mutable_bounds.pop(v_id, None) self._needs_updated = True - def _remove_params(self, params: List[_ParamData]): + def _remove_parameters(self, params: List[_ParamData]): pass def _update_variables(self, variables: List[_GeneralVarData]): @@ -770,7 +770,7 @@ def _update_variables(self, variables: List[_GeneralVarData]): gurobipy_var.setAttr('vtype', vtype) self._needs_updated = True - def update_params(self): + def update_parameters(self): for con, helpers in self._mutable_helpers.items(): for helper in helpers: helper.update() diff --git a/pyomo/contrib/solver/ipopt.py b/pyomo/contrib/solver/ipopt.py index 3e911aea036..ad12e26ee92 100644 --- a/pyomo/contrib/solver/ipopt.py +++ b/pyomo/contrib/solver/ipopt.py @@ -13,16 +13,10 @@ import subprocess import datetime import io -import sys from typing import Mapping, Optional, Sequence from pyomo.common import Executable -from pyomo.common.config import ( - ConfigValue, - NonNegativeFloat, - document_kwargs_from_configdict, - ConfigDict, -) +from pyomo.common.config import ConfigValue, document_kwargs_from_configdict, ConfigDict from pyomo.common.errors import PyomoException, DeveloperError from pyomo.common.tempfiles import TempfileManager from pyomo.common.timing import HierarchicalTimer @@ -78,54 +72,7 @@ def __init__( ), ) self.writer_config: ConfigDict = self.declare( - 'writer_config', NLWriter.CONFIG() - ) - - -class IpoptResults(Results): - def __init__( - self, - description=None, - doc=None, - implicit=False, - implicit_domain=None, - visibility=0, - ): - super().__init__( - description=description, - doc=doc, - implicit=implicit, - implicit_domain=implicit_domain, - visibility=visibility, - ) - self.timing_info.ipopt_excluding_nlp_functions: Optional[float] = ( - self.timing_info.declare( - 'ipopt_excluding_nlp_functions', - ConfigValue( - domain=NonNegativeFloat, - default=None, - description="Total CPU seconds in IPOPT without function evaluations.", - ), - ) - ) - self.timing_info.nlp_function_evaluations: Optional[float] = ( - self.timing_info.declare( - 'nlp_function_evaluations', - ConfigValue( - domain=NonNegativeFloat, - default=None, - description="Total CPU seconds in NLP function evaluations.", - ), - ) - ) - self.timing_info.total_seconds: Optional[float] = self.timing_info.declare( - 'total_seconds', - ConfigValue( - domain=NonNegativeFloat, - default=None, - description="Total seconds in IPOPT. NOTE: Newer versions of IPOPT (3.14+) " - "no longer separate timing information.", - ), + 'writer_config', ConfigValue(default=NLWriter.CONFIG(), description="Configuration that controls options in the NL writer.") ) @@ -416,11 +363,11 @@ def solve(self, model, **kwds): if len(nl_info.variables) == 0: if len(nl_info.eliminated_vars) == 0: - results = IpoptResults() + results = Results() results.termination_condition = TerminationCondition.emptyModel results.solution_loader = SolSolutionLoader(None, None) else: - results = IpoptResults() + results = Results() results.termination_condition = ( TerminationCondition.convergenceCriteriaSatisfied ) @@ -435,18 +382,22 @@ def solve(self, model, **kwds): results = self._parse_solution(sol_file, nl_info) timer.stop('parse_sol') else: - results = IpoptResults() + results = Results() if process.returncode != 0: results.extra_info.return_code = process.returncode results.termination_condition = TerminationCondition.error results.solution_loader = SolSolutionLoader(None, None) else: results.iteration_count = iters - results.timing_info.ipopt_excluding_nlp_functions = ( - ipopt_time_nofunc - ) - results.timing_info.nlp_function_evaluations = ipopt_time_func - results.timing_info.total_seconds = ipopt_total_time + if ipopt_time_nofunc is not None: + results.timing_info.ipopt_excluding_nlp_functions = ( + ipopt_time_nofunc + ) + + if ipopt_time_func is not None: + results.timing_info.nlp_function_evaluations = ipopt_time_func + if ipopt_total_time is not None: + results.timing_info.total_seconds = ipopt_total_time if ( config.raise_exception_on_nonoptimal_result and results.solution_status != SolutionStatus.optimal @@ -554,7 +505,7 @@ def _parse_ipopt_output(self, stream: io.StringIO): return iters, nofunc_time, func_time, total_time def _parse_solution(self, instream: io.TextIOBase, nl_info: NLWriterInfo): - results = IpoptResults() + results = Results() res, sol_data = parse_sol_file( sol_file=instream, nl_info=nl_info, result=results ) diff --git a/pyomo/contrib/solver/persistent.py b/pyomo/contrib/solver/persistent.py index e389e5d4019..4b1a7c58dcd 100644 --- a/pyomo/contrib/solver/persistent.py +++ b/pyomo/contrib/solver/persistent.py @@ -274,11 +274,11 @@ def remove_variables(self, variables: List[_GeneralVarData]): del self._vars[v_id] @abc.abstractmethod - def _remove_params(self, params: List[_ParamData]): + def _remove_parameters(self, params: List[_ParamData]): pass - def remove_params(self, params: List[_ParamData]): - self._remove_params(params) + def remove_parameters(self, params: List[_ParamData]): + self._remove_parameters(params) for p in params: del self._params[id(p)] @@ -297,7 +297,7 @@ def remove_block(self, block): ) ) ) - self.remove_params( + self.remove_parameters( list( dict( (id(p), p) @@ -325,7 +325,7 @@ def update_variables(self, variables: List[_GeneralVarData]): self._update_variables(variables) @abc.abstractmethod - def update_params(self): + def update_parameters(self): pass def update(self, timer: HierarchicalTimer = None): @@ -396,12 +396,12 @@ def update(self, timer: HierarchicalTimer = None): self.remove_sos_constraints(old_sos) timer.stop('cons') timer.start('params') - self.remove_params(old_params) + self.remove_parameters(old_params) # sticking this between removal and addition # is important so that we don't do unnecessary work - if config.update_params: - self.update_params() + if config.update_parameters: + self.update_parameters() self.add_parameters(new_params) timer.stop('params') diff --git a/pyomo/contrib/solver/tests/solvers/test_gurobi_persistent.py b/pyomo/contrib/solver/tests/solvers/test_gurobi_persistent.py index f2dd79619b4..2f281e2abf0 100644 --- a/pyomo/contrib/solver/tests/solvers/test_gurobi_persistent.py +++ b/pyomo/contrib/solver/tests/solvers/test_gurobi_persistent.py @@ -487,7 +487,7 @@ def setUp(self): opt.config.auto_updates.check_for_new_or_removed_params = False opt.config.auto_updates.check_for_new_or_removed_vars = False opt.config.auto_updates.check_for_new_or_removed_constraints = False - opt.config.auto_updates.update_params = False + opt.config.auto_updates.update_parameters = False opt.config.auto_updates.update_vars = False opt.config.auto_updates.update_constraints = False opt.config.auto_updates.update_named_expressions = False diff --git a/pyomo/contrib/solver/tests/solvers/test_solvers.py b/pyomo/contrib/solver/tests/solvers/test_solvers.py index c6c73ea2dc7..cf5f6cf5c57 100644 --- a/pyomo/contrib/solver/tests/solvers/test_solvers.py +++ b/pyomo/contrib/solver/tests/solvers/test_solvers.py @@ -9,23 +9,24 @@ # This software is distributed under the 3-clause BSD License. # ___________________________________________________________________________ +import random +import math +from typing import Type + import pyomo.environ as pe +from pyomo import gdp from pyomo.common.dependencies import attempt_import import pyomo.common.unittest as unittest - -parameterized, param_available = attempt_import('parameterized') -parameterized = parameterized.parameterized from pyomo.contrib.solver.results import TerminationCondition, SolutionStatus, Results from pyomo.contrib.solver.base import SolverBase from pyomo.contrib.solver.ipopt import Ipopt from pyomo.contrib.solver.gurobi import Gurobi -from typing import Type from pyomo.core.expr.numeric_expr import LinearExpression -import math -numpy, numpy_available = attempt_import('numpy') -import random -from pyomo import gdp + +np, numpy_available = attempt_import('numpy') +parameterized, param_available = attempt_import('parameterized') +parameterized = parameterized.parameterized if not param_available: @@ -802,10 +803,6 @@ def test_mutable_param_with_range( opt.config.writer_config.linear_presolve = True else: opt.config.writer_config.linear_presolve = False - try: - import numpy as np - except: - raise unittest.SkipTest('numpy is not available') m = pe.ConcreteModel() m.x = pe.Var() m.y = pe.Var() @@ -907,7 +904,7 @@ def test_add_and_remove_vars( m.y = pe.Var(bounds=(-1, None)) m.obj = pe.Objective(expr=m.y) if opt.is_persistent(): - opt.config.auto_updates.update_params = False + opt.config.auto_updates.update_parameters = False opt.config.auto_updates.update_vars = False opt.config.auto_updates.update_constraints = False opt.config.auto_updates.update_named_expressions = False @@ -1003,14 +1000,10 @@ def test_with_numpy( a2 = -2 b2 = 1 m.c1 = pe.Constraint( - expr=(numpy.float64(0), m.y - numpy.int64(1) * m.x - numpy.float32(3), None) + expr=(np.float64(0), m.y - np.int64(1) * m.x - np.float32(3), None) ) m.c2 = pe.Constraint( - expr=( - None, - -m.y + numpy.int32(-2) * m.x + numpy.float64(1), - numpy.float16(0), - ) + expr=(None, -m.y + np.int32(-2) * m.x + np.float64(1), np.float16(0)) ) res = opt.solve(m) self.assertEqual(res.solution_status, SolutionStatus.optimal) diff --git a/pyomo/contrib/solver/tests/unit/test_base.py b/pyomo/contrib/solver/tests/unit/test_base.py index a9b3e4f4711..74c495b86cc 100644 --- a/pyomo/contrib/solver/tests/unit/test_base.py +++ b/pyomo/contrib/solver/tests/unit/test_base.py @@ -80,7 +80,7 @@ def test_custom_solver_name(self): class TestPersistentSolverBase(unittest.TestCase): def test_abstract_member_list(self): expected_list = [ - 'remove_params', + 'remove_parameters', 'version', 'update_variables', 'remove_variables', @@ -88,7 +88,7 @@ def test_abstract_member_list(self): '_get_primals', 'set_instance', 'set_objective', - 'update_params', + 'update_parameters', 'remove_block', 'add_block', 'available', @@ -116,12 +116,12 @@ def test_class_method_list(self): 'is_persistent', 'remove_block', 'remove_constraints', - 'remove_params', + 'remove_parameters', 'remove_variables', 'set_instance', 'set_objective', 'solve', - 'update_params', + 'update_parameters', 'update_variables', 'version', ] @@ -142,12 +142,12 @@ def test_init(self): self.assertEqual(self.instance.add_constraints(None), None) self.assertEqual(self.instance.add_block(None), None) self.assertEqual(self.instance.remove_variables(None), None) - self.assertEqual(self.instance.remove_params(None), None) + self.assertEqual(self.instance.remove_parameters(None), None) self.assertEqual(self.instance.remove_constraints(None), None) self.assertEqual(self.instance.remove_block(None), None) self.assertEqual(self.instance.set_objective(None), None) self.assertEqual(self.instance.update_variables(None), None) - self.assertEqual(self.instance.update_params(), None) + self.assertEqual(self.instance.update_parameters(), None) with self.assertRaises(NotImplementedError): self.instance._get_primals() @@ -168,12 +168,12 @@ def test_context_manager(self): self.assertEqual(self.instance.add_constraints(None), None) self.assertEqual(self.instance.add_block(None), None) self.assertEqual(self.instance.remove_variables(None), None) - self.assertEqual(self.instance.remove_params(None), None) + self.assertEqual(self.instance.remove_parameters(None), None) self.assertEqual(self.instance.remove_constraints(None), None) self.assertEqual(self.instance.remove_block(None), None) self.assertEqual(self.instance.set_objective(None), None) self.assertEqual(self.instance.update_variables(None), None) - self.assertEqual(self.instance.update_params(), None) + self.assertEqual(self.instance.update_parameters(), None) class TestLegacySolverWrapper(unittest.TestCase): diff --git a/pyomo/contrib/solver/tests/unit/test_ipopt.py b/pyomo/contrib/solver/tests/unit/test_ipopt.py index eff8787592e..cc459245506 100644 --- a/pyomo/contrib/solver/tests/unit/test_ipopt.py +++ b/pyomo/contrib/solver/tests/unit/test_ipopt.py @@ -44,7 +44,7 @@ def test_custom_instantiation(self): config.tee = True self.assertTrue(config.tee) self.assertEqual(config._description, "A description") - self.assertFalse(config.time_limit) + self.assertIsNone(config.time_limit) # Default should be `ipopt` self.assertIsNotNone(str(config.executable)) self.assertIn('ipopt', str(config.executable)) @@ -54,24 +54,6 @@ def test_custom_instantiation(self): self.assertFalse(config.executable.available()) -class TestIpoptResults(unittest.TestCase): - def test_default_instantiation(self): - res = ipopt.IpoptResults() - # Inherited methods/attributes - self.assertIsNone(res.solution_loader) - self.assertIsNone(res.incumbent_objective) - self.assertIsNone(res.objective_bound) - self.assertIsNone(res.solver_name) - self.assertIsNone(res.solver_version) - self.assertIsNone(res.iteration_count) - self.assertIsNone(res.timing_info.start_timestamp) - self.assertIsNone(res.timing_info.wall_time) - # Unique to this object - self.assertIsNone(res.timing_info.ipopt_excluding_nlp_functions) - self.assertIsNone(res.timing_info.nlp_function_evaluations) - self.assertIsNone(res.timing_info.total_seconds) - - class TestIpoptSolutionLoader(unittest.TestCase): def test_get_reduced_costs_error(self): loader = ipopt.IpoptSolutionLoader(None, None) diff --git a/pyomo/contrib/solver/tests/unit/test_results.py b/pyomo/contrib/solver/tests/unit/test_results.py index 4856b737295..74404aaba4c 100644 --- a/pyomo/contrib/solver/tests/unit/test_results.py +++ b/pyomo/contrib/solver/tests/unit/test_results.py @@ -21,7 +21,7 @@ from pyomo.contrib.solver import results from pyomo.contrib.solver import solution import pyomo.environ as pyo -from pyomo.core.base.var import ScalarVar +from pyomo.core.base.var import Var class SolutionLoaderExample(solution.SolutionLoaderBase): @@ -213,8 +213,8 @@ def test_display(self): def test_generated_results(self): m = pyo.ConcreteModel() - m.x = ScalarVar() - m.y = ScalarVar() + m.x = Var() + m.y = Var() m.c1 = pyo.Constraint(expr=m.x == 1) m.c2 = pyo.Constraint(expr=m.y == 2)