PyROS Add caching for computed uncertain parameter bounds

[jas-yao]

Fixes

_fbbt_parameter_bounds in uncertainty_sets.py

Summary/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.

1. Validation in 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.
2. Validation in is_nonempty. This occurs for intersection, polyhedral, and custom uncertainty sets, where a feasibility problem is constructed.
3. Preprocessing in get_effective_uncertain_dimensions if parameter bounds are not provided or the provided ones are not exact.
4. Separation problem construction in add_uncertainty_set_constraints if 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_bounds where the returned bounds are not a float (e.g., a binary variable m.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:

• Add a _solve_bounds_optimization method that uses functools.cache to cache solutions for bounding problems solved for any uncertain parameter that is used by _compute_exact_parameter_bounds
• Add a line for clearing the _solve_bounds_optimization cache during validation, which is run at the start of every PyROS solve.
• Add tests for checking caching behavior of _solve_bounds_optimization
• Fix the issue in _fbbt_parameter_bounds

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[jas-yao]

@shermanjasonaf @jsiirola

[jsiirola]

This looks pretty good. Some questions and a couple minor suggestions.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (3665c13) to head (c4268de).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3877   +/-   ##
=======================================
  Coverage   90.12%   90.12%           
=======================================
  Files         905      905           
  Lines      107648   107682   +34     
=======================================
+ Hits        97021    97052   +31     
- Misses      10627    10630    +3     

Flag	Coverage Δ
builders	29.12% <21.73%> (+<0.01%)	⬆️
default	86.47% <100.00%> (?)
expensive	35.18% <21.73%> (?)
linux	87.61% <100.00%> (-2.01%)	⬇️
linux_other	87.61% <100.00%> (+<0.01%)	⬆️
oldsolvers	28.07% <21.73%> (+<0.01%)	⬆️
osx	83.00% <100.00%> (+<0.01%)	⬆️
win	85.53% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[shermanjasonaf]

Good PR. I have a few questions about edge cases and testing.

[jsiirola]

@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.

[blnicho]

@shermanjasonaf I vote for option 2 and doing a single update to the PyROS version and changelog

[shermanjasonaf]

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, whereas parameter_bounds does not.
• parameter_bounds is 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_bounds is public (and therefore accessible). If the default behavior of parameter_bounds is changed in the suggested way, then accessing parameter_bounds results by default in population of the cache, a protected attribute that must be clear(ed) ahead of calling PyROS.solve().

[shermanjasonaf]

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?

[jsiirola]

@shermanjasonaf @jas-yao: I pushed the updated cache context manager. Are we good to "final review" and merge? (@shermanjasonaf: are you satisfied with how caching interacts with the CartesianProductSet, or do you want to wait for additional testing?)

[shermanjasonaf]

@jsiirola @jas-yao I think we are close to merging. The new cache context manager works nicely. I am satisfied with how caching interacts with the CartesianProductSet; my comments include suggestions for additional testing. We may also want a similar custom implementation for IntersectionSet._cache_manager(), as bounds caching of the operand UncertaintySet attributes may also occur in some (seemingly rare) cases.

[shermanjasonaf]

This works nicely. Should IntersectionSet._cache_manager() be implemented similarly, to ensure that the bounds caches of an IntersectionSet's operand UncertaintySet objects are also cleared?

[jas-yao]

Yes, that makes sense to me. I have added this.

[jas-yao]

@jsiirola @shermanjasonaf , I've taken a look at the caching behavior with the latest updates and everything works as expected. I added the suggestions made by @shermanjasonaf for IntersectionSet._cache_manager() and tests.

Looks good to me for merging, pending any final feedback on my latest changes.

[mrmundt]

Remove line?

[jas-yao]

I added this to copy the format the other solvers in test_grcs.py were written with.
If this is removed, should we also remove # @SolverFactory.register("time_delay_solver") and # @SolverFactory.register("subsolver_error__solver")?

[mrmundt]

It's not a showstopper, but this exact code is repeated, no differences, in 4095-4105. It'd be better to not have repeat code.

[jas-yao]

I tried to update the implementation to move to move the repeated code to a single class (shown below). Would this make sense?

1 file changed, 21 insertions(+), 28 deletions(-)
pyomo/contrib/pyros/uncertainty_sets.py | 49 ++++++++++++++-------------------

modified   pyomo/contrib/pyros/uncertainty_sets.py
@@ -1283,6 +1283,25 @@ class UncertaintySetList(MutableSequence):
         return self._dim
 
 
+class UncertaintySetOperation:
+    """
+    Mixin class for uncertainty sets that perform operations
+    on other uncertainty sets (e.g., IntersectionSet, CartesianProductSet).
+    """
+    @contextlib.contextmanager
+    def _cache_manager(self):
+        with contextlib.ExitStack() as stack:
+            # Verify this set's cache is empty
+            stack.enter_context(super()._cache_manager())
+            for uset in self._all_sets:
+                # Verify all component caches are empty
+                stack.enter_context(uset._cache_manager())
+            yield self
+            # This will re-enter when this context manager is exited,
+            # which will exit the stack context, triggering all the
+            # context managers entered above to exit.
+
+
 class BoxSet(UncertaintySet):
     """
     A hyperrectangle (or box).
@@ -3676,7 +3695,7 @@ class DiscreteScenarioSet(UncertaintySet):
         )
 
 
-class IntersectionSet(UncertaintySet):
+class IntersectionSet(UncertaintySetOperation, UncertaintySet):
     """
     An intersection of two or more uncertainty sets, each of which
     is represented by an `UncertaintySet` object.
@@ -3831,19 +3850,6 @@ class IntersectionSet(UncertaintySet):
 
         return []
 
-    @contextlib.contextmanager
-    def _cache_manager(self):
-        with contextlib.ExitStack() as stack:
-            # Verify this (IntersectionSet's) cache is empty
-            stack.enter_context(super()._cache_manager())
-            for uset in self.all_sets:
-                # Verify all component caches are empty
-                stack.enter_context(uset._cache_manager())
-            yield self
-            # This will re-enter when this context manager is exited,
-            # which will exit the stack context, triggering all the
-            # context managers entered above to exit.
-
     def point_in_set(self, point):
         """
         Determine whether a given point lies in the intersection set.
@@ -3955,7 +3961,7 @@ class IntersectionSet(UncertaintySet):
         super().validate(config)
 
 
-class CartesianProductSet(UncertaintySet):
+class CartesianProductSet(UncertaintySetOperation, UncertaintySet):
     """
     A Cartesian product of uncertainty sets.
 
@@ -4091,19 +4097,6 @@ class CartesianProductSet(UncertaintySet):
                 return []
         return parameter_bounds
 
-    @contextlib.contextmanager
-    def _cache_manager(self):
-        with contextlib.ExitStack() as stack:
-            # Verify this (CartesianProductSet's) cache is empty
-            stack.enter_context(super()._cache_manager())
-            for uset in self._all_sets:
-                # Verify all component caches are empty
-                stack.enter_context(uset._cache_manager())
-            yield self
-            # This will re-enter when this context manager is exited,
-            # which will exit the stack context, triggering all the
-            # context managers entered above to exit.
-
     def _iterate_over_all_sets(self):
         """
         Iterate over the sets contained in `self`.

[shermanjasonaf]

@jas-yao Looks good to me. I have just one suggestion for testing.