Modify PyROS Subproblem Initialization and Solver Call Routines #2515
Conversation
Remove walrus operator (for Python < 3.8)
…manjasonaf/pyomo into fix-pyros-subproblem-solve-routines
Codecov Report
@@ Coverage Diff @@
## main #2515 +/- ##
==========================================
+ Coverage 86.54% 86.60% +0.06%
==========================================
Files 715 715
Lines 80165 80190 +25
==========================================
+ Hits 69375 69449 +74
+ Misses 10790 10741 -49
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
One note: patch coverage is a bit low. Most of the uncovered lines relate to handling solver errors. We should consider adding a test case that exercises (some of) the solver failure cases. |
|
Can you add @natalieisenberg as a reviewer? I'm still unable to request reviewers. |
| # check: initial point feasible? | ||
| for con in sep_model.component_data_objects(Constraint, active=True): | ||
| lb, val, ub = value(con.lb), value(con.body), value(con.ub) | ||
| lb_viol = val < lb - 1e-5 if lb is not None else False |
There was a problem hiding this comment.
There's a hard-coded tolerance here of 1e-5...I assume that was agreed upon at some point, but should it be made into a global variable somewhere so we don't loose track of it? Like initial_point_feas_tol or something? Also out of curiosity, why 1e-5?
There was a problem hiding this comment.
My choice of 1E-5 is somewhat arbitrary; I chose this initially as this is the default absolute constraint violation tolerance for BARON, a solver I use frequently with PyROS. I could make this a global variable (perhaps defined in util.py), or opt for config.robust_feasibility_tolerance instead.
| 'pyros_var_map', | ||
| ) | ||
| setattr(model_data.master_model, varmap_name, | ||
| list(model_data.master_model.component_data_objects(Var))) |
There was a problem hiding this comment.
No need to hold up this PR, but in the future, you maybe want to use the get_vars_from_components utility (defined here) in case there are variables used in Constraints or Objectives that are no in the active tree rooted at master_model.
There was a problem hiding this comment.
Noted. Should I make this change here? If I understand correctly, master_model has no parent, such that all variables are in the active tree rooted here.
There was a problem hiding this comment.
Oh, oops, I may have mislead you. The fact that it has no parent doesn't necessarily mean that all the variables are in the active tree (they could be on deactivated Blocks, for example), but pyros makes master_model, right? So this is actually in your control at this point, I think. I just brought it up because when you initially collect the variables from the user-defined model, you likely want to use get_vars_from_components.
| ) | ||
| for master_dr, polish_dr in dr_var_zip: | ||
| for mvar, pvar in zip(master_dr.values(), polish_dr.values()): | ||
| mvar.set_value(value(pvar)) |
There was a problem hiding this comment.
Should skip_validation=True here? Just noticing since it looks like this is change from before.
There was a problem hiding this comment.
Restored skip_validation=True in subsequent commits.
Summary/Motivation:
Modify subproblem initialization and solver call routines such that:
Changes proposed in this PR:
General
solve()callsMaster Feasibility Problem
Master Problem
DR Polishing Problem
solve_master_globally=False, and global subsolver otherwise.globallyOptimalsolver termination (resolve bug)Separation Problem
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: