-
Notifications
You must be signed in to change notification settings - Fork 571
PyROS Add caching for computed uncertain parameter bounds #3877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
53bbc30
b6a03aa
65e9564
a483ab6
549d5b8
bb7fca5
a6ef4f5
3a955f6
d003145
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -656,6 +656,7 @@ def validate(self, config): | |||||
| """ | ||||||
| Validate the uncertainty set with a nonemptiness | ||||||
| and boundedness check. | ||||||
| Clears any cached exact parameter bounds. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
|
|
@@ -667,6 +668,10 @@ def validate(self, config): | |||||
| ValueError | ||||||
| If nonemptiness check or boundedness check fails. | ||||||
| """ | ||||||
| # clear any cached exact parameter bounds | ||||||
| self._solve_bounds_optimization.cache_clear() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I anticipate that clearing the cache here will work for most use cases, but I am questioning whether this is the safest place to clear the cache. If, for example, the user implements a custom Perhaps cache clearing should be invoked in def solve(...):
... # everything up to disclaimer logging
... # <-- clear cache here
try:
# everything from argument validation
# to finalization of results object
...
finally:
... # <-- clear cache here
... # log termination-related messages
return ...If we proceed with this change, then we may want to add to
|
||||||
|
|
||||||
| # perform validation checks | ||||||
| if not self.is_nonempty(config=config): | ||||||
| raise ValueError(f"Nonemptiness check failed for uncertainty set {self}.") | ||||||
|
|
||||||
|
|
@@ -788,45 +793,78 @@ def _compute_exact_parameter_bounds(self, solver, index=None): | |||||
| if index is None: | ||||||
| index = [(True, True)] * self.dim | ||||||
|
|
||||||
| # create bounding model and get all objectives | ||||||
| bounding_model = self._create_bounding_model() | ||||||
| objs_to_optimize = bounding_model.param_var_objectives.items() | ||||||
|
|
||||||
| param_bounds = [] | ||||||
| for idx, obj in objs_to_optimize: | ||||||
| # activate objective for corresponding dimension | ||||||
| obj.activate() | ||||||
| for idx in range(self.dim): | ||||||
| bounds = [] | ||||||
|
|
||||||
| # solve for lower bound, then upper bound | ||||||
| # solve should be successful | ||||||
| for i, sense in enumerate((minimize, maximize)): | ||||||
| # check if the LB or UB should be solved | ||||||
| if not index[idx][i]: | ||||||
| bounds.append(None) | ||||||
| continue | ||||||
| obj.sense = sense | ||||||
| res = solver.solve(bounding_model, load_solutions=False) | ||||||
| if check_optimal_termination(res): | ||||||
| bounding_model.solutions.load_from(res) | ||||||
| else: | ||||||
| raise ValueError( | ||||||
| "Could not compute " | ||||||
| f"{'lower' if sense == minimize else 'upper'} " | ||||||
| f"bound in dimension {idx + 1} of {self.dim}. " | ||||||
| f"Solver status summary:\n {res.solver}." | ||||||
| ) | ||||||
| bounds.append(value(obj)) | ||||||
| bounds.append(self._solve_bounds_optimization(solver, idx, sense)) | ||||||
|
|
||||||
| # add parameter bounds for current dimension | ||||||
| param_bounds.append(tuple(bounds)) | ||||||
|
|
||||||
| # ensure sense is minimize when done, deactivate | ||||||
| obj.sense = minimize | ||||||
| obj.deactivate() | ||||||
|
|
||||||
| return param_bounds | ||||||
|
|
||||||
| @functools.cache | ||||||
| def _solve_bounds_optimization(self, solver, index, sense): | ||||||
| """ | ||||||
| Compute value of bounds for a single parameter | ||||||
| of `self` at a specified index by solving a bounding model. | ||||||
| Results are cached as efficiency for large uncertainty sets. | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| solver : ~pyomo.opt.base.solvers.OptSolver | ||||||
| Optimizer to invoke on the bounding problems. | ||||||
| index : int | ||||||
| The index of the parameter to solve for bounds. | ||||||
| sense : Pyomo objective sense | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| A Pyomo objective sense to optimize for the bounding model. | ||||||
| `maximize` solves for an upper bound and | ||||||
| `minimize` solves for a lower bound. | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| bound : float | ||||||
| A value of the lower or upper bound for | ||||||
| the corresponding dimension at the specified index. | ||||||
|
|
||||||
| Raises | ||||||
| ------ | ||||||
| ValueError | ||||||
| If solver failed to compute a bound for a | ||||||
| coordinate. | ||||||
| """ | ||||||
| # create bounding model and get all objectives | ||||||
| bounding_model = self._create_bounding_model() | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are re-creating the bounding model for every variable and both directions. This seems wasteful. Given that this is typically called in a loop, maybe the model should be created once outside the look and passed in to this function? That may foul up the use of @functools.cache, but we could just directly implement our own cache as an instance / model attribute. |
||||||
|
|
||||||
| # select objective corresponding to specified index | ||||||
| obj = bounding_model.param_var_objectives[index] | ||||||
| obj.activate() | ||||||
|
|
||||||
| # optimize with specified sense | ||||||
| obj.sense = sense | ||||||
| res = solver.solve(bounding_model, load_solutions=False) | ||||||
| if check_optimal_termination(res): | ||||||
| bounding_model.solutions.load_from(res) | ||||||
| else: | ||||||
| raise ValueError( | ||||||
| "Could not compute " | ||||||
| f"{'lower' if sense == minimize else 'upper'} " | ||||||
| f"bound in dimension {index + 1} of {self.dim}. " | ||||||
| f"Solver status summary:\n {res.solver}." | ||||||
| ) | ||||||
|
|
||||||
| # ensure sense is minimize when done, deactivate | ||||||
| obj.sense = minimize | ||||||
| obj.deactivate() | ||||||
|
Comment on lines
+861
to
+862
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the solve fails, the objective is left activated. I recommend moving this earlier: try:
res = solver.solve(bounding_model, load_solutions=False)
finally:
# ensure sense is minimize when done, deactivate
obj.sense = minimize
obj.deactivate() |
||||||
|
|
||||||
| bound = value(obj) | ||||||
|
|
||||||
| return bound | ||||||
|
|
||||||
| def _fbbt_parameter_bounds(self, config): | ||||||
| """ | ||||||
| Obtain parameter bounds of the uncertainty set using FBBT. | ||||||
|
|
@@ -858,7 +896,8 @@ def _fbbt_parameter_bounds(self, config): | |||||
| ) | ||||||
|
|
||||||
| param_bounds = [ | ||||||
| (var.lower, var.upper) for var in bounding_model.param_vars.values() | ||||||
| (value(var.lower), value(var.upper)) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider adding a test that is affected by this change. In light of the write-up for this PR, it may also be worth noting that as of PR #3733, the PyROS Uncertainty Sets documentation page explicitly states that mixed-integer uncertainty sets are not supported. More broadly, and perhaps in a future PR, we may want to make that documentation and/or the docstring for the |
||||||
| for var in bounding_model.param_vars.values() | ||||||
| ] | ||||||
|
|
||||||
| return param_bounds | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right place to clear the cache? This clears it at the very beginning of the PyROS algorithm, right? It should be empty now. Does it make more sense to clear the cache at the end of the algorithm so that we don't leave unnecessary models hanging around in memory?