Fix GDPopt LOA maximization objective sense#3986
Conversation
emma58
left a comment
There was a problem hiding this comment.
This looks good to me. I would strongly prefer you don't call private methods in tests, though, so perhaps you could remove test_loa_augmented_penalty_objective_preserves_objective_sense and keep just test_LOA_maximize_matches_minimize_negated_objective (since the latter fully tests the change).
| m.obj = Objective(expr=m.x, sense=sense) | ||
|
|
||
| solver = SolverFactory('gdpopt.loa') | ||
| original_obj = solver._setup_augmented_penalty_objective(m.GDPopt_utils) |
There was a problem hiding this comment.
It would be ideal if the test didn't rely on calling a private method. The test below covers the change, so I think you could just remove this one, keep the one below, and dodge the problem.
There was a problem hiding this comment.
Addressed in a18df5d85: I removed the private-method test and its now-unused import, while keeping the end-to-end LOA regression. I also mutation-checked the remaining test: it passes with the fix and fails with the prior behavior (x = 1 instead of 9). The full GDPopt module and Black check pass.
|
Addressed the remaining review feedback in
Verification:
No review comments were intentionally left unaddressed. The PR remains approved. GitHub CI and Read the Docs are pending for the new head; the review thread was replied to but not resolved. |
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 is a replacement for #3938, which GitHub would not allow me to reopen after it was closed. The prior review discussion and history remain available there.
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.python -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py::TestGDPoptUnit::test_LOA_maximize_matches_minimize_negated_objective1 passed in 0.54spython -m pytest -q pyomo/contrib/gdpopt/tests/test_gdpopt.py59 passed, 21 skipped, 3 deselected in 16.93spython -m black --check --diff pyomo/contrib/gdpopt/loa.py pyomo/contrib/gdpopt/tests/test_gdpopt.py2 files would be left unchangedAI-Use Disclosure
or
AI tools contributed to the development of this PR
Review process (select ONE):
Notes for reviewers (optional): This replacement PR carries over the implementation and tests from #3938, 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 LOA objective-sense change and the regression coverage around equivalent minimize/maximize formulations.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: