contrib.fbbt, pyomo.gdp: Adding new walker for compute_bounds_on_expr #3027
Conversation
…r, which may or may not matter for this exercise...
…igm, which basically means caching Var bounds info for the whole transformation
…uming it was there from before config...
…e bigm transformations only building on visitor, and caching the leaf bounds
… trig with bounds on the range
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3027 +/- ##
==========================================
+ Coverage 87.88% 87.89% +0.01%
==========================================
Files 769 770 +1
Lines 89555 89662 +107
==========================================
+ Hits 78705 78812 +107
Misses 10850 10850
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| self, | ||
| leaf_bounds=None, | ||
| feasibility_tol=1e-8, | ||
| use_fixed_var_values_as_bounds=False, |
There was a problem hiding this comment.
I expected this default to be True?
There was a problem hiding this comment.
I did False because it seems safer to me... Because then the bounds are right regardless of if variables get unfixed later. Basically this way we get positive confirmation from a user that they know things are potentially invalid if they unfix Vars.
| return False, None | ||
|
|
||
|
|
||
| def _before_other(visitor, child): |
There was a problem hiding this comment.
What is this handler for?
There was a problem hiding this comment.
The previous walker basically had a default that if it didn't understand what was going on, it still returned bounds of (-inf, inf). So this maintains that behavior. I'm not sure that it's a good idea long-term though?
There was a problem hiding this comment.
I wonder if this should toss a warning that your expression had things we weren't expecting?
There was a problem hiding this comment.
I think we should, but that's actually going to require a more significant rewrite because right now this is catching both things we don't understand and things that we do understand, just don't need a beforeChild handler for (like SumExpression). So I think it would make the most sense to move this onto the new BeforeChildDispatcher base class and then finally get pedantic about when we're confused and when we just don't care. But I'd rather that be a separate PR.
| return False, None | ||
|
|
||
|
|
||
| def _before_other(visitor, child): |
There was a problem hiding this comment.
I wonder if this should toss a warning that your expression had things we weren't expecting?
| elif not child.is_potentially_variable(): | ||
| handlers[child_type] = _before_NPV | ||
| else: | ||
| handlers[child_type] = _before_other |
There was a problem hiding this comment.
I feel like if we hit this, then we should warn about an unknown / unhandled expression type?
Fixes # .
Summary/Motivation:
The majority of the time in the
gdp.bigmtransformation is spent calculating M values, mostly in the function (from fbbt)compute_bounds_on_expr. This PR adds a new walker to contrib.fbbt specifically for computing bounds on expressions based on Var bounds. It is very similar to the leaf-to-root walker, but not identical as it is designed for only a single pass (unlike in actual fbbt), and it caches only the leaf node bounds. Moving the bigm transformations onto it helps performance in calculating M values significantly.Changes proposed in this PR:
ExpressionBoundsVisitorto contrib.fbbtcompute_bounds_on_exprwrapExpressionBoundsVisitorinstead of_FBBTVisitorLeafToRoot_FBBTVisitorLeafToRoot, moving it onto theStreamBasedExpressionVisitorbase classmulininterval.py: This is actually quite significant performance-wise.gdp.bigmandgdp.mbigmto store an instance ofExpressionBoundsVisitorfor all ofapply_toto take advantage of caching, rather than relying oncompute_bounds_on_expr.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: