Whole-plan constraints: one resolver, so the engine and the validator agree - #135
Conversation
|
Amended for review — B1 (blocking), B2, B3 and both nits. B1 — the exact path bypassed the resolver. Confirmed and fixed. Three guards, each verified to fail against the un-fixed encoding and pass with it:
B2 — B3 — Nits. The merge note now skips a cardinality pair whose intersection is empty — "select 4–2" under "their tightest combination" described a range the model applies, and the validation error beside it already states that case correctly. The Full suite: 964 passed, 2 skipped. Test count for the PR is now 12 in Rebase: #132 had not landed on |
… agree max_allocation and cardinality describe the WHOLE plan, so several rows of either type resolve to a single applied value — and the two sides of the engine resolved them differently. _parse_constraints kept the LAST row while the pre-solve checks read the FIRST via next(...), so validate could hard-error a model on a cap the solver was never going to apply. Verified both ways: max_allocation 30 then 60, three options — solver applies 60 (feasible), while validate reasons about 30 and errors "cap (30%) × 3 = 90% < 100%": ready=False on a model that solves. cardinality (1,3) then (1,5) with four force_includes — solver applies max 5 (feasible), while validate errors "force_include count (4) exceeds cardinality max (3)". Fix, by the same vocabulary-coherence logic the allocation_bound merge used: several rows INTERSECT (tightest cap; range intersection), and both sides read one pair of resolvers — merged_max_allocation() and merged_cardinality() — which is what keeps them in agreement. Resolution is order-independent, a single row resolves to itself unchanged, validate warns per merged type, and an empty cardinality intersection is an error instead of a silently applied range. Also closes the create-time echo gap: constraints passed at model create now get the same validation_issues + constraints_merged_note echo an update gives them, rather than going unchecked until the next update or the solve. Docs: architecture.md create/update rows, the Cardinality/MaxAllocation model docstrings, the constraints param description, the create docstring, and the problem_framing constraint schema. Tests: 8 in tests/test_optimizer.py (both disagreements, order independence, intersection, empty range, warnings, single-row identity, the group-floor check's cardinality max) and 2 in tests/test_server.py (update echo, create echo). Full suite: 959 passed.
Review found the resolvers had consumers I missed, one of them blocking. BLOCKING — the exact path bypassed merged_cardinality. _build_milp_data still read a single row into mc["card"], and mc is what both exact backends encode, so the NSGA half intersected while the exact half kept the last row: a NEW engine-vs-engine disagreement (rows (1,2) then (1,5) — NSGA applies (1,2), HiGHS returned plans of size 1–5). explore certify compares exactly those two frontiers, so its optimality gap and its NSGA-never-dominates invariant would have been measured across two different feasible sets. mc["card"] now resolves through merged_cardinality and declines an empty intersection in words. Three guards, all verified to fail against the un-fixed encoding: the mc["card"] == merged_cardinality pin, the decline, and an end-to-end HiGHS frontier check (nothing in the suite exercised duplicate cardinality on the exact path, which is why it was green despite the bug). analyze_infeasibility diagnosed an empty search against a range the search never applied, and appended a cardinality entry per row — duplicating the binding rule AND its suggestion. Now resolver-backed, with one cardinality entry however many rows, in the per-rule list and the jointly-infeasible fallback alike. Verified: rows (1,2)+(1,5) with three force_includes used to blame cardinality twice; it now names the forced trio, which is the actual conflict. solution_quality read the FIRST max_allocation row for its at-a-bound flag (the fourth next(...) of the family), so with rows 60 then 30 the flag reasoned with 60 while the engine applied 30 and silently missed pinned allocations. Nits: the merge note skips a cardinality pair with an empty intersection (it had rendered "select 4–2" under "their tightest combination" — the validation error beside it says that case correctly); the max_allocation merge warning is gated on proportional, so a binary model no longer hears the applied value of a constraint just declared ignored.
Rebase resolution onto #132 plus the read-path gap the fold surfaced. _attach_constraint_merge_note is the single writer of constraints_merged_note, carrying #132's per-option allocation_bound boxes AND the whole-plan resolutions, called from create and update alike — two independent writers of one key is how one of them goes silently missing, which is the defect class these notes exist to prevent. Empty intersections are skipped on both kinds now: a note reports what the model applies, and the validation error beside it states the conflict. _formatted_constraints collapses every rule that collapses, not just allocation_bound: six rows in, three applied lines out, each captioned with the rows behind it. Rendering max_allocation and cardinality raw described a model the solver would not run — the same read-path defect #132 fixed one type at a time.
0db5330 to
cffd783
Compare
|
Rebased onto Resolution followed your notes exactly, plus one thing the fold surfaced:
One gap the fold exposed, now closed. With both merges live, the formulation card rendered Suite on the new base: 1009 passed, 2 skipped, plus 37 UI tests.
|
The defect class PR #132 fixed for
allocation_bound, next door — with a sharper symptom:max_allocationandcardinalityare not just last-writer-wins, the engine and the validator pick different rows.The disagreement
max_allocationandcardinalitydescribe the WHOLE plan (one global per-option cap, one selection-count range), so several rows of either type resolve to a single applied value._parse_constraintskept the last row (optimizer.py:max_allocation = c.max,cardinality_min/max = c.min/c.max) while the pre-solve conflict checks read the first vianext(...). Both verified againstmain:validatesaysmax_allocation30, then 60 (3 options)ready=False: "cap (30%) × available options (3) = 90% < 100%"cardinality(1,3), then (1,5), 4 ×force_includeready=False: "force_include count (4) exceeds cardinality max (3)"So the checkable half of the product hard-errors a model the solvable half would run — and which row wins depends on authoring order. The silent-loss half is the same as #132: two
cardinalityrows, the last one silently wins.Fix
Same vocabulary-coherence logic as #132: rows constraining the same thing intersect rather than overwrite — the tightest cap for
max_allocation, the range intersection (max of the mins, min of the maxes) forcardinality. The part that matters most here is where it lives:optimizer.merged_max_allocation()andmerged_cardinality()are the single resolvers, and both the solve encoding and every pre-solve check read them, which is what makes a future disagreement impossible rather than merely fixed._check_constraint_conflicts' threenext(...)reads (the global cap for allocation-bound floors, the cap sum, the group-floor-vs-cardinality max) now go through them too.validatewarns per merged type naming the applied value, and an emptycardinalityintersection (merged min > merged max) is an error instead of a silently applied range.model updateandmodel createechoconstraints_merged_note.Create-time echo gap (also closed): constraints passed at
model creategot no validation or merge echo at all — a one-shot framing call carrying twocardinalityrows learned what its model actually applies at the next update, or at solve. Create now runs the same non-scoring validation echo an update does, for the constraints it was given; a create without constraints keeps its lean response. Echoing matched the surface better than a docstring caveat, sinceupdatealready proves the pattern for the same content.Receipts
tests/test_optimizer.py::TestSingletonConstraintMerging(8 tests) — both disagreements reconstructed (engine and validator asserted to read the same value, with validate's verdict now describing the model the solver runs), order independence, range intersection, empty-intersection error, the per-type warnings, single-row identity, and the group-floor check reading the merged cardinality max.tests/test_server.py(2) — the update echo and the create echo, plus the negative cases.Full suite: 959 passed, 2 skipped.
Note for whoever merges second
This touches
optimizer.validate/_check_constraint_conflicts/_parse_constraintsand theconstraints_merged_notekey, which PR #132 also touches forallocation_bound. Branched offorigin/mainas asked, so expect a small textual conflict in those spots — the two merges are complementary (per-option box vs whole-plan value), and the resolution is to keep both lists feeding oneconstraints_merged_note.