Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions pyomo/contrib/pyros/tests/test_uncertainty_sets.py
Original file line number Diff line number Diff line change
Expand Up @@ -3257,10 +3257,41 @@ def test_compute_exact_parameter_bounds(self):
baron = SolverFactory("baron")
custom_set = CustomUncertaintySet(dim=2)
self.assertEqual(custom_set.parameter_bounds, [(-1, 1)] * 2)

# check clearing cache
# Expecting 0 hits, misses, size
custom_set._solve_bounds_optimization.cache_clear()
info = custom_set._solve_bounds_optimization.cache_info()
self.assertEqual(info.hits, 0)
self.assertEqual(info.misses, 0)
self.assertEqual(info.maxsize, None)
self.assertEqual(info.currsize, 0)

# check cache info
# Expecting 4 misses and size 4
self.assertEqual(
custom_set._compute_exact_parameter_bounds(baron), [(-1, 1)] * 2
)

info = custom_set._solve_bounds_optimization.cache_info()
self.assertEqual(info.hits, 0)
self.assertEqual(info.misses, 4)
self.assertEqual(info.maxsize, None)
self.assertEqual(info.currsize, 4)

# run again and check caching
# Expecting additional 4 hits from accessing cached values
self.assertEqual(
custom_set._compute_exact_parameter_bounds(baron), [(-1, 1)] * 2
)

info = custom_set._solve_bounds_optimization.cache_info()
self.assertEqual(info.hits, 4)
self.assertEqual(info.misses, 4)
self.assertEqual(info.maxsize, None)
self.assertEqual(info.currsize, 4)
custom_set._solve_bounds_optimization.cache_clear()

@unittest.skipUnless(baron_available, "BARON is not available")
def test_solve_feasibility(self):
"""
Expand Down
95 changes: 67 additions & 28 deletions pyomo/contrib/pyros/uncertainty_sets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
----------
Expand All @@ -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()
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 UncertaintySet subclass where the validate() method is overriden such that the cache clearing statement is omitted, then the _compute_exact_parameter_bounds() method may return an inaccurate result when, for example, the state of a subclass instance is altered between successive calls to PyROS.solve() that use the subclass instance.

Perhaps cache clearing should be invoked in PyROS.solve(), just before argument validation? We probably also want the cache cleared near the end of PyROS.solve() to clear memory (see @jsiirola's comment). To this end, I would consider modifying PyROS.solve() to the following:

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 test_grcs.py a test to check that successive calls to PyROS.solve() using the same UncertaintySet object yield accurate results, particularly for a case where:

  • The validate() method is overriden
  • The exact bounds are computed exclusively using _compute_exact_parameter_bounds.
  • Between the successive calls to PyROS.solve(), the attributes of the UncertaintySet object are changed such that the exact bounds are changed.


# perform validation checks
if not self.is_nonempty(config=config):
raise ValueError(f"Nonemptiness check failed for uncertainty set {self}.")

Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sense : Pyomo objective sense
sense : ~pyomo.common.ObjectiveSense

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()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 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?)

for var in bounding_model.param_vars.values()
]

return param_bounds
Expand Down
Loading