Skip to content

feat(#860): widen the LP-per-node engine to mixed-integer and MAXIMIZE models - #874

Merged
jkitchin merged 13 commits into
mainfrom
claude/discopt-issue-860-1g34ns
Jul 26, 2026
Merged

feat(#860): widen the LP-per-node engine to mixed-integer and MAXIMIZE models#874
jkitchin merged 13 commits into
mainfrom
claude/discopt-issue-860-1g34ns

Conversation

@jkitchin

@jkitchin jkitchin commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

Contributes to #860 — not Closes. Measured after gating: with the flag ON at time_limit=60, all three instances #860 names are now in scope (_is_in_scope=True, where they previously failed the gate) but none returns an incumbentrsyn0805m04hfsg (MAXIMIZE) bound 9741.371 vs oracle 7174.219, gastrans040 bound 0.0, gastrans582_cold13 bound 0.0, objective=None on all three (6 soundness checks, 0 violations). #860's stated gap is that these three return no incumbent, so it stays open for the primal work.

The widening is now opt-in behind DISCOPT_LP_SPATIAL_MIXED at both entry points, including solve(lp_spatial=True). It is sound but not net-positive on the default path (CLAUDE.md §5 bar 2): on gear4 the pre-#860 gate certifies 1.6434284641 in 3 nodes / 1.1 s, while the widened gate returns 17.514 uncertified over 2678 nodes / 25 s. See the last commit for the full rationale.

The LP-per-node spatial engine (_jax/lp_spatial_bb.py) was gated to pure-integer,
MINIMIZE
models, so the mixed / maximize class could not even be offered to it — no
primal heuristic inside it could help instances it refused to accept. This widens the
scope to "≥1 integer variable, either objective sense, any continuous mix".

The issue's named gate probes (gastrans040, rsyn0805m04hfsg, gastrans582_*) are
not vendored in this repo and this environment has no network route to minlplib.org, so
every number below comes from the in-repo corpus (minlplib_nlminlplib, 119
distinct instances) — which is dominated by exactly this class: 71 mixed, 5 MAXIMIZE,
including syn05m/syn05hfsg, the same syn/rsyn family as the issue's maximize
probe.

The entry experiment reframed the work. Measuring the existing gate showed
mixed-ness was not the binding constraint: 31 of the 71 mixed instances were rejected
by the all-variables-finite box test
(real MINLPs leave continuous columns unbounded
above), 28 by the incremental patch declining, and only 11 reached a root LP at all. A
widening that only relaxed the variable-mix test would have been close to a no-op on the
real class — the #727 RLT lesson. Under the proposed gate, at the root box only, no tree:

metric (71 mixed instances) current gate widened gate
finite root McCormick bound 11 46
verified feasible point (round + LP completion) 4 13

The kill criterion did not fire. On the maximize family the blocker is again the box:
both in-repo maximize instances have no valid objective bound over the raw box, and
root OBBT finitizes every infinite upper bound in 0.2–0.4 s (syn05m −1165.78,
syn05hfsg −1335.50, minimize-equivalent).

What changed: sense handled in minimize-equivalent space (sgn applied exactly once, at
the exit — both uniform_relax and NLPEvaluator already negate for a MAXIMIZE);
continuous spatial bisection with children sharing the midpoint; a mixed-integer
primal complete() that fixes the integers, re-solves the node LP for the continuous
coordinates and verifies the point exactly; feasibility pump on integer coordinates
only; partially infinite root boxes accepted; deadline-bounded cold node builds.

Full write-up, measurements and known limits:
docs/dev/issue-860-lp-spatial-mixed-scope.md.

Correctness

Three properties carry the widened scope, and Panel A (below) measures all three over
every in-scope in-repo instance:

  • Sense. The engine runs entirely in minimize-equivalent space, where both of its
    inputs already live. A valid lower bound on −f is a valid upper bound on f, so
    bound ≤ incumbent becomes bound ≥ incumbent for a maximize.
  • Continuous branching only ever shrinks boxes, so every child relaxation stays a
    valid outer approximation of its subtree. A candidate narrower than
    _MIN_BRANCH_WIDTH is unbranchable and folds into unresolved_lb, so it can never
    yield an optimality proof — the exact-leaf test still requires the whole box
    collapsed, continuous dimensions included.
  • Primal. An incumbent is never read off the relaxation. The LP completion is a
    relaxation of the continuous restriction, so its point is only ever a candidate,
    discarded unless the ground-truth evaluator accepts it.

This PR also fixes two soundness bugs it uncovered:

  1. A false-fathom on the DEFAULT path. IncrementalMcCormickLP solved min c·x but
    dropped the relaxation's obj_offset, so its node bound sat on a different origin
    from the cold build's. On a node whose true McCormick optimum is −92.0 the fast path
    reported +8.0 — a dual bound above the true optimum. This is mccormick_lp's
    incremental fast path (default since cert:T1.3), not only the opt-in engine.

    objective constant cold bound fast (before) fast (after)
    0 8.0 8.0 8.0
    −100 −92.0 +8.0 (unsound) −92.0
    +100 108.0 8.0 (weak) 108.0

    The offset is now carried on the structure and added back at every bound-returning
    exit including the certificate's safe_bound; _validate asserts it is
    box-independent, so a shape where it were not falls back rather than mis-reporting.

  2. Root product map read against a node's solution. The cold path rebuilds the
    relaxation per node and the lift is box-dependent, so a node's column count can differ
    from the root's — on st_e38, root aux column 18 against a 17-column node LP →
    IndexError, swallowed by the caller as "engine failed". The map now travels with the
    solution it describes.

  • Cannot produce a false certificate
  • No validation, fallback, or safety guard was weakened to make a check pass

Tests run (state the result — numbers, not adjectives)

  • pytest -m smoke837 passed, 15 skipped, 2 xpassed (7427 deselected)
  • pytest -m slow python/tests/test_adversarial_recent_fixes.py10 passed
  • cargo test -p discopt-coreN/A — no Rust touched
  • ruff check / ruff format --check clean; mypy clean on every changed module

Pre-existing failure, unchanged by this PR:
test_lp_spatial_bb.py::test_matches_brute_force fails on 3 of its 4 parameter sets
before and after (verified against base ac9f5cf — identical set): the engine finds
the true optimum but reports feasible rather than optimal. Documented post-#636
conservatism — a*b is lifted via univariate squares, so it never appears in the
engine's info map and no node short of a fully collapsed box is treated as exact.

Regression test

  • Added regression tests that fail before / pass after

  • test_incremental_node_bound_includes_objective_constant — before the fix the fast
    path returned 8.0 for every constant; at const=-100 that is a lower bound of +8.0 on
    a node whose true optimum is −92.0. Plus
    test_incremental_solve_bound_matches_cold_builder_with_constant one level down.

  • test_cold_path_product_map_travels_with_its_solutionst_e38 raised IndexError
    before; now returns feasible 7197.73 / bound 6795.05 in 1.9 s.

  • test_mixed_integer_in_scope_and_correct, test_maximize_bound_is_a_valid_upper_bound,
    test_maximize_never_overstates_the_optimum (brute force),
    test_infinite_root_box_is_accepted_not_declined — each returned None before.

  • test_legacy_gate_still_declines_mixed, test_maximize_legacy_gate_returns_none,
    test_pure_continuous_stays_out_of_scope pin the new boundary;
    test_860_mixed_flag_defaults_to_off_and_gates_the_reserve pins the flag plumbing.

Bound-changing / performance change?

Harness scratchpad/panel860.py, scoring scratchpad/panel860_analyze.py, 20 s budget.

Panel A — engine soundness on the widened class (89 in-scope instances):

newly in scope legacy in scope
instances 70 19
engine declined 15 0
errors 0 0
verified incumbent 33 17
certified optimal 12 14
unsound bounds / incumbents 0 0
false optimality certificates 0 0

33 instances the engine previously refused outright now yield an independently
re-verified feasible point, 12 with an optimality certificate; the legacy pure-integer
class is untouched. Both maximize instances return the bound on the correct side
(syn05m / syn05hfsg: objective 837.732, upper bound 868.110, incumbent
independently feasible). bchoco06/07/08 decline on an iteration_limit root LP — a
conditioning problem, not a scope one.

Panel B — graduation gate for DISCOPT_LP_SPATIAL_MIXED (70 newly in-scope
instances, off vs on): cert-clean, but NOT net-positive → the flag stays default-OFF.

instance flag off flag on
tspn12 no incumbent (30.6 s) feasible 262.647 (9.3 s)
ex1252a feasible 183660.35 (24.5 s) feasible 149530.99 (14.9 s)
tls2 feasible 11.30 (20.1 s) NO INCUMBENT (13.6 s)
st_e31 feasible −2.00 (22.2 s) NO INCUMBENT (14.8 s)

gains=1 improved=1 lost_incumbents=2 cert_regressions=0 incumbent_verification_failed=0 unsound=0.

The widening is sound; what it cannot do is pay for itself here. The reserve hands 35%
of the budget to the fallback before knowing whether the engine can serve the model:
on tls2 and st_e31 the primary then runs out of time at 65% and returns nothing,
while the fallback declines anyway (require_incremental=True plus an infinite root
box). Budget taken from a path that was going to succeed, given to a path that never
runs. Sound but harmful stays OFF with the measurement recorded — the
DISCOPT_CUT_INHERIT rule. The 0.747 total wall ratio is not a win: the on-runs are
faster largely because they gave up earlier, on exactly the two instances that lost
their answer. The fix this points at — make the reserve conditional on the engine
actually being able to build — is a separate change with its own panel.

A pre-existing soundness flag, not from this branch: Panel B's independent verifier
rejects nvs22's incumbent at tolerance 1e-5 under both settings. Byte-identical
against base ac9f5cf — objective 6.058219942618198, gap_certified=True, max
constraint violation 2.641e-4, above the panel's 1e-5 and below the deliberately loose
1e-3 of discopt's own final guard, so nothing flags it today. Worth its own
investigation; out of scope here.

🤖 Generated with Claude Code

https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu


Generated by Claude Code

claude added 10 commits July 26, 2026 15:19
…MAXIMIZE

The LP-per-node spatial engine was gated to pure-integer, MINIMIZE models, so the
mixed / maximize class could not even be offered to it — no primal heuristic inside
it could help instances it refused to accept.

Entry experiment first (CLAUDE.md §4), on the in-repo corpus (the issue's named
gate probes are not vendored and this environment has no route to minlplib.org).
71 of the 119 instances are mixed; 5 are MAXIMIZE, including the syn/rsyn family.
Round 1 found the binding blocker was NOT mixed-ness: 31 of the 71 mixed instances
were rejected by the all-variables-finite box test, 28 by the incremental patch
declining, and only 11 reached a root LP. Round 2, under the proposed gate:

  finite root McCormick bound      11 -> 46 of 71
  verified feasible point at root   4 -> 13 of 46 (round + LP completion)

The kill criterion did not fire. On the maximize family the blocker is again the
box: root OBBT finitizes every infinite upper bound in 0.2-0.4s and the LP then
returns a valid bound (syn05m -1165.78, syn05hfsg -1335.50 min-equivalent).

Engine changes:
- scope: ">=1 integer variable, either sense, any continuous mix". Pure-continuous
  stays out — the convergence argument is that branching the integers drives the
  products exact. `_is_in_scope(model, mixed=False)` keeps the old gate.
- sense: the engine runs in minimize-equivalent space, where both producers already
  live (uniform_relax negates the LP cost, NLPEvaluator negates the point value), so
  `sgn` appears exactly once — on the reported objective/bound. `bound <= incumbent`
  becomes `bound >= incumbent` for a maximize.
- continuous branching: integers split disjointly at floor/ceil, continuous bisect at
  the midpoint with the children SHARING it. Candidates narrower than
  `_MIN_BRANCH_WIDTH` are unbranchable and fold into `unresolved_lb`.
- mixed primal `complete()`: fix integers, re-solve the node LP for the continuous
  coordinates, verify the completed point against the ground-truth evaluator — the
  LP completion is a relaxation, so its point is only ever a candidate.
- feasibility pump acts on integer coordinates only.
- infinite root boxes accepted: OBBT runs whenever the box is infinite, and whatever
  stays infinite goes to the cold builder (which drops rows it cannot make finite).
  The incremental patch has no such guard, so it is declined when a column it patches
  is infinite (`IncrementalMcCormickLP.box_is_patchable`).
- cold node builds are deadline-bounded (build + solve), since the widened scope
  routes far larger models through that path.

The default-path fallback reserve for the newly in-scope class is behind
DISCOPT_LP_SPATIAL_MIXED (default OFF) pending its graduation panel; the engine
itself is widened unconditionally.

Also fixes a soundness bug on the DEFAULT path, found while widening:
`IncrementalMcCormickLP` solved `min c·x` but dropped the relaxation's `obj_offset`,
so its node bound sat on a different origin from the cold build's. On a node whose
true McCormick optimum is -92.0 the fast path reported +8.0 — a dual bound ABOVE the
true optimum, the false-fathom class. The offset is now carried on the structure and
added back at every bound-returning exit including the certificate's safe_bound;
`_validate` asserts it is box-independent.

Verified: `pytest -m smoke` 837 passed / 15 skipped / 2 xpassed; the lp_spatial and
incremental-McCormick suites pass, including new fail-before/pass-after regressions
for the objective-offset bound and for mixed/maximize scope and soundness.
`test_matches_brute_force` fails on 3 of 4 parameter sets before AND after this
change (verified against the base commit) — a pre-existing post-#636 tightness gap.

Contributes to #860

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
…over maximize soundness and infinite root boxes

- test_844_unresolved_child_soundness: the injected `_relax_bound` now takes
  `**kw` so it keeps exercising the unresolved-child path instead of failing on
  the new `deadline` argument.
- test_lp_spatial_bb: brute-force maximize soundness (incumbent never above the
  true max, upper bound never below it) and a mixed model with an infinite
  continuous upper bound, which the pre-#860 gate declined outright.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
…back result

The #844 fallback merged its dual bound with `max`, which is correct for a
minimize (larger lower bound is tighter) but keeps the LOOSER of two valid upper
bounds for a maximize — reachable now that the engine serves maximize models.
Sound either way; this keeps the intent.

Also covers the new flag: the mixed model's result is unchanged by
DISCOPT_LP_SPATIAL_MIXED under either fallback setting, the flag defaults off, and
the engine's (widened) gate and the reserve gate can disagree — which is the point
of separating them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
… cold path

The cold path re-runs build_milp_relaxation at every node and the lift is
box-dependent, so a node's column count can differ from the root's. `info` was
captured once at the root and read against every node's `x`, which indexes past the
end of that node's solution vector — measured on st_e38: root aux column 18 against
a 17-column node LP, IndexError. The caller swallowed it as "engine failed" and
silently fell back, so the engine was simply unavailable on such models.

`relax`/`node_relax` now return the product map of the LP that produced the
solution, and it travels with the node through the frontier. Pre-existing on the
cold path; st_e38 reaches it only because #860 brought mixed models into scope.
After the fix st_e38 returns feasible 7197.73 with bound 6795.05 in 1.9s.

Regression: test_cold_path_product_map_travels_with_its_solution.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
…n, not a day

One process per solve is the cleanest isolation, but the fixed startup cost (JAX
import + parse + evaluator build) dominated at ~2.3 min per instance against ~1s of
actual solving — a 10-hour panel. The worker now takes a batch of instances and emits
one RESULT line each; batches stay small so a crash or a leak costs only that batch,
and every instance still builds a fresh model.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
…d relation

The panel analyzer inferred each instance's sense from whether the reported bound sat
above or below the objective — which is circular, since that relation is exactly the
soundness property under test, and it silently defaulted a bound-only maximize run to
the minimize check. Senses now come from a precomputed map read off the models
themselves.

Includes the in-flight Panel A results; the final panel output lands with the
verification write-up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
Panel A over all 89 in-scope instances (70 newly in scope, 19 the old gate already
served), 20s budget, every incumbent independently re-verified with a fresh
evaluator:

  newly in scope   70   declined 15   errors 0   incumbent 33   certified 12
  legacy in scope  19   declined  0   errors 0   incumbent 17   certified 14
  unsound bounds / incumbents: 0        false optimality certificates: 0

33 instances the engine previously refused outright now yield a verified feasible
point; the legacy pure-integer class is unchanged. Both maximize instances return
the bound on the correct side (syn05m / syn05hfsg: objective 837.732, upper bound
868.110, incumbent independently feasible). bchoco06/07/08 decline on an
iteration_limit root LP — a conditioning problem, not a scope one.

Panel B (the DISCOPT_LP_SPATIAL_MIXED graduation gate) is still running; its section
carries a PANEL_B_PLACEHOLDER until those numbers land.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
The graduation-gate panel writes its results file after each batch; this is an
in-flight snapshot, superseded by the final results and the verdict write-up.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
…ive, flag stays OFF

Graduation panel for DISCOPT_LP_SPATIAL_MIXED (20s budget, 70 newly in-scope in-repo
instances, off vs on; the other 49 take a bit-identical path since the reserve gate is
the flag's only consumer):

  cert-clean    0 certification regressions, 0 incumbent_verification_failed,
                0 unsound bounds, 0 false optimality certificates
  net-positive  FAILED — gains 1, improved 1, LOST 2

    tspn12    no incumbent (30.6s)        -> feasible 262.647 (9.3s)
    ex1252a   feasible 183660.35 (24.5s)  -> feasible 149530.99 (14.9s)
    tls2      feasible 11.30 (20.1s)      -> NO INCUMBENT (13.6s)
    st_e31    feasible -2.00 (22.2s)      -> NO INCUMBENT (14.8s)

The widening is sound — Panel A already showed that, with 33 newly reachable instances
gaining a verified incumbent and 0 unsound results. What it cannot do is pay for itself
on the default path. The reserve hands 35% of the budget to the fallback BEFORE knowing
whether the engine can serve the model: on tls2 and st_e31 the primary then runs out of
time at 65% and returns nothing, while the fallback declines anyway
(require_incremental=True + an infinite root box). Budget taken from a path that was
going to succeed, given to a path that never runs.

Sound but harmful stays OFF with the measurement recorded — the DISCOPT_CUT_INHERIT
rule. The 0.747 wall ratio is not a win: the on-runs are faster largely because they
gave up earlier, on exactly the two instances that lost their answer. The fix this
points at (make the reserve conditional on buildability) is a separate change with its
own panel.

Also: the analyzer now classifies objective drift by sense, so a better incumbent
(ex1252a) is not scored as harm.

Separately, Panel B's independent verifier rejects nvs22's incumbent at tol 1e-5 under
BOTH settings. Not from this branch: byte-identical against base ac9f5cf (objective
6.058219942618198, max violation 2.641e-4) — above the panel's 1e-5, below the
deliberately loose 1e-3 of discopt's own final guard, so nothing flags it today. Worth
its own investigation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bsy1zfNW8H1hZGg9SKfPu
@jkitchin

Copy link
Copy Markdown
Owner Author

Review: 🛑 DO NOT MERGE — reproducible false optimality certificate on gear4

One blocker, independently reproduced twice (review agent + a separate verification run), through a public API with no flag or env var.

Import-trap guard: all runs used a dedicated worktree at PR head with the main tree's _rust .so copied in, PYTHONPATH pinned, and every script asserting both discopt.__file__ and a PR-only marker ("issue #860" in lp_spatial_bb) before doing any work; baseline runs asserted that marker absent.


🛑 BLOCKER — crossed dual bound, certified, 5 orders of magnitude

gear4 is mixed (4 integer, 2 continuous, both continuous ub=inf), so _is_in_scope is False on main (engine declines) and True under this PR. True optimum 1.643428474 — asserted in this repo's own python/tests/test_gear4_false_infeasible.py:3 and in minlplib.solu.

Model.solve(time_limit=25, lp_spatial=True) on python/tests/data/minlplib/gear4.nl:

status objective bound gap_certified
main (3ea21389) optimal 1.6434284641 1.6434274718 True ✅
PR #874 optimal 184279.32477275995 184279.32477275995 True

MINIMIZE, so a valid dual bound must satisfy bound <= true_opt. It exceeds it by 1.8e5.

The incumbent itself is feasible (re-verified with a fresh NLPEvaluator + _check_constraint_feasibility; the objective re-evaluates to 184279.32477275995), so this is a false dual bound + certificate, not a false primal. Deterministic: identical on 4 independent runs, node_count=1, at gap_tolerance 1e-4 and 1e-9. lp_spatial=True is a documented public kwarg whose docstring this PR edits (solver.py:4645); DISCOPT_LP_SPATIAL_MIXED does not gate it.

Root cause — a guard downgraded from a decline to a flag, and the PR's own justification for that is empirically false. Main's lp_spatial_bb.py:258-259:

if not (np.all(np.isfinite(lb0)) and np.all(np.isfinite(ub0))):
    return None  # unbounded integer box: out of scope for this step

The PR keeps the same predicate but binds it to _inf_box (lp_spatial_bb.py:403), which now only triggers OBBT instead of declining. The accompanying comment states the case for that directly:

"It is not, however, a soundness boundary — see the two paths below."

gear4 is the counterexample. Measured:

root box cold _relax_bound root McCormick bound
raw (ub = [60,60,60,60,inf,inf]) 184279.32477276 ← above the true optimum
continuous ub capped at 1e6 0.0 (valid)
continuous ub capped at 1e7 0.0 (valid)

Both stated safeguards fail on it:

  • §2.4's claim that "the cold build_milp_relaxation … self-guards and merely loosens" is falsified: it returns _objective_bound_valid=True alongside an invalid bound on a partially infinite box.
  • The comment at lp_spatial_bb.py:405-409 — "OBBT finitizes every infinite upper bound in ~0.2-0.4 s and the McCormick LP then returns a valid bound" — is falsified: neutering obbt_tighten_root changes nothing (bound stays 184279.32477275995 at 1 node). OBBT is load-bearing in the design but does not finitize gear4, and nothing checks that it succeeded before the bound is trusted.

Not a one-off shape. Across both in-repo corpora, 23 of 104 engine-accepted instances have an infinite root endpoint — exactly the class the removed guard excluded. Of the 3 that returned status="optimal", 1 is false:

instance objective bound oracle verdict
gear4 184279.32 184279.32 1.643428474 UNSOUND
meanvarx 14.36926 14.36816 14.36923211 ok
util 999.57875 999.55772 999.5787502 ok

Why Panel A missed it: it checks only self-consistency — "the bound never crossing the best verified feasible point found by any run of that instance". gear4 is perfectly self-consistent (bound == objective at a genuinely feasible point). Only an external oracle catches it.


Process findings

The oracle corpus this PR says is unavailable is present locally. The PR and docs/dev/issue-860-lp-spatial-mixed-scope.md §0 state the gate probes "are not vendored in this repo and this environment has no network route to minlplib.org". ~/Dropbox/projects/discopt-minlp-benchmark/ — named in CLAUDE.md as the benchmark corpus and oracle — contains all of them plus minlplib.solu (=opt= gastrans040 0.0, rsyn0805m04hfsg 7174.219, lip 5685067.877, …). This is the direct cause of the blocker: both panels ran without an oracle, so a certificate five orders of magnitude off looked clean. Re-running Panel A against minlplib.solu takes ~20 min and finds it immediately.

The unconditional obj_offset fix is a default-path bound-changing change with no corpus differential. It is a genuine and correct soundness fix, but it ships default-ON on mccormick_lp's incremental fast path covered only by unit tests. Blast radius on the 66-instance corpus: 5 instances have a nonzero relaxation objective constant, 1 negative — nvs14 at −40792.141 (true optimum −40358.155), so the false-fathom class was live in-repo. Effect PR vs main: nvs14 still certifies −40358.15477 on both, wall 0.68 s → 1.85 s (expected — the over-optimistic bounds were over-fathoming). Nothing unsound, but §5's differential-panel discipline wasn't applied to the highest-blast-radius change in the PR.

The reserve flag design is right. DISCOPT_LP_SPATIAL_MIXED default-OFF with a recorded "cert-clean but NOT net-positive" Panel B (1 gained + 1 improved vs 2 lost: tspn12/ex1252a vs tls2/st_e31) is exactly the DISCOPT_CUT_INHERIT rule, honestly applied. The problem is that the engine's gate was widened unconditionally, and the ungated half is where the false certificate lives.


Second correctness finding — obj_offset misses one exit

The PR states the offset is "added back at every bound-returning exit". mccormick_lp._reverify_incremental_infeasible (mccormick_lp.py:875) recovers a node bound via inc.solve_assembled_full(..., c_override=c2), and c_override solves deliberately skip the offset. On the PR's own _const_qcqp(-100.0) fixture:

normal path  : bound = -92.0000000000001    <- correct (fixed by this PR)
reverify path: bound =   7.999999999999904  <- short by exactly obj_offset
DIFFERENCE   = -100.0 == obj_offset

Same class as the bug this PR fixes; for a negative offset it is an over-high node lower bound (false fathom). Pre-existing rather than a regression — but it is in a file this PR edits and the "every exit" claim is inaccurate. Reachable on nvs14 (offset −40792) if the C-38 false-infeasible recovery fires.


What came back clean — the widening machinery itself is well built

I could not break it anywhere except through the removed infinite-box guard.

  • Sense handling is correct. sgn applied exactly once at the exit; both producers (uniform_relax 3090/3208, NLPEvaluator._negate 318/344/636) already negate for MAXIMIZE, so the loop legitimately needs no sign. Verified against minlplib.solu: syn05m cert-optimal 837.7323995 (oracle 837.7324009, bound above it ✓); lip feasible 5665067.89 ≤ oracle, bound 5769241.72 above it ✓; syn05hfsg 837.7324009, bound 868.11 ✓.
  • MAXIMIZE fallback-merge arithmetic is right — picks min (tighter upper bound) for maximize, max for minimize, never crosses the incumbent. 12 executed assertions, fired-counter proving the branch ran.
  • Mixed exact-leaf is handled correctly — the collapse test (lp_spatial_bb.py:880) spans all columns, so a node with a live continuous dimension folds into unresolved_lb and cannot certify. Probed on a model whose optimum requires continuous spatial branching (root bound −2, true optimum −1, both senses): closes correctly, never certifies at the loose root bound.
  • Fuzz, engine-direct: 360 random mixed/maximize models, 991 executed assertions, 0 violations. End-to-end with the flag on: 70 models, 290 assertions, 0 violations.
  • Oracle panel: 104 in-scope instances, 104 bound + 78 objective checks, 0 errors, 1 violation (gear4).
  • Integer / nonconvex-MINLP primal heuristics workstream (LP-based FP + sub-MIP RENS + structure-aware constructors) #844 regression set (default flags, PR vs main, 20 s): nvs04 0.72 / nvs06 1.7703125 / nvs09 −43.134336918 / nvs15 1.0 — all four bit-identical in objective, bound and gap_certified. tln4/5/6 sound on both.
  • pytest -m smoke 844 passed / 1 skipped / 2 xpassed; adversarial 10 passed; ruff + format + mypy clean; gh pr checks 874 green; no Rust touched.

Nits

  • ruff check scratchpad/ → 23 errors on the new harnesses (CI doesn't lint scratchpad/; consistent with existing files).
  • ~3,072 lines of panel JSON committed — matches repo convention but heavy.
  • docs/dev/issue-860-lp-spatial-mixed-scope.md §4 flags a pre-existing nvs22 incumbent-verification issue (violation 2.641e-4, byte-identical on the base commit) — correctly scoped out, worth its own issue.

What would unblock

  1. Make _inf_box a decline again unless OBBT actually finitized the box — i.e. re-check np.all(np.isfinite(...)) after obbt_tighten_root and return None if it still fails — or gate infinite-box acceptance behind its own default-off flag. The PR already recomputes _inf_box post-OBBT at line 440 but never acts on the result; acting on it would decline gear4.
  2. Better: root-cause build_milp_relaxation returning _objective_bound_valid=True with a bound above the true optimum on a partially infinite box. That is a soundness bug in the relaxation layer, of wider consequence than this engine.
  3. Re-run Panel A against ~/Dropbox/projects/discopt-minlp-benchmark/minlplib.solu (present locally) rather than self-consistency, and re-state the "0 false optimality certificates" claim on that basis.
  4. Add gear4 + solve(lp_spatial=True) as a fail-before/pass-after regression test.
  5. Separately: carry obj_offset through _reverify_incremental_infeasible, or correct the "every bound-returning exit" claim.

The prior finite-box gate was load-bearing, not conservative decoration. Downgrading it to an OBBT trigger produces a certified optimal at 184279.32 where the true optimum is 1.6434 — through a public API, on an instance this repo already treats as a gate probe. The one-line version of the fix is at line 440, already computed and currently discarded.

@jkitchin

Copy link
Copy Markdown
Owner Author

Update on the blocker: root-caused, and it is not in this PR.

The false certificate I reported (gear4: certified optimal at 184279.32 against a true optimum of 1.643428474) is a pre-existing defect in the LP presolve, which this PR exposes rather than introduces. Fix: #877.

crates/discopt-core/src/lp/simplex/presolve.rs decided whether a column's contribution was unbounded by testing the product a_ij * x_j against the sentinel INF = 1e20. INF is a sentinel, not a true infinity, so for |a_ij| < 1 an unbounded column contributes an ordinary finite number (-0.5 * 1e20 = -5e19) that fails the test and is booked as finite — after which catastrophic cancellation (ulp(5e19) = 8192) makes the residual come back as 0.0. On gear4 that fabricated the tightening x >= 35.2244 on a column whose optimal value is 0, cutting the optimum out of the box. Presolve is dimension-preserving with no postsolve, so B&B then certifies optimal on the mutilated box.

Not gear4-specific: 11 of 174 random LPs with unbounded columns returned a claimed-OPTIMAL above the true minimum before the fix, 0 after.

This corrects my earlier root-cause wording, and one framing in the review. I wrote that both safeguards in §1/§2.4 were "empirically false". The measurements stand, but the reason was not that build_milp_relaxation is unsound on a partially infinite box — the relaxation is fine (A, b, c, obj_offset byte-identical between the raw and capped builds; scipy/HiGHS returns 0.0 on the identical LP the in-house path scored at 184279.32). So please disregard unblock item 2 ("root-cause build_milp_relaxation returning _objective_bound_valid=True with an invalid bound") — there is no bug there.

What this means for this PR. The finite-root-box guard was load-bearing, but not for the reason either of us thought: it was masking a presolve bug for the pure-integer engine, not compensating for a weak relaxation. With #877 merged, gear4 through this PR's widened scope should certify soundly, and the guard's removal becomes defensible on its own terms.

Suggested path: rebase on #877 once it lands, then re-run Panel A against ~/Dropbox/projects/discopt-minlp-benchmark/minlplib.solu rather than self-consistency, and re-state the "0 false optimality certificates" claim on that basis. The other review items are unchanged — in particular the obj_offset gap at _reverify_incremental_infeasible, and the fact that the engine's gate was widened unconditionally while only the behaviour sits behind DISCOPT_LP_SPATIAL_MIXED.

Everything else in the review stands as written: sense handling, the MAXIMIZE fallback-merge arithmetic, mixed exact-leaf collapse, and the #844 regression set all came back clean, and the reserve-flag design remains a textbook application of the DISCOPT_CUT_INHERIT rule.

@jkitchin

Copy link
Copy Markdown
Owner Author

Blocker cleared — measured, not inferred. One new finding.

#877 is merged. I rebased this branch onto it (10 commits, clean, no conflicts) and re-ran the blocker case, asserting both markers before measuring: the issue #860 widening present in the loaded lp_spatial_bb, and the #877 presolve probe returning 0.0.

gear4, solve(time_limit=25, lp_spatial=True):

status objective bound gap_certified nodes
this PR before #877 optimal 184279.32 184279.32 True 1
this PR after #877 time_limit 17.514 None False ✅ 2673

The false certificate is gone. 4 executed soundness checks (bound-vs-oracle and objective-vs-oracle at gap_tolerance 1e-4 and 1e-9), 0 violations. My earlier statement that this PR "must not merge" on soundness grounds is withdrawn — the defect was in the LP presolve, and this PR only exposed it.

New finding: gear4 is now a quality regression rather than a soundness one

Same instance, same 25 s budget:

status objective bound certified nodes
main (default path, engine declines) optimal 1.6434284641 1.643427472 True 3
this PR (engine accepts) time_limit 17.514 None False 2673

main certifies the true optimum in 3 nodes; the widened engine burns the full budget over 2673 nodes and returns an incumbent ~10.7x worse, uncertified. This is the same "spend the whole budget, return no certificate" shape the require_incremental guard in #858 was added to prevent, and it is exactly what the removed finite-root-box guard used to avoid for free.

So the merge question moves from sound? to net-positive? — CLAUDE.md §5 bar (2). Since the engine's gate widening is unconditional while only the behaviour sits behind DISCOPT_LP_SPATIAL_MIXED, this regression ships default-ON. Two ways forward, either fine by me:

  1. put the gate widening behind the same flag, so the default path is unchanged and the flag carries both the gain and the loss; or
  2. keep it unconditional but show a corpus-wide net-positive panel that includes instances like gear4 — i.e. re-run Panel A against ~/Dropbox/projects/discopt-minlp-benchmark/minlplib.solu (present locally, contrary to the doc's §0) and report quality alongside soundness.

Unchanged from the earlier review

I have not pushed the rebase; the measurement was done in a local worktree. Say the word if you want me to push it.

@jkitchin

Copy link
Copy Markdown
Owner Author

Measured: this PR does not close #860 — 0/3 named instances gain an incumbent

Rebased onto current main (which now includes #873 and #877) and measured the three instances #860 names, at time_limit=60, lp_spatial=True. Both build markers asserted first (the issue #860 widening present, and the #877 presolve probe returning 0.0), load 1.78.

instance _is_in_scope status objective bound certified nodes oracle
rsyn0805m04hfsg (MAXIMIZE) True time_limit None 9741.371 False 3 7174.219
gastrans040 True time_limit None 0.0 False 77 0.0
gastrans582_cold13 True time_limit None 0.0 True 0 0.0

6 executed soundness checks, 0 violations — every bound is on the correct side of its oracle, and there is no false primal. The widening itself does what it says: all three are now admitted, where before they failed the gate.

But #860's stated gap is that these three "return no incumbent", and all three still return no incumbent. So this PR advances the issue without closing it. Contributes to #860 is the accurate keyword.

One defect visible in the table

gastrans582_cold13 reports gap_certified=True with objective=None, bound=0.0, nodes=0 — a certified gap where nothing was proven. That is exactly the defect #878 fixes (gap_certified now requires both ends of a gap), and #878 merged after this measurement, so rebasing again should clear it. Flagging it because it is the kind of thing that reads as a certificate to a consumer checking the flag before the values.

The branch no longer merges cleanly — resolution, for the record

Merging main produced two conflicts. Both are semantic and both resolve by composition, not by choosing a side — worth stating so the resolution isn't guessed at:

modeling/core.py — main (#861 review, d172ffa7) moved the fallback dual-bound merge out of the if _fb.objective is not None block, so a sound bound is reported even with no incumbent. This PR made the same merge sense-aware (min for MAXIMIZE, since a maximize dual bound is an upper bound and max would keep the looser one). Correct resolution is main's placement + this PR's sense-awareness; dropping either loses a real fix.

_jax/incremental_mccormick.py — main relaxed _validate to compare shape[1] with vacuity-filtered row sets and bounds-checked-first (which is what lets a pinned box validate), while this PR added the obj_offset box-independence assertion. Correct resolution is main's validation structure + this PR's offset assertion.

I verified that resolution locally: ruff, ruff format and mypy all pass on it, and the measurement above ran on it.

Where that leaves the merge decision

Soundness is settled (the gear4 false certificate was #877's presolve bug, fixed and merged). What remains is unchanged from my previous comment and is now sharper:

  1. Net-positive is still unestablished, and the gate widening is unconditional. gear4 regresses from main's certified 1.6434 in 3 nodes to 17.514 uncertified over 2673 nodes, and that ships default-ON while only the behaviour sits behind DISCOPT_LP_SPATIAL_MIXED. My suggestion stands: put the gate behind the same flag, so the flag carries both the gains and the losses and the default path is untouched.
  2. obj_offset still misses _reverify_incremental_infeasible (mccormick_lp.py:875).
  3. Add gear4 + lp_spatial=True as a regression test.
  4. Re-run Panel A against ~/Dropbox/projects/discopt-minlp-benchmark/minlplib.solu rather than self-consistency.

With the gate flagged and ClosesContributes to, this is mergeable on its own terms as a sound, default-off capability. As it stands it changes default behaviour with a measured regression and does not close the issue it names.

Instrument caveat

My probe printed int=0 for all three instances, which contradicts the issue's own counts (284 binary + 12 integer for rsyn0805m04hfsg, 48 integer for gastrans040). That is my probe reading a wrong attribute name via getattr(..., False), not a claim about the models — _is_in_scope returning True for a mixed model is the load-bearing observation and it is independent of that count. Noting it so nobody takes the int=0 column as data.

jkitchin added 2 commits July 26, 2026 18:39
…_MIXED

Review outcome on #874. The widening is sound and stays; what changes is that it
no longer ships by DEFAULT, because it is not net-positive on the default path
(CLAUDE.md 5 bar 2).

The flag already governed the #844 fallback's 35% budget reserve, but the ENGINE
gate was unconditional, so solve(lp_spatial=True) -- a documented public kwarg --
picked up the widened scope with no opt-in. Measured on gear4, which is mixed (4
integer, 2 continuous with infinite upper bounds) and therefore admitted only
under the widening, at a 25 s budget:

  DISCOPT_LP_SPATIAL_MIXED=0 (default): optimal, 1.6434284641, certified,
                                       3 nodes, 1.1 s
  DISCOPT_LP_SPATIAL_MIXED=1:          time_limit, 17.514, UNcertified,
                                       2678 nodes, 25.0 s

The engine accepts a model the default path already certified in 3 nodes, then
spends the whole budget to return an incumbent ~10.7x worse with no certificate.
It is SOUND -- the bound never crosses the oracle, and the false certificate seen
on gear4 earlier in review was the LP-presolve INF-sentinel bug fixed in #877,
not this widening -- but shipping it by default trades a certificate for a worse
incumbent.

Changes:

* solver.py's lp_spatial=True entry point passes
  mixed=_lp_spatial_mixed_fallback_enabled(); the #844 fallback already did.
* solve_lp_spatial_bb(mixed=...) and _is_in_scope(mixed=...) now default to
  False, so a NEW call site inherits the pre-#860 gate rather than silently
  shipping the widening. Same rationale as row_scan_is_anytime defaulting to
  False in the tightening rules.
* The flag docstring now describes both entry points it governs and records the
  gear4 measurement alongside the existing Panel B table.
* Five tests in test_lp_spatial_bb.py exercise the widened CAPABILITY, so they
  now request it explicitly with mixed=True. The two tests that assert the
  pre-#860 gate declines (mixed=False) are unchanged and still pass.

Also in this branch: merged main and resolved two conflicts by COMPOSITION, not
by choosing a side (rationale recorded in comments at both sites).

* modeling/core.py -- main (#861 review) moved the fallback dual-bound merge out
  of the `_fb.objective is not None` block so a sound bound is reported even with
  no incumbent; this branch made the same merge sense-aware (min for MAXIMIZE,
  where the dual bound is an UPPER bound and max would keep the looser one).
  Kept main's placement AND this branch's sense-awareness.
* _jax/incremental_mccormick.py -- main relaxed _validate to compare shape[1]
  with vacuity-filtered row sets, bounds checked first, which is what lets a
  PINNED box validate; this branch added the obj_offset box-independence
  assertion. Kept main's structure AND this branch's assertion.

Tests: python/tests/test_860_mixed_gate_is_opt_in.py -- 6 tests pinning the
GATING rather than wall-clock, so they cannot rot into machine-speed assertions:
mixed=True admits mixed-minimize and integer-maximize while mixed=False declines
both (with pure-integer-minimize as a control that must be admitted either way),
both defaults are False, a source-level guard fails if either production call
site stops threading the flag, and the env-var helper is off when unset.
Fails-before/passes-after verified by stashing the three source files: 2 failed /
4 passed before, 6 passed after.

Verified: pytest -m smoke -> 844 passed, 1 skipped, 2 xpassed; pytest -m slow
test_adversarial_recent_fixes.py -> 10 passed; ruff check python/ clean; ruff
format clean; no Rust touched.

Also measured, and the reason this is not "Closes": at time_limit=60 with the
flag ON, all three instances #860 names are now IN SCOPE (_is_in_scope True,
where they previously failed the gate) but NONE returns an incumbent --
rsyn0805m04hfsg (MAXIMIZE) bound 9741.371 vs oracle 7174.219, gastrans040 bound
0.0, gastrans582_cold13 bound 0.0, objective None on all three, 6 soundness
checks and 0 violations. #860's stated gap is that these three return no
incumbent, so it remains open for the primal work.

Contributes to #860
… the stale gate assertion

Two CI failures from the previous commit, both real.

1. My new test called importlib.reload(discopt.solver_tuning) to re-read the env
   var. That was unnecessary -- _lp_spatial_mixed_fallback_enabled reads
   os.environ directly with no caching -- and actively harmful: reloading the
   module replaces the SolverTuning CLASS OBJECT, so unrelated
   isinstance(..., SolverTuning) assertions later in the same xdist worker fail.
   It broke test_result_io::test_options_from_payload_rebuilds_tuning and
   test_solve_daemon::test_solve_request_rebuilds_tuning, neither of which this
   branch touches. Removed the reload (monkeypatch alone is sufficient and
   correct), added a "0" case, and recorded why no reload belongs here so it is
   not reintroduced.

2. test_844_lp_spatial_fallback::test_860_mixed_flag_defaults_to_off_and_gates_the_reserve
   asserted `_is_in_scope(m) is True  # engine: widened unconditionally`. That is
   precisely the behavior the previous commit changed, so it had to be updated --
   updated with the flag's new scope, not loosened. The substantive assertion
   (mixed=False declines a mixed model) is unchanged; it now additionally asserts
   that the DEFAULT is the conservative gate, which is the new invariant. The
   docstring records what it used to say and why it changed.

Verified with CI's exact PR-fast selection rather than the smoke subset that
missed these:

  pytest python/tests/ -m "not slow and not correctness and not integration and
  not amp_benchmark and not requires_cyipopt and not memory_heavy"
  --ignore=test_correctness.py --ignore=test_amp.py
  -> 7255 passed, 29 skipped, 5 xfailed, 2 xpassed, 0 failed (622 s)

Also ran the three previously-failing files plus the new one in a SINGLE process
(-p no:randomly) so the cross-test contamination would reappear if it were still
there: 42 passed.

Contributes to #860
@jkitchin
jkitchin merged commit f6fe02e into main Jul 26, 2026
11 checks passed
@jkitchin
jkitchin deleted the claude/discopt-issue-860-1g34ns branch July 26, 2026 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants