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 30 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 89.92% 89.93%
========================================
Files 901 902 +1
Lines 106285 106598 +313
========================================
+ Hits 95580 95869 +289
- Misses 10705 10729 +24
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:
|
pyomo/gdp/plugins/hull.py
Outdated
| default=ComponentMap(), | ||
| domain=ComponentMap, |
There was a problem hiding this comment.
Disjunctions are hashable, so there's no need to require this be a ComponentMap: It just needs to be dict-like.
There was a problem hiding this comment.
Fixed, but note that the inner maps still need to be ComponentMap.
| for x in fallback_vars: | ||
| x.fix(0) | ||
| for x in regular_vars: | ||
| x.set_value(0) |
There was a problem hiding this comment.
Do you know that 0 is in the domain in these two cases?
There was a problem hiding this comment.
For the x's in fallback_vars, it's an intentional assumption; we want to try it this way first before unfixing those, since the fallback_vars are costlier than the others to set offsets. For the regular_vars we are relying on the solver to disregard the warm start values if it produces an infeasible or ill-defined starting point, however we would like to start with all zeros when we can because a sparser solution is better here. Also as a practical matter we can't leave variables on None here unless we clean up after the solve, since load_from after solve may in some cases not fill in variables we need values for (like if they were eliminated in presolve? not sure all the situations where this can happen). Finally note that this is all moot if we don't have warmstart, which we don't by default (but maybe should?).
pyomo/gdp/plugins/hull.py
Outdated
| if arg.__class__ in EXPR.native_types or not arg.is_potentially_variable(): | ||
| return () | ||
| if node.name == 'log': | ||
| return (arg >= EPS,) | ||
| if node.name == 'log10': | ||
| return (arg >= EPS,) | ||
| if node.name == 'sqrt': | ||
| return (arg >= 0,) | ||
| if node.name == 'asin': | ||
| return (arg >= -1, arg <= 1) | ||
| if node.name == 'acos': | ||
| return (arg >= -1, arg <= 1) | ||
| # It can't be exactly pi/2 plus a multiple of pi. Rather difficult | ||
| # to enforce, so make a conservative effort by instead keeping it in | ||
| # (-pi/2, pi/2). | ||
| if node.name == 'tan': | ||
| return (arg >= -(math.pi / 2) + EPS, arg <= (math.pi / 2) - EPS) | ||
| if node.name == 'acosh': | ||
| return (arg >= 1,) | ||
| if node.name == 'atanh': | ||
| return (arg >= -1 + EPS, arg <= 1 - EPS) |
There was a problem hiding this comment.
I think it's more efficient to make a dictionary of unary function handlers and use indirection here too. At the very least, these should be elifs.
There was a problem hiding this comment.
I changed this to also be done with handlers for consistency, though I'm not sure which way is actually faster in the end (arguments could be made either way, since function calls have overhead too).
|
Updated for review comments |
3 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: