Implementing tighter logical constraints in bigm and hull transformations of nested GDPs
Fixes # NA
Summary/Motivation:
In a nested GDP, the tightest formulation is to set the sum of the inner disjunction's binary indicator variables to the outer disjunct's binary indicator variable. This PR both makes this change in the bigm and hull transformations, as well as finally taking advantage of the preprocessing we do in order to not recurse into already-transformed components.
Changes proposed in this PR:
- Changing form of algebraic constraints for nested Disjunctions (as well as the "re-aggregation" constraints in hull linking original variables to disaggregated variables)
- No longer moving transformation blocks for inner Disjuncts to their parent Disjunct's transformation block: Instead we declare it at the "root" disjunct to begin with.
- Changing hull to transform from the root to the leaves of the "GDP tree" rather than vice versa (what bigm does). This is because it is more efficient if we know the parent Disjunct's disaggregated variables when we transform inner Disjunctions--else we'll have to transform constraints we created later.
- NOTE: The above change changes the order in which components are transformed in hull. This changed both the baseline tests in GDP and the FME tests that rely on the hull transformation.
- (In general this PR is revealing of some problems with the GDP tests given how many had to change--I'll get to that in another 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:
- I agree my contributions are submitted under the BSD license.
- 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.
The Jenkins failures are I think Gurobi license related, not related to the changes... So this is ready for review @jsiirola.
Codecov Report
Base: 86.39% // Head: 86.65% // Increases project coverage by +0.26% :tada:
Coverage data is based on head (
c87988a) compared to base (7993efa). Patch coverage: 97.27% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@
## main #2550 +/- ##
==========================================
+ Coverage 86.39% 86.65% +0.26%
==========================================
Files 723 723
Lines 80646 80579 -67
==========================================
+ Hits 69672 69825 +153
+ Misses 10974 10754 -220
| Flag | Coverage Δ | |
|---|---|---|
| linux | 83.45% <97.24%> (+1.74%) |
:arrow_up: |
| osx | 73.67% <97.24%> (+0.01%) |
:arrow_up: |
| other | 83.63% <97.24%> (+1.73%) |
:arrow_up: |
| win | 80.66% <97.24%> (+0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| pyomo/gdp/util.py | 92.25% <94.11%> (+4.29%) |
:arrow_up: |
| pyomo/gdp/plugins/bigm.py | 95.00% <100.00%> (+3.00%) |
:arrow_up: |
| pyomo/gdp/plugins/hull.py | 93.44% <100.00%> (+2.92%) |
:arrow_up: |
| pyomo/gdp/plugins/partition_disjuncts.py | 94.75% <0.00%> (-3.75%) |
:arrow_down: |
| pyomo/gdp/disjunct.py | 86.13% <0.00%> (-0.73%) |
:arrow_down: |
| pyomo/common/env.py | 89.15% <0.00%> (-0.41%) |
:arrow_down: |
| pyomo/contrib/interior_point/interior_point.py | 95.03% <0.00%> (+0.26%) |
:arrow_up: |
| pyomo/contrib/parmest/utils/mpi_utils.py | 75.25% <0.00%> (+27.83%) |
:arrow_up: |
| ...o/contrib/interior_point/linalg/mumps_interface.py | 76.82% <0.00%> (+54.87%) |
:arrow_up: |
| ... and 2 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
One note: merging this will mean that you cannot expect to solve individual relaxed disjuncts after either the BigM or Hull transformation is applied to the entire model, correct?
@jsiirola, you could solve disjunct.transformation_block() (if you added an objective) for not-nested models. But you are right that for nested models, the nested stuff is not a descendant of that block. But I don't think that it was before either?
Do you think that's something we should support? I think we'd have to transform from root to leaf in bigm too, if we were going to support that. Because we would need to be putting each inner-Disjunct transformation block on the transformation block of its parent.
@emma58: no, I don't think that we need to support that. At least not yet. If the use case arises, it shouldn't be too onerous to code up: we don't need to transform root-to-leaf: only create the transformation blocks root-to-leaf. The we can go back and do the normal leaf-to-root transformations.