Add OA equality relaxation penalties#20
Conversation
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review of PR #20 (OA equality relaxation + augmented penalty, Closes #12). I am the PR author, so GitHub only allows a COMMENT review here; the formal approval or change request must come from another maintainer.
Overall this is solid and the design is sound. The default (non-slack) OA path is preserved bit-for-bit: public_bound_valid collapses to decomp.master_bound_valid when add_slack=False, the linear-objective c vector and epigraph index reduce to the prior values when there are no slacks, and every append to oa_A_rows now routes through _append_master_cut, keeping oa_slack_flags aligned (the _solve_master_milp length check guards against drift). Slack rows are constructed correctly (coeffs.x - s <= rhs, s in [0, max_slack], penalized in the objective), and slacks are correctly excluded from objective-epigraph, no-good, and feasibility cuts. gap_certified is a real SolveResult field and NLPResult.multipliers exists, so the new wiring does not raise. The _add_oa_cuts rewrite from generate_oa_cuts_from_evaluator to a manual generate_oa_cut loop faithfully reproduces generate_oa_cuts_from_evaluator_report (same cons_vals/jac/convex-mask-skip), with the added benefit of a row index for dual orientation. The four issue-12 acceptance items (cut orientation, slack row construction, penalty coefficients, gap_certified=False) each have a corresponding test.
No blocking issues.
Nonblocking
-
The MIP-NLP option-key list is now duplicated in three places:
python/discopt/solver.py:2376,python/discopt/solver.py:2690, and_OA_OPTION_KEYSinpython/discopt/solvers/mip_nlp.py:15. This PR had to extend all three in lockstep; a future option added to only one will silently fail to forward (solver.py) or be wrongly rejected (mip_nlp.py). Define the set once (e.g. export_OA_OPTION_KEYSfrommip_nlp.py) and have bothsolve_modelextraction blocks iterate it. -
Minor: unsupported options raise
ValueErrorwhen entered throughsolve_mip_nlp(mip_nlp.py:56) butNotImplementedErrorwhen entered throughsolve_oa's newkwargsguard (oa.py). Same error class, two exception types depending on entry point. Consider aligning them.
Questions
-
Dual-orientation multiplier sign convention (oa.py:345-346).
_equality_relaxation_sigmaassumes a fixed sign convention formultipliers[row_index], butNLPResultdocuments multipliers as being "in the sign convention of the problem as passed to the solver," and OA can use eithernlp_pounceornlp_ipopt. If those two backends differ in sign convention, the chosen cut orientation will be correct for one and inverted for the other. Because slack/heuristic runs are uncertified this is not a soundness bug, but an inverted orientation defeats the feature's purpose. Confirm both backends share the assumed convention, or normalize per-solver, and consider an integration test that exercises multipliers end-to-end (the current test calls_equality_relaxation_sigmadirectly). -
Certification scope of
equality_relaxationalone (oa.py:848). Uncertification is tied toadd_slack(andheuristic_nonconvex, which forcesadd_slack). A caller passingequality_relaxation=Truewithoutadd_slackstill getsgap_certified=Trueand a public bound/gap, even though relaxing a nonlinear equality to a single-direction tangent cut is a nonconvex heuristic. This matches pre-PR behavior (no regression), but issue #12's task "mark heuristic nonconvex OA results as uncertified" is arguably broader than the slack path. Confirm whether standaloneequality_relaxationshould also setgap_certified=False, or document that it is intended only for affine/convex-compatible equalities.
Issue intent: Closes #12 is appropriate. #12 is a sub-issue of epic #7 and its four tasks and four acceptance-test items are all delivered. Question 2 above is the only arguable gap, and it is a scoping clarification rather than a missed core ask.
Tests run (clean worktree at head 247f2b7, with the prebuilt _rust extension copied in since it is not part of the diff):
PYTHONPATH=python pytest python/tests/test_mip_nlp.py -q-> 9 passed.PYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_oa.py -q-> 16 passed, 10 deselected.PYTHONPATH=python pytest python/tests/test_mip_nlp.py test_oa.py test_gdp.py test_lp_backend_select.py test_adversarial_recent_fixes.py -q-> 132 passed, 32 deselected.ruff checkandruff format --checkon the four changed files -> passed.
The only initial failures wereModuleNotFoundError: discopt._rust, a local build artifact (the Rust extension is not in the diff); they cleared once the prebuilt.sowas made importable.
Merge-readiness: ready on the merits. No blocking issues; the nonblocking maintainability item and the two questions can be addressed in follow-up. The formal approval must come from another maintainer since I am the author.
| mip_nlp_options = kwargs.pop("mip_nlp_options", None) | ||
| mip_nlp_kwargs: dict[str, Any] = {} | ||
| for key in ("equality_relaxation", "ecp_mode", "feasibility_cuts"): | ||
| for key in ( |
There was a problem hiding this comment.
Nonblocking (maintainability). This option-key tuple is duplicated at python/discopt/solver.py:2690 and again as _OA_OPTION_KEYS in python/discopt/solvers/mip_nlp.py:15. The issue-12 options had to be added to all three; a future option added to only one will silently fail to forward here or be wrongly rejected in mip_nlp. Define the set once (export _OA_OPTION_KEYS from mip_nlp.py) and iterate it in both extraction blocks.
There was a problem hiding this comment.
Addressed in 72b7c5e. OA_OPTION_KEYS is now defined once in discopt.solvers.mip_nlp and both solve_model extraction blocks iterate that shared set.
| if multipliers is None or row_index >= len(multipliers): | ||
| return 1.0 | ||
| dual = float(multipliers[row_index]) | ||
| sign_adjust = 1.0 if objective_sense == ObjectiveSense.MAXIMIZE else -1.0 |
There was a problem hiding this comment.
Question. This assumes a fixed sign convention for multipliers[row_index], but NLPResult documents multipliers as being in "the sign convention of the problem as passed to the solver," and OA can run on either nlp_pounce or nlp_ipopt. If those backends differ in convention, the orientation is correct for one and inverted for the other. Not a soundness bug (slack/heuristic runs are uncertified), but an inverted orientation defeats the feature. Confirm both backends agree, or normalize per-solver, and consider an end-to-end test (the current test calls _equality_relaxation_sigma directly).
There was a problem hiding this comment.
Addressed in 72b7c5e. Both NLP wrappers expose Ipopt-compatible mult_g unchanged; the OA helper now documents that assumption, and test_equality_relaxation_orientation_flips_master_cut_row verifies the sign flips the actual appended master cut row.
| model._objective.sense if model._objective is not None else ObjectiveSense.MINIMIZE | ||
| ) | ||
| constraint_cut_mask = None if heuristic_nonconvex else decomp.oa_constraint_mask | ||
| public_bound_valid = decomp.master_bound_valid and not add_slack |
There was a problem hiding this comment.
Question. Uncertification is tied to add_slack only. A caller setting equality_relaxation=True without add_slack still gets gap_certified=True plus a public bound/gap, despite single-direction tangent relaxation of a nonlinear equality being a nonconvex heuristic. This matches pre-PR behavior (no regression), but issue #12's "mark heuristic nonconvex OA results as uncertified" may be broader than the slack path. Should standalone equality_relaxation also set gap_certified=False, or is it intended only for affine/convex-compatible equalities?
There was a problem hiding this comment.
Addressed in 72b7c5e. Integer OA runs that actually relax equality cuts now suppress public bound/gap reporting and return gap_certified=False; test_er_helps_nonlinear_equality asserts that behavior.
|
Pushed commit Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
bernalde
left a comment
bernalde
left a comment
There was a problem hiding this comment.
Focused review of commit 72b7c5ed2 ("Address OA review feedback"), scoped to the new certification-gating logic in solve_oa and the multiplier sign-convention claim, per the round-2 verification hand-off. I am the PR author, so GitHub only permits a COMMENT review; the formal approval or change request must come from another maintainer, and the round-1 threads remain unresolved.
Verification result: the new gating is sound and the sign-convention claim holds. No blocking issues.
What I checked and confirmed:
- No over-certification path.
uses_uncertified_relaxation = add_slack or uses_relaxed_equality_cutsgates every relevant exit: the LB update (oa.py:1030), the finalbound(oa.py:1172, soreported_gapcollapses to None), andgap_certifiedon all fourreturn SolveResultsites. When tainted,bound/gapare None andgap_certifiedis False; status is never "optimal". - The predicate is correctly scoped.
uses_relaxed_equality_cutsrequiresbool(decomp.int_indices), which matches where relaxed equality cuts are actually emitted:_add_oa_cutsonly runs in the integer loop (the no-integer branch returns after a direct, exact NLP solve, so leaving it certified is right). A==indecomp.constraint_senses[:n_cons]is a nonlinear equality (affine equalities live in the linear set), so it is genuinely nonconvex and tainting is justified. - The sign-convention claim is verified, not just asserted. Both backends store
info["mult_g"]intoNLPResult.multipliersunchanged (nlp_ipopt.py:227,241;nlp_pounce.py:113,127, which is documented as cyipopt-semantics-compatible), so the two agree on convention and the orientation logic is fed consistent duals. That resolves the round-1 Question. - Tests pass at
72b7c5ed2:pytest python/tests/test_mip_nlp.py test_oa.py test_gdp.py test_lp_backend_select.py test_adversarial_recent_fixes.py -q-> 134 passed, 32 deselected (the two new tests included).
Nonblocking
-
Docstring not updated for the changed contract. The
equality_relaxationdocstring (oa.py, around line 802) still describes only the mechanism. Enabling it now suppresses the certified bound/gap (bound/gap None,gap_certified=False, status never "optimal") whenever a nonlinear equality is relaxed. This is a public option whose result contract changed; document it as theadd_slack/heuristic_nonconvexentries already do. -
Missing test for the certified-default boundary. The new tests cover the tainting case (
test_oa.py::TestEqualityRelaxation) and orientation, but nothing asserts the predicate does NOT over-trigger: e.g.equality_relaxation=Trueon a convex MINLP with no nonlinear equality should still returngap_certified=True. Add that case to lock in that the new taint is scoped to relaxed equalities and does not regress certified convex OA. -
Orientation may use unconverged duals.
_solve_nlpreturnsmultiplierseven onITERATION_LIMIT(its own comment notes the IPM "may not certify dual convergence"), and_equality_relaxation_sigmaorients cuts from them. Soundness is unaffected (these runs are uncertified), but cut orientation can silently degrade. Consider only trusting multipliers from anOPTIMALNLP exit, or note the limitation in the helper.
Question
- In
test_oa.py::TestEqualityRelaxation, the in-test comment says the equality is "relaxed to x^2 <= y" and the assertion isstatus in ("optimal", "feasible"). With the new dual-oriented, always-uncertified path the status can only be "feasible" or "iteration_limit", never "optimal", and the orientation is multiplier-driven rather than fixed to<=. Worth tightening the comment/assertion to document the actual contract.
Merge-readiness: from this focused review, the new gating logic is correct and merge-ready; the items above are follow-ups, not blockers. Because this is my own PR, the formal verdict must come from another maintainer, and the round-1 review threads are still open. I would not consider the verification loop closed until those threads are resolved, but I see nothing here that blocks merge.
| model._objective.sense if model._objective is not None else ObjectiveSense.MINIMIZE | ||
| ) | ||
| constraint_cut_mask = None if heuristic_nonconvex else decomp.oa_constraint_mask | ||
| uses_relaxed_equality_cuts = ( |
There was a problem hiding this comment.
Nonblocking (doc gap). This predicate changes the public result contract for equality_relaxation: when it is True and a nonlinear equality is relaxed (integers present), bound/gap become None, gap_certified is False, and status is never "optimal". The equality_relaxation docstring (around line 802) still describes only the mechanism. Add a sentence noting the uncertified result, mirroring the add_slack/heuristic_nonconvex docstring entries. The predicate logic itself is sound: integer-gating matches where _add_oa_cuts actually relaxes equalities, and a == in constraint_senses[:n_cons] is a nonlinear (nonconvex) equality.
There was a problem hiding this comment.
Addressed in f9e4771. The equality_relaxation docstring now states that runs relaxing nonlinear equality cuts do not report certified public bounds or gaps.
| """Return the dual-oriented sign for a relaxed equality row.""" | ||
| if multipliers is None or row_index >= len(multipliers): | ||
| return 1.0 | ||
| # Both NLP backends expose Ipopt-compatible ``mult_g`` values unchanged; |
There was a problem hiding this comment.
Nonblocking. The claim is verified: both nlp_ipopt and nlp_pounce store info["mult_g"] unchanged (nlp_pounce is documented cyipopt-compatible), so the two backends agree and orientation is consistent. One residual: _solve_nlp returns multipliers even on ITERATION_LIMIT, where its own comment notes dual convergence is not certified. _equality_relaxation_sigma then orients cuts from possibly-unconverged duals. Soundness is fine (these runs are uncertified), but orientation can silently degrade; consider trusting multipliers only from an OPTIMAL exit, or note the limitation here.
There was a problem hiding this comment.
Addressed in f9e4771. _solve_nlp now drops multipliers on ITERATION_LIMIT and test_iteration_limited_nlp_drops_uncertified_multipliers covers that behavior.
| assert result.status in ("optimal", "feasible") | ||
| assert result.bound is None | ||
| assert result.gap is None | ||
| assert result.gap_certified is False |
There was a problem hiding this comment.
Nonblocking (missing test for changed-behavior boundary). This locks in the tainting case well. Add the complementary case: equality_relaxation=True on a convex MINLP with no nonlinear equality should still return gap_certified=True (the predicate's any(sense == "==") is False), to prove the new taint does not over-trigger and regress certified convex OA. Separately (Question): the in-test comment "relaxed to x^2 <= y" and status in ("optimal", "feasible") no longer match the dual-oriented, always-uncertified behavior (status can only be "feasible"/"iteration_limit" now); consider tightening.
There was a problem hiding this comment.
Addressed in f9e4771. Added test_er_without_nonlinear_equality_remains_certified, and tightened the nonlinear-equality ER test comment plus status assertion to the incumbent-only uncertified contract.
|
Pushed commit Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks or follow-up:
|
1 participant
Summary
heuristic_nonconvex=TrueOA as incumbent-only: no public bound/gap certificate is reported.solver="mip-nlp"and reject unrelated future options.Closes #12.
Tests
PYTHONPATH=python pytest python/tests/test_mip_nlp.py -qPYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_oa.py -qPYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_oa.py python/tests/test_gdp.py -qPYTHONPATH=python pytest python/tests/test_lp_backend_select.py python/tests/test_adversarial_recent_fixes.py -qmake lint RUFF='python -m ruff'PYTHONPATH=python python -m mypy python/discopt/ --ignore-missing-importsNotes:
git commithook could not run becausepre-commitis not installed in this environment. The equivalent Ruff and mypy checks above were run directly.Branch Hygiene
mainfix/issue-12-equality-penaltye36d683(fork/main, synced with upstreammainafter Move OA selection to mip-nlp solver jkitchin/discopt#320)