Skip to content

Fix GLOA convex affine cut slope - #4011

Open
bernalde wants to merge 1 commit into
Pyomo:mainfrom
SECQUOIA:fix/issue-3939-gloa-convex-cut-slope
Open

Fix GLOA convex affine cut slope#4011
bernalde wants to merge 1 commit into
Pyomo:mainfrom
SECQUOIA:fix/issue-3939-gloa-convex-cut-slope

Conversation

@bernalde

Copy link
Copy Markdown
Contributor

Fixes #3939.

Summary/Motivation:

GDPopt GLOA builds both of its affine cuts from a single cut_body expression computed from the MC++ concave slopes (ccSlope). The concave underestimator cut is correct, but the convex overestimator cut reuses those same concave slopes instead of the convex slopes (cvSlope). That produces an invalid upper cut, which can cut off feasible points and let GLOA terminate at a non-global solution.

MindtPy's GOA affine-cut path already uses the convex slope for its upper cut, so this brings GLOA in line with the existing correct implementation.

This is a replacement for #3940, which GitHub would not allow me to reopen after it was closed. The prior review discussion and history remain available there.

Integration evidence from GDPlib (recorded on #3940): on the GDPlib methanol GLOA benchmark, unpatched Pyomo 6.10.0 terminated after 1 iteration at objective -1743.4292381783366, while this branch ran 15 iterations and reached -1793.4292385308854, matching GDPlib's documented best-known objective -1793.4292381783 to solver tolerance. This was the focused methanol GLOA case; I did not run the full GDPlib benchmark matrix.

Changes proposed in this PR:

  • Compute the concave and convex affine cut bodies separately in pyomo/contrib/gdpopt/gloa.py, so the convex overestimator cut uses cvSlope (MC++ subcv()) rather than reusing the concave ccSlope.
  • Evaluate the trivially-True and trivially-False checks per cut, and add each cut independently, so a constant concave cut no longer suppresses a variable convex cut (and vice versa).
  • Add test_gloa_affine_cut_uses_convex_slope_for_upper_cut to pyomo/contrib/gdpopt/tests/test_gdpopt.py, a solver-free regression covering the invalid-cut case from the issue.
  • Add the matching test_goa_affine_cut_uses_convex_slope_for_upper_cut to pyomo/contrib/mindtpy/tests/unit_test.py, documenting that MindtPy GOA already uses the convex slope for its upper cut.
  • Validation performed locally on this branch, rebased onto current main:
    • python -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py pyomo/contrib/mindtpy/tests/unit_test.py
      • Result: 65 passed, 19 skipped, 3 deselected in 39.87s
    • python -m black --check pyomo/contrib/gdpopt/gloa.py pyomo/contrib/gdpopt/tests/test_gdpopt.py pyomo/contrib/mindtpy/tests/unit_test.py
      • Result: passed, 3 files would be left unchanged
    • typos --config ./.github/workflows/typos.toml on the three changed files
      • Result: passed, no findings

AI-Use Disclosure

  • AI tools were NOT used during the preparation of this PR

or

  • AI tools contributed to the development of this PR

    • AI tools generated documentation (including the PR description/comments, code comments, and/or Sphinx documentation)
    • AI tools generated tests (baselines, examples, and/or code)
    • AI tools generated code (apart from tests)

    Review process (select ONE):

    • Rewritten: All AI-generated content was rewritten by me before being committed.
    • Reviewed/verified: I retained AI-generated content and verified it before committing. Verification included (as applicable):
      • Ran the code and fixed issues
      • Added and ran tests
      • Checked correctness/logic of code and tests
      • Checked for alignment with the contribution guide
      • Considered security implications
    • As-is: AI-generated content was commited directly to the repository

Notes for reviewers (optional): This replacement PR carries over the implementation and tests from #3940 unchanged, then refreshes the branch against current main. The replacement PR description and branch-refresh workflow were prepared with AI assistance and reviewed before posting. Reviewers may want to focus on the restructured trivial-cut handling: previously a single is_potentially_variable check governed both cuts together, and this PR splits that into independent per-cut checks, so the trivially-False DeveloperError conditions are now evaluated separately for the concave lower cut and the convex upper cut.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GDPopt GLOA convex affine cuts use concave MC++ slopes

1 participant