Fix a bug in gdp.bigm transformation for nested GDPs

[emma58]

Fixes .

Summary/Motivation:

The gdp.bigm transformation was only relaxing constraints on nested Disjuncts when the constraint's Disjunct was not selected. This is wrong--the constraint also needs to be relaxed when any parent Disjunct is not selected (when the nested indicator vars are not local to their parent Disjunct). For example, consider:

$$\begin{align} \min \quad &x \\\ \text{s.t.} \quad &\begin{bmatrix} Y_1 \\\ \begin{bmatrix} Z_1 \\\ x \geq 12 \end{bmatrix} \lor \begin{bmatrix} Z_2 \\\ x \geq 9 \end{bmatrix} \end{bmatrix} \lor \begin{bmatrix} Y_2 \\\ \begin{bmatrix} Z_3 \\\ x \geq 11 \end{bmatrix} \lor \begin{bmatrix} Z_4 \\\ x \geq 8 \end{bmatrix} \end{bmatrix} \\\ &Z_1 \iff Z_3 \\\ &Z_2 \iff Z_4 \\\ &0 \leq x \leq 30 \end{align}$$

The optimal solution should be $x=8$, with $Y_2$, $Z_4$, and $Z_2$ True and the other Booleans False. But if the $x \geq 9$ constraint is not relaxed when $Y_1$ is False, then we wrongly conclude that $x = 9$ is optimal. This PR adds parent Disjunct indicator variables to the big-M term so that this happens properly. So in the example, we write the transformed constraint as:

$$x \geq 9 - 9(1 - z_2 + 1 - y_1)$$

(Note that another way to fix this would be to put transformed constraints onto their parent Disjunct and simply let them get transformed again when the parent is transformed, but I opted against that because we don't need to recalculate the M value--we know what it is, we just need the BigM term to be ''activated'' when anything in the GDP tree above is False.)

Changes proposed in this PR:

• Changes constraint expressions for nested GDPs in bigm transformation
• Adds a test using the example above
• Fixes a few tests that were wrongly assuming that the nested indicator vars were local.
• Changes the arguments to _transform_constraint which changes some (suboptimal) code in GDPopt.
• The formulation of the 8 process example with logical constraints now exceeds baron demo size, so we skip those tests in GDPopt now when we don't have a baron license.

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.

[bernalde]

This fixes the error, and certainly the code has improved (thank you for the solver checks in other methods).
I wonder if the other solution you proposed is more flexible as it would allow to have recursive nested disjunctions, or just have different relaxations at different levels.
In any case, this could be a longer conversation!

[emma58]

@bernalde, this allows for arbitrarily deep nesting as well (assuming that's what you mean by recursive nested?). The different relaxations at different levels point is a good one though. The changes in this PR don't preclude it, but if you did want your bigm-ed inner constraints to then be hull-ed by the outer disjunction, for example, you would have to do that through careful use of the targets argument right now. I guess since it is still possible, I'm still leaning towards this solution because much of the computational time in the bigm transformation for large models is spent calculating M's--avoiding the performance hit of recalculating M's we know will be a big deal at scale, even if it's a slightly less parsimonious solution.

[mrmundt]

I have a deep hatred of this. Not asking for it to change - it's just like directions from my nightmares.

[jsiirola]

Why was this import added?

[emma58]

Oh, good catch. Because of a long and stupid adventure trying to get the appsi test to skip properly when Gurobi is installed but has no license. But we actually don't need this import or the one below.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.44%. Comparing base (f697791) to head (a3d6a43).
Report is 54 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3213      +/-   ##
==========================================
+ Coverage   88.39%   88.44%   +0.05%     
==========================================
  Files         846      847       +1     
  Lines       95153    96518    +1365     
==========================================
+ Hits        84106    85369    +1263     
- Misses      11047    11149     +102     

Flag	Coverage Δ
linux	86.33% <100.00%> (+<0.01%)	⬆️
osx	76.18% <100.00%> (+0.01%)	⬆️
other	86.53% <100.00%> (+<0.01%)	⬆️
win	83.82% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.