GDP Hull transform: add handling for cases of constraint functions not well-defined at the origin#3880
GDP Hull transform: add handling for cases of constraint functions not well-defined at the origin#3880sadavis1 wants to merge 33 commits intoPyomo:mainfrom
Conversation
… on solver particulars. Hopefully also less buggy.
it's probably more likely to be installed than baron
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3880 +/- ##
==========================================
+ Coverage 86.80% 89.95% +3.15%
==========================================
Files 903 903
Lines 106604 106748 +144
==========================================
+ Hits 92537 96027 +3490
+ Misses 14067 10721 -3346
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:
|
|
Updated for review comments |
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Please address the inline comments.
| local_vars[disj].add(var) | ||
| all_local_vars.add(var) | ||
| else: | ||
| vars_to_disaggregate[disjunct].add(var) |
There was a problem hiding this comment.
Blocking: This branch uses the stale outer-loop disjunct variable instead of the current disj. For a variable marked LocalVars while appearing in multiple disjuncts, this only adds the variable to vars_to_disaggregate for the last active disjunct, leaving other disjuncts without a substitute. A simple two-disjunct model with x in both disjuncts and LocalVars entries for both currently fails during transformation with ValueError: No value for uninitialized ScalarVar object x.
Please use the current disjunct and add regression coverage for the generalized-local-vars case. The fix may need to ensure every disjunct in disjuncts_var_appears_in[var] gets a disaggregated variable once the variable is treated as generalized local.
| f"{e.__class__.__name__} (was prepared for success or a ValueError)." | ||
| ) | ||
| raise | ||
| # Second, try making it well-defined by editing only the regular vars |
There was a problem hiding this comment.
Blocking: _get_well_defined_point() mutates original model variables before calling the heuristic solver, but restores values/fixed states only on the successful path. If the solver is unavailable, errors, or no point is found, the user model is left changed. For example, with a fixed initialized variable, a failed hull transform due to missing gurobipy left x changed from value=5, fixed=True to value=0, fixed=False.
Please wrap the solve/search section in try/finally and restore orig_values and orig_fixed on all exits, including solver exceptions and the GDP_Error path.
| self.assertEqual(m_pts[m.disjunction][m.a], 0) | ||
| self.assertEqual(m_pts[m.disjunction][m.x], 0) | ||
|
|
||
| @unittest.skipUnless(gurobi_available, "Gurobi is not available") |
There was a problem hiding this comment.
Nonblocking: Most new domain-restriction coverage is skipped when Gurobi is unavailable, including the documented escape hatch for users who pass well_defined_points directly. Please add at least one non-Gurobi test that supplies a ComponentMap manually, for example transforming log(m.x - 1) with {m.disjunction: ComponentMap([(m.x, 2)])}.
There was a problem hiding this comment.
I don't think this is a concern--we have Gurobi in our testing infrastructure.
| """, | ||
| ), | ||
| ) | ||
| CONFIG.declare( |
There was a problem hiding this comment.
Nonblocking: The class docstring says the transformation accepts keyword arguments but still lists only perspective_function, EPS, and targets. Since this PR adds public options, please document well_defined_points and well_defined_points_heuristic_solver there or point readers to the generated CONFIG documentation.
5 participants
Fixes # n/a
Summary/Motivation:
When applying the gdp.hull transformation to a model containing a constraint function whose evaluation is not well-defined when every variable is fixed at zero, the perspective functions used by the hull transformation are not well-defined either and lead to errors, especially when the mode is set to FurmanSawayaGrossmann (which is the useful one). This PR slightly alters the mathematical formulation to permit a nonzero base point which can be used in place of the origin, and adds a heuristic to try to magically find one by calling gurobi. When the zero point works, it uses that so Gurobi is never invoked.
Note: this is a minor merge conflict with #3874, if that is merged first I can rebase this
Changes proposed in this PR:
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: