Initialization Module#3912
Conversation
Co-authored-by: Miranda Mundt <55767766+mrmundt@users.noreply.github.com>
Co-authored-by: Miranda Mundt <55767766+mrmundt@users.noreply.github.com>
Co-authored-by: Miranda Mundt <55767766+mrmundt@users.noreply.github.com>
Co-authored-by: Miranda Mundt <55767766+mrmundt@users.noreply.github.com>
|
Looks like tests are failing due to a cplex release yesterday... |
blnicho
left a comment
There was a problem hiding this comment.
@michaelbynum I have a few questions and suggestions but overall I think this looks great and I'm excited to try it out!
| `Design of Experiments` | ||
| `MPC` | ||
| `AOS` | ||
| `NLP Initialization` |
There was a problem hiding this comment.
I don't think this change was necessary. I'm pretty sure this is just an old comment block we were using to track the documentation reorganization effort.
| # in all cases, try to solve the nlp before doing extra work | ||
| res = nlp_solver.solve( | ||
| nlp, load_solutions=False, raise_exception_on_nonoptimal_result=False | ||
| ) | ||
| logger.info(f'solved NLP: {res.solution_status}, {res.termination_condition}') | ||
|
|
||
| if res.solution_status == SolutionStatus.optimal: | ||
| res.solution_loader.load_vars() | ||
| logger.info('NLP solved without any initialization') | ||
| return res |
There was a problem hiding this comment.
What do you think about adding an option to skip this initial NLP solve? I feel like this could add a lot of extra time for complex models and I can picture a scenario where a user has already tried and failed to solve it directly so they turn to this initialization tool and have to wait again for the NLP solver to fail (which could take a while even with the default solver settings).
| lb = -1e6 | ||
| else: | ||
| lb = v.lb | ||
| if v.ub is None: | ||
| ub = 1e6 |
There was a problem hiding this comment.
Should these be using the default_bound instead of being hard-coded to 1e6?
There was a problem hiding this comment.
I don't think this code should exist. We set the bounds to default_bound before we ever get to this function. I'm going to delete it. Good catch.
jsiirola
left a comment
There was a problem hiding this comment.
This is really cool. Lots of comments / questions, but I don't think there was anything that should block merging.
| @@ -0,0 +1 @@ | |||
| The purpose of this module is to provide methods for initializing nonlinear programming models. No newline at end of file | |||
There was a problem hiding this comment.
Is this readme necessary? Should it just have a pointer to the documentation?
| constraints in the model, m. If variable bounds cannot be obtained, | ||
| we use default_bound. | ||
| """ | ||
| fbbt(m) |
There was a problem hiding this comment.
Is it worth only running FBBT on constraints that actually involve unbounded variables (instead of attempting to tighten all variables in the model)?
| return opt | ||
|
|
||
|
|
||
| def initialize_nlp( |
There was a problem hiding this comment.
Design question: would it better to make this a context manager instead of a "global gateway for all initialization routines"? The problem with the "gateway function" is that:
- it has to be updated every time we add a new strategy/method
- the API must be the superset of all the options / arguments that any method takes (and then there is the question about warning about arguments that the user provided, but are ignored by the chosen method)
Instead, if you made this a context manager, it might look like:
with initialization_context(m):
res = initialize_with_piecewise_linear_approximation(
nlp=m,
mip_solver="xpress",
max_iter=5,
)And InitializationContext could be as simple as
@context_manager
def initialization_context(nlp):
# get all variable bounds, domains, etc. to restore them later
orig_var_data = [
(v, v.lower, v.upper, v.domain, v.fixed, v.value) for v in get_vars(nlp)
]
yield
for v, lb, ub, domain, fixed, value in orig_var_data:
v.setlb(lb)
v.setub(ub)
v.domain = domain
if fixed:
assert v.value == value
assert v.fixed
else:
v.unfix()There was a problem hiding this comment.
I don't know how to ensure users use the context manager. However, I agree. I'm going to just put this code into helper functions and ensure all the initialization functions call them.
|
|
||
| # get all variable bounds, domains, etc. to restore them later | ||
| orig_vars = get_vars(nlp) | ||
| orig_var_data = ComponentMap( |
There was a problem hiding this comment.
This doesn't need to be a ComponentMap; a list is sufficient
| elif lb is None: | ||
| ps = m.slacks.add() | ||
| ps.setlb(0) | ||
| con.set_value(body - ub - ps <= 0) | ||
| elif ub is None: | ||
| ns = m.slacks.add() | ||
| ns.setlb(0) | ||
| con.set_value(body - lb + ns >= 0) |
There was a problem hiding this comment.
Will you get an exception if the constraint is unbounded? Maybe catch that case above with
if lb == ub:
if lb is not None:
ps = m.slacks.add()
# ...
else:
# trivial constraint; no slacks needed
pass| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _minimize_infeasibility(m): |
There was a problem hiding this comment.
This is basically a reimplementation of the core.add_slack_variables transformation. Why not just use it?
| _handlers[t] = _handle_node | ||
|
|
||
|
|
||
| class _PWLRefinementVisitor(StreamBasedExpressionVisitor): |
There was a problem hiding this comment.
I lost the thread on the logic here, but this seems to do way more expression manipulation than I would think is necessary. I would have thought this could have been done in one pass through the model where you substituted the nonlinear expressions with a new variable. If you kept a list of all the "new" constraints, you would have only had to rebuild the refined piecewise linearization of those expressions (no walker needed)?
| if val <= v.lb + 1e-6 + 1e-6 * abs(v.lb): | ||
| val += 1e-5 | ||
| if val >= v.ub - 1e-6 - 1e-6 * abs(v.ub): | ||
| val -= 1e-5 |
There was a problem hiding this comment.
Should these embedded constants be moved to either an argument or a global named constant?
|
|
||
| if len(violations) == 0: | ||
| raise RuntimeError( | ||
| 'Did not find any piecewise linear functions with variable values' |
There was a problem hiding this comment.
I do not understand this exception message. Can you clarify it?
| TerminationCondition, | ||
| ) | ||
| from pyomo.contrib.solver.common.base import Availability, SolverBase | ||
| import pytest |
There was a problem hiding this comment.
| import pytest | |
| from pyomo.common.unittest import pytest |
…tialization.rst Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
|
Thanks for all the feedback! Working on this now. |
Fixes #3878
Summary/Motivation:
This PR adds a module to
develcalledinitialization. The goal of the module is to provide methods to help initialize nonconvex nonlinear programming problems.Changes proposed in this PR:
initializationmoduleLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: