PyROS Add caching for computed uncertain parameter bounds#3877
PyROS Add caching for computed uncertain parameter bounds#3877jas-yao wants to merge 18 commits intoPyomo:mainfrom
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.08% 90.08% -0.01%
==========================================
Files 904 904
Lines 106999 107013 +14
==========================================
+ Hits 96391 96402 +11
- Misses 10608 10611 +3
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.
|
@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. |
|
Thank you for all the detailed feedback @jsiirola and @shermanjasonaf . I have updated the caching implementation and added additional tests. This PR is ready for review. I had some questions come to mind when working on this and would appreciate any insights:
|
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(|
|
||
| # Define the uncertainty set | ||
| interval_cache = CustomExactBoundsUncertaintySet( | ||
| bounds=[(0.25, 2)], sleep_time=0.5, cache=True |
There was a problem hiding this comment.
Can this be made smaller (maybe 0.1)? I worry about how long this test takes (it's about 5 seconds).
| local_subsolver = SolverFactory("baron") | ||
| global_subsolver = SolverFactory("baron") |
There was a problem hiding this comment.
Do we really need to use baron here?
| local_subsolver = SolverFactory("baron") | ||
| global_subsolver = SolverFactory("baron") |
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: