-
Notifications
You must be signed in to change notification settings - Fork 582
Fix GDPopt LOA maximization objective sense #3938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ | |
| Integers, | ||
| LogicalConstraint, | ||
| maximize, | ||
| minimize, | ||
| Objective, | ||
| RangeSet, | ||
| TransformationFactory, | ||
|
|
@@ -322,6 +323,48 @@ def test_complain_when_no_algorithm_specified(self): | |
| ): | ||
| SolverFactory('gdpopt').solve(m) | ||
|
|
||
| def test_loa_augmented_penalty_objective_preserves_objective_sense(self): | ||
| for sense in (minimize, maximize): | ||
| m = ConcreteModel() | ||
| m.GDPopt_utils = Block() | ||
| m.x = Var(bounds=(0, 1)) | ||
| m.obj = Objective(expr=m.x, sense=sense) | ||
|
|
||
| solver = SolverFactory('gdpopt.loa') | ||
| original_obj = solver._setup_augmented_penalty_objective(m.GDPopt_utils) | ||
|
Comment on lines
+333
to
+334
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| self.assertIs(original_obj, m.obj) | ||
| self.assertFalse(m.obj.active) | ||
| self.assertEqual(m.GDPopt_utils.oa_obj.sense, sense) | ||
|
|
||
| @unittest.skipUnless( | ||
| SolverFactory(mip_solver).available(), "MIP solver not available" | ||
| ) | ||
| def test_LOA_maximize_matches_minimize_negated_objective(self): | ||
| def build_model(maximize_objective): | ||
| m = ConcreteModel() | ||
| m.x = Var(bounds=(0, 10)) | ||
| m.disjunction = Disjunction(expr=[[m.x == 1], [m.x == 9]]) | ||
| if maximize_objective: | ||
| m.obj = Objective(expr=m.x, sense=maximize) | ||
| else: | ||
| m.obj = Objective(expr=-m.x) | ||
| return m | ||
|
|
||
| for maximize_objective in (False, True): | ||
| m = build_model(maximize_objective) | ||
| results = SolverFactory('gdpopt.loa').solve( | ||
| m, | ||
| mip_solver=mip_solver, | ||
| nlp_solver=nlp_solver, | ||
| init_algorithm='no_init', | ||
| ) | ||
|
|
||
| self.assertEqual( | ||
| results.solver.termination_condition, TerminationCondition.optimal | ||
| ) | ||
| self.assertAlmostEqual(value(m.x), 9) | ||
|
|
||
| @unittest.skipIf( | ||
| not LOA_solvers_available, | ||
| "Required subsolvers %s are not available" % (LOA_solvers,), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set an
exprhere since it's a placeholder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.