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.

[jsiirola]

Suggested change

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

[jsiirola]

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.

[jsiirola]

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?

[jsiirola]

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()

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.93%. Comparing base (6744ca0) to head (a6ef4f5).
⚠️ Report is 34 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3877   +/-   ##
=======================================
  Coverage   89.92%   89.93%           
=======================================
  Files         901      901           
  Lines      106285   106290    +5     
=======================================
+ Hits        95580    95587    +7     
+ Misses      10705    10703    -2     

Flag	Coverage Δ
builders	29.20% <11.76%> (+<0.01%)	⬆️
default	86.23% <100.00%> (?)
expensive	35.66% <11.76%> (?)
linux	87.38% <100.00%> (-2.04%)	⬇️
linux_other	87.38% <100.00%> (+<0.01%)	⬆️
oldsolvers	28.11% <11.76%> (+<0.01%)	⬆️
osx	82.70% <100.00%> (+<0.01%)	⬆️
win	85.80% <100.00%> (+<0.01%)	⬆️
win_other	85.80% <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.

[shermanjasonaf]

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

[shermanjasonaf]

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.