pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Implementing tighter logical constraints in bigm and hull transformations of nested GDPs

Open emma58 opened this issue 3 years ago • 2 comments

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:

  1. I agree my contributions are submitted under the BSD license.
  2. 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.

emma58 avatar Sep 28 '22 18:09 emma58

The Jenkins failures are I think Gurobi license related, not related to the changes... So this is ready for review @jsiirola.

emma58 avatar Sep 28 '22 22:09 emma58

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.

codecov[bot] avatar Sep 30 '22 00:09 codecov[bot]

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 avatar Oct 05 '22 14:10 jsiirola

@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 avatar Oct 05 '22 19:10 emma58

@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.

jsiirola avatar Oct 06 '22 03:10 jsiirola