fix: stop reading rounding as proof of infeasibility - #38
Merged
Conversation
Closes #36. solve_qp rejected feasible problems at degenerate vertices. The reproducer in the issue is solved by the reference C implementation and raised "constraints are inconsistent, no solution" here. The dual method concludes infeasibility when the entering constraint's normal already lies in the span of the active set and no active multiplier can be reduced. That argument assumes the constraint is *genuinely* violated, and the Householder deviation makes the other case reachable: at a vertex where all four constraints are tight and the three nonzero normals span two dimensions, the iterate carries 8 * eps on the fourth constraint where the reference's Givens chain carries 4.68 * eps -- either side of the 6.43 * eps VSMALL snap both apply to the slacks. Above it the constraint reads as violated, the primal cannot move, no multiplier can fall, and _step_choice declares the dual unbounded. The fix guards the *conclusion*, not the selection. Bumping VSMALL was the obvious alternative and is worse on two counts: it changes which constraints get selected, which the differential suite pins against the reference iteration for iteration, and it puts the burden on a threshold that has to separate "rounding" from "violated but tiny" -- a distinction with no safe margin, since a real violation can be arbitrarily small. The infeasibility test only has to separate "rounding" from "provably infeasible", and infeasibility is macroscopic: the violation is set by the geometry, not the arithmetic. Any threshold in the thirteen orders between them works, so _NOISE_MARGIN is loose on purpose and there is nothing to tune. _is_spurious_violation scales the bound by ||c|| ||x|| + |b| rather than testing an absolute number, because the slack inherits the error in xv rather than merely the error of its own dot product -- for a constraint x already sits on, the terms that formed it are themselves at the noise floor and say nothing. A constraint judged spurious is set aside in `ignored` and the outer loop takes the next candidate; the set is cleared whenever xv moves, so nothing stays masked on the strength of a stale iterate. Nothing that solved before can change: the guard is reachable only on the path that previously raised. Confirmed by the differential suite, whose 867 comparisons still match the reference's iteration counts exactly, and by the three infeasibility cases, which still raise. Verified further with 30000 randomized Hypothesis examples at caps wider than the committed ones (max_n=6, max_m=8, max_meq=3): 60000 solves, zero false infeasibility, zero certificate failures. The same setting produced roughly 10 per 6000 before. Cost is one Python call per inner iteration: +2% at n <= 100 and within noise from n = 200 up, measured A/B interleaved against the pre-fix build rather than against an earlier session's numbers. solve_qp stays B (10) on radon and the package average is unchanged at A (3.71). Mutation testing left seven survivors in the new function, all tolerance mutants. tests/test_structure.py now kills all seven by deriving its bounds from _NOISE_MARGIN and VSMALL rather than restating them, so the margin stays free to be retuned without rewriting the tests. The baseline moves 12 -> 14 for two survivors that are *not* from this change: _drop_constraint 24 and 25 survive on main too, verified by applying both to the pre-fix source and running the full suite, which passes. They write to the slot past the shrunken active set, which is only ever read as [:nact]. They were missing from the old baseline because mutmut caches per-mutant verdicts and a run that changes only tests reuses them -- a trap documented in MUTATION.md now, since it silently reported the seven above as un-killed until the cache was cleared. The README records the one way this now diverges from the reference: a problem whose infeasibility is itself at the rounding floor may be solved here and rejected there. 1013 passed, 100% statement and branch coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a false infeasibility verdict in solve_qp that can occur at maximally degenerate vertices when rounding pushes a slack just above the existing VSMALL snap threshold, causing the dual method’s “stuck ⇒ infeasible” inference to trigger incorrectly.
Changes:
- Add
_is_spurious_violationand use it insolve_qpto set aside rounding-scale “violations” on the specific stuck path that previously raised infeasibility. - Remove the strict
xfail/ rejection logic around issue #36 and make the regression test and properties assert on the real solver result. - Add unit tests for the new spurious-violation rule and update documentation/mutation-testing baseline to reflect the new behavior and tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/cvx/quadprog/_solve.py |
Adds _NOISE_MARGIN, _is_spurious_violation, and an ignored mechanism to avoid concluding infeasibility from rounding-scale slacks. |
tests/test_structure.py |
Adds unit tests that pin the shape/scale behavior of _is_spurious_violation. |
tests/test_properties.py |
Removes the xfail/reject shim and asserts the solver no longer rejects the known-feasible degenerate case. |
README.md |
Documents the new “infeasibility only concluded above rounding floor” behavior and its deliberate divergence edge case. |
docs/development/MUTATION.md |
Updates mutmut baseline counts and documents cache behavior and the new helper’s mutants. |
.github/workflows/mutation.yml |
Bumps mutation baseline from 12 to 14. |
Suppressed comments (1)
src/cvx/quadprog/_solve.py:310
ignoredis only cleared whenxvmoves (ztn is not None). But the active set can also change without movingxv(theztn is None/ dual-only branch drops constraints). In that case, a constraint previously marked ignored may become enforceable under the new active-set span, yet it will remain masked untilxvmoves, which can let the solver terminate without reconsidering it.
if ztn is not None:
xv += step * zv
obj += step * ztn * (step / 2.0 + u)
# xv moved, so every slack set aside against the old one is
# stale and must be measured again.
nign = 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+294
to
+301
| if _is_spurious_violation(ztn, idel, slack, nbv_safe[iadd - 1], xv, b[iadd - 1]): | ||
| # Satisfied to within the accuracy of xv, but the primal cannot | ||
| # move and no multiplier can be reduced. Enforcing it would be a | ||
| # no-op and concluding infeasibility from it would be wrong, so | ||
| # set it aside and let the outer loop take the next candidate. | ||
| ignored[nign] = iadd - 1 | ||
| nign += 1 | ||
| break |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
solve_qprejected feasible problems at degenerate vertices, where the reference Cimplementation returns the minimiser.
Closes #36
The defect
The dual method calls a problem infeasible when the entering constraint's normal already
lies in the span of the active set and no active multiplier can be reduced. That argument
assumes the constraint is genuinely violated.
The Householder deviation makes the other case reachable. At a vertex where all four
constraints are tight and the three nonzero normals span two dimensions:
6.43 * epsVSMALLsnapAbove the snap it reads as violated, the primal cannot move, no multiplier can fall, and
_step_choicedeclares the dual unbounded.Why guard the conclusion rather than raise
VSMALLBumping the constant was the obvious alternative and is worse on two counts:
test_against_c.pypins that againstthe reference iteration for iteration across 867 comparisons.
distinction with no safe margin, since a real violation can be arbitrarily small.
The infeasibility test only has to separate "rounding" from "provably infeasible", and
infeasibility is macroscopic: the violation is set by the geometry, not the arithmetic.
Any threshold in the ~13 orders between them works, so
_NOISE_MARGINis loose on purposeand there is nothing to tune.
Nothing that solved before can change — the guard is reachable only on the path that
previously raised.
Changes
_is_spurious_violation— scales the bound by||c|| ||x|| + |b|rather than testing anabsolute number, because the slack inherits the error in
xv, not merely the error of itsown dot product. For a constraint
xalready sits on, the terms that formed it arethemselves at the noise floor and say nothing.
solve_qp— a constraint judged spurious goes intoignoredand the outer loop takes thenext candidate. The set is cleared whenever
xvmoves, so nothing stays masked on a staleiterate. The insertion moved inside the inner loop so the new exit needs no flag.
tests/test_structure.py— seven unit tests for the new helper.README.md— records the one resulting divergence (below).docs/development/MUTATION.md,.github/workflows/mutation.yml— baseline 12 → 14, see below.Verification
counts exactly; the three infeasibility cases still raise.
(
max_n=6, max_m=8, max_meq=3): 60000 solves, zero false infeasibility, zerocertificate failures. The same setting produced ~10 per 6000 before.
solve_qpstays B (10), package average unchanged at A (3.71).n = 200 up, measured A/B interleaved against a pre-fix build rather than against earlier
numbers, since this repo has been bitten by machine drift before.
Mutation testing
Seven survivors appeared in the new function, all tolerance mutants. All seven are killed by
the new unit tests, which derive their bounds from
_NOISE_MARGINandVSMALLinstead ofrestating them — the margin is documented as deliberately loose, and a test that froze it
would contradict that.
The baseline still moves 12 → 14, for two survivors that are not from this change:
_drop_constraint24 and 25 survive onmaintoo, verified by applying both to the pre-fixsource and running the full suite, which passes. They write to the slot past the shrunken
active set, only ever read as
[:nact].They were missing from the old baseline because mutmut caches per-mutant verdicts and a run
that changes only tests reuses them — it reported the seven above as un-killed until the
cache was cleared, at which point all seven died and the killed count moved by 77. Now
documented in
MUTATION.md.Divergence this introduces
A problem whose infeasibility is itself at the rounding floor may now be solved here and
rejected by the reference. Recorded under README → Deliberate deviations.
Testing
make testpasses locally — 1013 passed, 100% coveragemake fmthas been runmake mutationre-run from a cleared cacheChecklist
CHANGELOG.mdentry — generated from the commit by git-cliffmake depspasses (no unused or missing dependencies)🤖 Generated with Claude Code