PyROS Add caching for computed uncertain parameter bounds#3877
PyROS Add caching for computed uncertain parameter bounds#3877jas-yao wants to merge 30 commits into
Conversation
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
This looks pretty good. Some questions and a couple minor suggestions.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3877 +/- ##
=======================================
Coverage 90.11% 90.12%
=======================================
Files 905 905
Lines 107502 107522 +20
=======================================
+ Hits 96878 96899 +21
+ Misses 10624 10623 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: John Siirola <jsiirola@users.noreply.github.com>
shermanjasonaf
left a comment
shermanjasonaf
left a comment
There was a problem hiding this comment.
Good PR. I have a few questions about edge cases and testing.
|
|
||
| param_bounds = [ | ||
| (var.lower, var.upper) for var in bounding_model.param_vars.values() | ||
| (value(var.lower), value(var.upper)) |
There was a problem hiding this comment.
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 UncertaintySet.set_as_constraint() method more specific/explicit about can/can't be done to model components within that method. (E.g., should altering the domains of the uncertain parameter Var objects be generally supported?)
There was a problem hiding this comment.
I have added a test for this.
Yes, I noticed that the FBBT issue comes up when uncertain parameter Var objects are given domains (e.g., Binary, NonNegativeReals, UnitInterval, etc.) and bounds at the same time. Setting bounds and domains leads to an expression object being returned by var.lower and var.upper rather than a value.
I think it is reasonable to not support altering the domains of uncertain parameters if they can already be specified through setting parameter bounds.
There was a problem hiding this comment.
@jas-yao: Note that var.lower returns the expression for the lower bound of the variable. For cases where the bound is defined by a Param, then you get the Param back. If the bound is determined by both the bound from bounds= and the implied bound from the domain, then you will get a max expression back. The same applies to the upper bound.
If you always want the current numerical bound, then use var.lb / var.ub
There was a problem hiding this comment.
Understood. I have made this change to use var.lb / var.ub. It will be included in my next push.
|
@jas-yao: we are adopting a new review process where we convert PRs that are "waiting on the author" back to "draft" (to signal the PR state to both the author and the dev team). Once you have had a chance to address the comments, please mark it as "ready for review" so the developers can get it back into the review queue. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Some more minor questions,a nd a potential significant design simplification: Instead of putting everything inside another context block, you could use a custom dict that supports context management:
diff --git a/pyomo/contrib/pyros/uncertainty_sets.py b/pyomo/contrib/pyros/uncertainty_sets.py
index a925ab1939..8a056e05b2 100644
--- a/pyomo/contrib/pyros/uncertainty_sets.py
+++ b/pyomo/contrib/pyros/uncertainty_sets.py
@@ -460,6 +460,15 @@ def validate_array(
)
+class ContextDict(dict):
+ def __enter__(self):
+ assert not self
+ return self
+
+ def __exit__(self, et, e, tb):
+ self.clear()
+
+
class Geometry(Enum):
"""
Geometry classifications for PyROS uncertainty set objects.
@@ -524,6 +533,14 @@ class UncertaintySet(object, metaclass=abc.ABCMeta):
"""
raise NotImplementedError
+ @property
+ def _cache(self):
+ try:
+ return self.__cache
+ except AttributeError:
+ self.__cache = ContextDict()
+ return self.__cache
+
def _create_bounding_model(self):
"""
Make uncertain parameter value bounding problems (optimizeThen you could just include the cache management in with the main timer:
diff --git a/pyomo/contrib/pyros/pyros.py b/pyomo/contrib/pyros/pyros.py
index 38104e98fa..c9fb37e611 100644
--- a/pyomo/contrib/pyros/pyros.py
+++ b/pyomo/contrib/pyros/pyros.py
@@ -386,10 +386,13 @@ class PyROS(object):
)
model_data = ModelData(original_model=model, timing=TimingData(), config=None)
- with time_code(
- timing_data_obj=model_data.timing,
- code_block_name="main",
- is_main_timer=True,
+ with (
+ uncertainty_set._cache,
+ time_code(
+ timing_data_obj=model_data.timing,
+ code_block_name="main",
+ is_main_timer=True,
+ ),
):
kwds.update(
dict(
|
@jsiirola Thanks for all the suggestions. I have added the A small difference was that I implemented class ContextDict(dict):
def __enter__(self):
self.clear()
return self
def __exit__(self, et, e, tb):
self.clear() |
|
|
||
| class ContextDict(dict): | ||
| def __enter__(self): | ||
| self.clear() |
There was a problem hiding this comment.
The concern I have around clearing this here, is that the cache should only be non-empty if we are already in the middle of a PyROS solve. That should never happen (hence why I originally suggested checking that with an assertion). With this implementation, the cache will be cleared out from under whatever was using it, and would end up with a) we would never know the assumption of only one PyROS using the cache at a time was being violated, and b) whatever was using the cache could unexpectedly fail in a very hard-to-debug manner.
There was a problem hiding this comment.
Thanks for this feedback. I'll change it back to the assertion then.
To give a bit more context, the idea to clear in __enter__ is if the user had modified the cache or performed bounds calculations and stored them in the cache of the uncertainty set before running PyROS with it.
The tests in test_pyros_cache_solutions are to check that clearing the cache prevents keeps PyROS from using incorrect/outdated cached bounds to end up at a wrong solution.
I think that checking with an assertion should have no problems, as it does the same thing in preventing incorrectly cached bounds from messing with PyROS, along with guarding from unexpected scenarios. There should be no issues in changing the uncertainty set within a loop as well, since the cache is cleared after every run. I think the main trigger for the assertion error would be caused by someone experimenting with a custom uncertainty set.
How does the following look for the error message?
class ContextDict(dict):
def __enter__(self):
assert not self, "Uncertainty set cache has been modified."
return self
def __exit__(self, et, e, tb):
self.clear()
|
@jas-yao @jsiirola As PR #3927 has been merged ahead of this PR, we will need to add bounds caching for the new
|
|
@shermanjasonaf I vote for option 2 and doing a single update to the PyROS version and changelog |
shermanjasonaf
left a comment
shermanjasonaf
left a comment
There was a problem hiding this comment.
After the more recent changes, this PR looks good overall. I have comments on functionality for IntersectionSet and CartesianProductSet cases. I also have comments on testing and several suggestions on documentation.
@jas-yao To address the second question of your earlier comment: I do not think that we should make UncertaintySet.parameter_bounds return the results of UncertaintySet._compute_exact_parameter_bounds() by default, for a few reasons:
_compute_exact_parameter_bounds()formally requires the user to provide a solver, whereasparameter_boundsdoes not.parameter_boundsis a property (basically an attribute), so we may want the calculations performed within to be somewhat quick. In contrast,_compute_exact_parameter_bounds()is generally slow when the bounds are not cached, due to the time required to construct and repeatedly solve the bounding model.parameter_boundsis public (and therefore accessible). If the default behavior ofparameter_boundsis changed in the suggested way, then accessingparameter_boundsresults by default in population of the cache, a protected attribute that must be clear(ed) ahead of callingPyROS.solve().
|
|
||
| class ContextDict(dict): | ||
| def __enter__(self): | ||
| assert not self, "Uncertainty set cache has been modified." |
There was a problem hiding this comment.
Consider modifying this to the effect of:
| assert not self, "Uncertainty set cache has been modified." | |
| assert not self, "Nonempty cache for uncertainty set's exact parameter bounds." |
| raise NotImplementedError | ||
|
|
||
| @property | ||
| def _cache(self): |
There was a problem hiding this comment.
Consider documenting this attribute by adding here a docstring to the effect of:
"""
dict : Cache for the bounds defining the minimum bounding box
of `self`. Each key is a 2-tuple containing the positional
index of a coordinate and an
:class:`~pyomo.common.enums.ObjectiveSense` object
indicating the type of bound (`minimize` for a lower bound,
`maximize for an upper bound) being specified for the
coordinate.
"""
| Compute value of bounds for a single parameter | ||
| of `self` at a specified index by solving a bounding model. | ||
| Results are cached as efficiency. |
There was a problem hiding this comment.
| Compute value of bounds for a single parameter | |
| of `self` at a specified index by solving a bounding model. | |
| Results are cached as efficiency. | |
| Compute an exact lower or upper bound for a specified | |
| coordinate of the points contained in `self`, by solving | |
| a bounding model. | |
| For efficiency, the result is cached if the bounding model | |
| is solved successfully. Further, if the cache already contains | |
| an entry corresponding to the coordinate bound of interest, | |
| then that entry is returned and solution of the bounding model | |
| is skipped. |
| Objective with name 'param_var_objectives' consisting | ||
| of `N` entries, all of which have been deactivated. | ||
| index : int | ||
| The index of the parameter to solve for bounds. |
There was a problem hiding this comment.
| The index of the parameter to solve for bounds. | |
| The positional index for the coordinate of interest. |
| `maximize` solves for an upper bound and | ||
| `minimize` solves for a lower bound. | ||
| solver : ~pyomo.opt.base.solvers.OptSolver | ||
| Optimizer to invoke on the bounding problems. |
There was a problem hiding this comment.
| Optimizer to invoke on the bounding problems. | |
| Optimizer to invoke on the bounding model. |
|
|
||
| # validate each set | ||
| for a_set in the_sets: | ||
| a_set.validate(config) |
There was a problem hiding this comment.
Isn't it possible that a_set._cache gets populated here, particularly for cases where calling a_set.validate() entails calling a_set._compute_exact_parameter_bounds()? Do we need to be careful to clear a_set._cache here?
| def test_pyros_cache_solutions(self): | ||
| """ | ||
| Check that PyROS clears cache before/after and yields accurate results. | ||
| """ |
There was a problem hiding this comment.
The test name and docstring here can be modified for consistency with what this test is checking. It seems this test checks that an AssertionError is raised by ContextDict.__enter__() if the cache is nonempty when PyROS is called.
| def test_pyros_cache_solutions(self): | |
| """ | |
| Check that PyROS clears cache before/after and yields accurate results. | |
| """ | |
| def test_pyros_solve_assert_bounds_cache_empty(self): | |
| """ | |
| Check that calling PyROS on an uncertainty set | |
| with a nonempty bounds cache results in an exception. | |
| """ |
| self.assertTrue(hasattr(interval, "_cache")) | ||
|
|
||
| # Call the PyROS solver | ||
| results = pyros_solver.solve( |
There was a problem hiding this comment.
Since results is not used after this line:
| results = pyros_solver.solve( | |
| pyros_solver.solve( |
| # Solve with PyROS | ||
| exc_str = r"Uncertainty set cache has been modified." | ||
| with self.assertRaisesRegex(AssertionError, exc_str): | ||
| results = pyros_solver.solve( |
There was a problem hiding this comment.
Since results is not used after this line:
| results = pyros_solver.solve( | |
| pyros_solver.solve( |
| # Solve with PyROS | ||
| exc_str = r"Uncertainty set cache has been modified." | ||
| with self.assertRaisesRegex(AssertionError, exc_str): | ||
| results = pyros_solver.solve( |
There was a problem hiding this comment.
Since results is not used after this line:
| results = pyros_solver.solve( | |
| pyros_solver.solve( |
5 participants
Fixes
_fbbt_parameter_boundsinuncertainty_sets.pySummary/Motivation:
PyROS solves optimization bounding problems for every uncertain parameter multiple times throughout its routine using
_compute_exact_parameter_bounds. There are up to 4 instances of PyROS accessing this method during its runtime.is_bounded. This occurs when no parameter bounds are provided and FBBT fails to find bounds. Only the bounds that FBBT has not found are evaluated.is_nonempty. This occurs for intersection, polyhedral, and custom uncertainty sets, where a feasibility problem is constructed.get_effective_uncertain_dimensionsif parameter bounds are not provided or the provided ones are not exact.add_uncertainty_set_constraintsif no parameter bounds are provided.The time taken to repeatedly solve these bounding problems may add up and be significant for larger uncertainty sets.
This PR adds a method for caching the solutions of these bounding problems so that subsequent processes do not need to solve the bounding problems again.
This PR also fixes a small bug in
_fbbt_parameter_boundswhere the returned bounds are not a float (e.g., a binary variablem.Var = pyo.Var(within=pyo.Binary, bounds=(0,1))or a binary variable in a model that FBBT has been used on has lower bound max(0,0) and upper bound min(1,1)).Changes proposed in this PR:
_solve_bounds_optimizationmethod that usesfunctools.cacheto cache solutions for bounding problems solved for any uncertain parameter that is used by_compute_exact_parameter_bounds_solve_bounds_optimizationcache during validation, which is run at the start of every PyROS solve._solve_bounds_optimization_fbbt_parameter_boundsLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: