-
Notifications
You must be signed in to change notification settings - Fork 557
Bugfix: IPOPT log parser and no objective case #3738
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
base: main
Are you sure you want to change the base?
Conversation
# If the NL instance has no objectives, report zeros | ||
if not getattr(self._nl_info, "objectives", None): | ||
vars_ = ( | ||
vars_to_load if vars_to_load is not None else self._nl_info.variable_map | ||
) | ||
return {v: 0.0 for v in vars_} |
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.
This fix is suitable if IPOPT converges to a feasible point, but if it converges to a point of local infeasibility, the values of the dual variables convey useful information about which bounds may be active at a solution.
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.
Per your suggestion, though, if there are no objectives, these should be set to 0. So I don't think we change this. We capture the raw logs anyway, so folks can inspect those if they want more details.
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.
Would it be better / more appropriate to return {}
here? Unspecified duals / reduced costs are assumed to be 0, right?
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.
@mrmundt IPOPT switches objectives when it enters into Restoration mode, using the L1 norm of the constraint violation as an objective. If it converges to an infeasible point with the exit message EXIT: Converged to a point of local infeasibility.
then it is at a local minimum of the constraint violation, and we expect nonzero dual variables and reduced costs for this objective. I don't know if IPOPT makes these values available to AMPL, though.
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.
I am not sure what to do with this information because I only understand what half of it means.
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.
If IPOPT converges to an infeasible point, the dual variables and reduced costs are nonzero and contain useful information. I'm not sure whether IPOPT makes that information available via the interface you're using.
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.
It might be worth adding some additional tests for the log parser with different Ipopt print_level
. We might just want to detect that a non-default print_level
was used and add logic to not try to parse it. This doesn't have to hold up this PR but I was thinking about it in the context of making the parser more robust.
# If the NL instance has no objectives, report zeros | ||
if not getattr(self._nl_info, "objectives", None): | ||
vars_ = ( | ||
vars_to_load if vars_to_load is not None else self._nl_info.variable_map | ||
) | ||
return {v: 0.0 for v in vars_} |
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.
Would it be better / more appropriate to return {}
here? Unspecified duals / reduced costs are assumed to be 0, right?
vars_ = ( | ||
vars_to_load if vars_to_load is not None else self._nl_info.variables | ||
) | ||
return ComponentMap((v, 0.0) for v in vars_) |
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.
Is it more correct to just return an empty ComponentMap? Aren't missing entries assumed to be 0?
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.
So I did this (both here and for get_duals
), and it actually goes against what we say is a norm, surprisingly enough:
________________________________________ TestSolvers.test_no_objective_3_ipopt _________________________________________
a = (<pyomo.contrib.solver.tests.solvers.test_solvers.TestSolvers testMethod=test_no_objective_3_ipopt>,), kw = {}
@wraps(func)
def standalone_func(*a, **kw):
> return func(*(a + p.args), **p.kwargs, **kw)
../venv-pyomo/lib/python3.12/site-packages/parameterized/parameterized.py:620:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pyomo.contrib.solver.tests.solvers.test_solvers.TestSolvers testMethod=test_no_objective_3_ipopt>
name = 'ipopt', opt_class = <class 'pyomo.contrib.solver.solvers.ipopt.Ipopt'>, use_presolve = False
@parameterized.expand(input=_load_tests(all_solvers))
def test_no_objective(
self, name: str, opt_class: Type[SolverBase], use_presolve: bool
):
opt: SolverBase = opt_class()
if not opt.available():
raise unittest.SkipTest(f'Solver {opt.name} not available.')
check_duals = True
if any(name.startswith(i) for i in nl_solvers_set):
if use_presolve:
check_duals = False
opt.config.writer_config.linear_presolve = True
else:
opt.config.writer_config.linear_presolve = False
m = pyo.ConcreteModel()
m.x = pyo.Var()
m.y = pyo.Var()
m.a1 = pyo.Param(mutable=True)
m.a2 = pyo.Param(mutable=True)
m.b1 = pyo.Param(mutable=True)
m.b2 = pyo.Param(mutable=True)
m.c1 = pyo.Constraint(expr=m.y == m.a1 * m.x + m.b1)
m.c2 = pyo.Constraint(expr=m.y == m.a2 * m.x + m.b2)
params_to_test = [(1, -1, 2, 1), (1, -2, 2, 1), (1, -1, 3, 1)]
for a1, a2, b1, b2 in params_to_test:
m.a1.value = a1
m.a2.value = a2
m.b1.value = b1
m.b2.value = b2
res: Results = opt.solve(m)
self.assertEqual(res.solution_status, SolutionStatus.optimal)
self.assertAlmostEqual(m.x.value, (b2 - b1) / (a1 - a2))
self.assertAlmostEqual(m.y.value, a1 * (b2 - b1) / (a1 - a2) + b1)
self.assertEqual(res.incumbent_objective, None)
self.assertEqual(res.objective_bound, None)
if check_duals:
duals = res.solution_loader.get_duals()
> self.assertAlmostEqual(duals[m.c1], 0)
E KeyError: <pyomo.core.base.constraint.ScalarConstraint object at 0x11b9effc0>
pyomo/contrib/solver/tests/solvers/test_solvers.py:994: KeyError
If I return an empty ComponentMap or an empty dict, then you can't actually access that item anymore.
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.
Okay, empty ComponentMap is fine; it's empty dict for get_duals
that causes these failures.
cons = ( | ||
cons_to_load if cons_to_load is not None else self._nl_info.constraints | ||
) | ||
return {c: 0.0 for c in cons} |
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.
As with RC, is it more correct to return {}?
Co-authored-by: John Siirola <jsiirola@users.noreply.github.com>
Co-authored-by: John Siirola <jsiirola@users.noreply.github.com>
@AlexGisi - you pointed out a different missed thing in the IPOPT parser, though! We did only have lowercase |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3738 +/- ##
==========================================
- Coverage 89.31% 89.31% -0.01%
==========================================
Files 896 896
Lines 103697 103728 +31
==========================================
+ Hits 92619 92645 +26
- Misses 11078 11083 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes #3736
Summary/Motivation:
The issue above mentions two bugs that are related to IPOPT/parsing both the output and the
sol
files. This PR fixes both of those bugs and adds tests.Changes proposed in this PR:
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: