Fix GDPopt LOA maximization objective sense#3938
Conversation
|
Verified the fix against the GDPLib methanol instance that motivated #3937. Setup:
Results:
Both formulations selected the same active disjuncts: This directly addresses the original failure mode: the maximization form no longer terminates at the negative-profit local solution and now matches the minimized negative-profit formulation for this benchmark. |
bernalde
left a comment
There was a problem hiding this comment.
I would approve this review, but GitHub does not allow the authenticated author account to approve its own pull request.
No blocking issues found.
Reviewed the LOA objective construction, the objective-sense-aware bound update path, the new focused unit/regression tests, and the repository test conventions. The change is narrowly scoped and preserves the existing penalty expression behavior while ensuring the OA master objective has the same sense as the original objective.
Tests run:
python -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py::TestGDPoptUnit::test_loa_augmented_penalty_objective_preserves_objective_sense pyomo/contrib/gdpopt/tests/test_gdpopt.py::TestGDPoptUnit::test_LOA_maximize_matches_minimize_negated_objective- 2 passedpython -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py- 62 passed, 19 skipped, 3 deselectedpython -m pytest -q pyomo/contrib/gdpopt/tests- 76 passed, 35 skipped, 5 deselectedpython -m black --check --diff pyomo/contrib/gdpopt/loa.py pyomo/contrib/gdpopt/tests/test_gdpopt.py- passed, files unchanged- GDPLib methanol reproducer with
algorithm="LOA",mip_solver="gams",nlp_solver="gams"- bothminimize(-profit)andmaximize(profit)returned optimal profit1793.4292381783353in 15 iterations with the same active disjuncts
Merge-ready from this review.
emma58
left a comment
There was a problem hiding this comment.
This is kind of an embarrassing bug--thanks for catching it! A couple comments below, though neither is major.
| discrete_problem_util_block.oa_obj = Objective( | ||
| expr=0, sense=discrete_objective.sense | ||
| ) | ||
|
|
There was a problem hiding this comment.
No need to set an expr here since it's a placeholder.
There was a problem hiding this comment.
Addressed in 3262583. The OA placeholder is now expressionless, and its sense is assigned explicitly after construction so the later augmented objective keeps the original sense.
| solver = SolverFactory('gdpopt.loa') | ||
| original_obj = solver._setup_augmented_penalty_objective(m.GDPopt_utils) |
There was a problem hiding this comment.
I'm not a big fan of using private methods in tests since it makes for a lot ofediting if/when they change, but there may not be a great way around this one.
There was a problem hiding this comment.
Not changed. I kept this focused unit test because it directly isolates the objective-sense setup, while the adjacent public LOA solve regression still covers the behavior through solve(). I also reran the public path manually with Gurobi after the commit.
|
Addressed the current PR review comments. Commits pushed:
Main changes made:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up items:
|
|
@bernalde - Required legal acknowledgement was deleted and tests are failing. Please feel free to reopen with the correct template once tests are passing. |
|
I refreshed this branch against current upstream/main and updated the PR body to preserve the current Pyomo template and legal acknowledgement. GitHub would not allow this closed PR to be reopened from my account, so I opened the replacement PR here: #3986. |
Fixes #3937.
Summary/Motivation:
GDPopt LOA currently creates the augmented OA master objective with minimization sense even when the original GDP objective is a maximization objective. For maximization GDPs, this can make the discrete OA master pursue the wrong objective direction.
This was observed while validating the GDPLib methanol benchmark:
In the methanol reproducer, the algebraically equivalent formulations
minimize(-profit)andmaximize(profit)selected different disjuncts before this fix. With this branch, both formulations return the same profit and active disjuncts.Changes proposed in this PR:
minimize(-x)andmaximize(x)should select the same disjunct.Tests run:
python -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py::TestGDPoptUnit::test_loa_augmented_penalty_objective_preserves_objective_sense pyomo/contrib/gdpopt/tests/test_gdpopt.py::TestGDPoptUnit::test_LOA_maximize_matches_minimize_negated_objective2 passed in 1.40spython -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py60 passed, 21 skipped, 3 deselected in 16.46spython -m black --check --diff pyomo/contrib/gdpopt/loa.py pyomo/contrib/gdpopt/tests/test_gdpopt.py2 files would be left unchangedLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: