Repository navigation
Merge next into master - #281
Merged
Merged
Conversation
…ingletons slack_singleton_action::presolve() treats a unit-coefficient integer singleton column as a free "slack" for its row: it folds the column's bounds into the row's rlo/rup and removes the column+row link entirely. This is unsafe whenever the row structurally encodes an assignment, packing, covering, or reduced-equality constraint (sum x == k, sum x <= k, sum x >= k, or an equality like col_j - col_i = rhs that reduces to the same all-unit-coefficient pattern): dropping the constraint lets B&B set the remaining row variable(s) to a value that violates the original row, and postsolve then reconstructs the substituted-out singleton's value from the (now-wrong) row activity, propagating the corruption into the final solution. The symptom is a clean integer/row violation (e.g. an assignment row with activity=2 against bounds=[1, 1]), not floating-point noise. The existing guard only protected the coeff == +1.0, sum-to-1 equality case. Broaden it to unconditionally skip substitution whenever the singleton has |coeff| == 1.0 and every other variable in the row is also a unit-coefficient integer, regardless of the singleton's coefficient sign or the row's bound structure (equality, <=, or >=). Verified against Cbc's mip-sanity-data regression suite (371 instances): fixes all of gap_5a30t_sd42_tight_anticorr_variable, gap_6a30t_sd137_tight_anticorr_heavy, gap_search_6a30t_sd1_a1.12_heavy, gap_search_6a32t_sd1_a1.1_heavy, gap_search_6a35t_sd3_a1.12_variable, gap_search_6a35t_sd3_a1.1_heavy, gap_search_6a38t_sd1_a1.12_heavy, gap_search_7a30t_sd1_a1.1_variable, gap_search_7a30t_sd3_a1.12_heavy, gap_search_7a35t_sd4_a1.1_heavy, gap_search_7a38t_sd3_a1.12_variable, cttp_small_c6t8_r2-3_sd3, cttp_hard_c10t12_r2-3_sd2, and several other previously-failing instances (349 -> 368 passed out of 371), with no correctness regressions (only minor gap-widening on a handful of already-passing instances from disabling more presolve reductions). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the binary-only knapsack bound propagation with mipster's more evolved implementation, which adds: - Feasibility-based bound tightening (FBBT) for continuous and general integer variables via interval-arithmetic activity bounds, not just binary singleton/knapsack fixing. - A combined scanRow() scanner that computes both the knapsack pre-check info and FBBT activity data (bEff, minAct, nUnbounded, unboundedK) in a single pass. - Kahan-compensated summation for row min-activity, fixing a real numerical-stability bug where floating-point overshoot could produce invalid (too-tight) bounds on some architectures. - Dirty-row tracking with cached per-row min-activity/unbounded counts so rounds after the first can skip re-scanning unaffected rows. - Optional case collection (collectCases) for external verification of tightening decisions. - General-integer bound floor/ceil with a primalTolerance guard band to avoid producing UB < LB purely from within-tolerance slack. Infeasibility detection now also covers FBBT bound crossing (in addition to the existing negative-effective-RHS and contradictory binary-fixing checks already present). Ported from mipster (~/dev/mipster), which has iterated on this class extensively and is the more tested implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously, a Clp-level wall-clock time limit (ClpMaxWallSeconds) was only checked between simplex iterations, but a single factorization call on a very large/dense matrix could run past the deadline with no way to interrupt it. Add a timeLimit_ member to CoinFactorization and check it periodically (every 64 pivots) inside factorSparseSmall() and factorSparseLarge(), setting status = -100 on timeout so callers can detect and react to a deadline overrun during factorization itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
topic_ (set via setTopic()) was omitted from both the copy constructor
and copy-assignment operator, so any copy of a CoinParam/CbcParam/ClpParam
(e.g. CbcParameters::operator=, used when parameters are round-tripped
through CbcSolverInterface) silently reset every parameter's topic to
empty. This broke Cbc's 'cbc -dumpParameters' JSON output (all topics
came back ""), which in turn made doc/generate_docs.sh produce empty
man page / LaTeX / Markdown / PDF parameter references ("Parsed 0
topics, 0 parameters").
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CoinFactorization::factorDense() (the LAPACK dgetrf-based dense fallback used when the remaining active submatrix is small/dense enough) had zero time-limit awareness, unlike factorSparseSmall()/factorSparseLarge() which already check timeLimit_ periodically inside their Markowitz pivot loop (commit 9223be3). Since dgetrf is a single atomic call that cannot be interrupted mid-flight once started, add two wall-clock deadline checks that return -100 (same convention as the sparse-path timeout) before ever committing to it: 1. At entry, before doing any dense-phase setup work at all - catches the case where the deadline was already exceeded before this factorize() call should have started (e.g. many B&B nodes/passes stacking up after time is already up). 2. Right before the dgetrf call itself, after the O(numberDense_^2) submatrix-building work - catches the case where that setup work itself consumed enough time to cross the deadline. This bounds the worst-case time-limit overshoot from a single dense factorization to roughly the setup cost, instead of unbounded (a single dgetrf call on a large dense block could previously run for minutes with no way for the caller's -sec/-maxNodes wall-clock deadline to be noticed). Verified on brazil3 (previously hung for minutes past -sec limits): with -sec 20 the process now stops at ~25.5s wallclock instead of running unbounded. Full CoinUtils/Cbc mip-sanity-data subset (bpc_*, kna*, set*, 22 instances) passes with no regressions. This is Tier 1 of a larger fix; Tier 2 (auditing CglPreProcess/root cut-generation/B&B node loops to stop scheduling more work once the deadline has passed, rather than relying solely on in-flight checks) is tracked as a follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CoinUtils' own zlib/bzip2/LAPACK detection is recorded in its internal, non-installed config header, so it isn't visible to downstream projects (Osi, Clp, Cgl, Cbc) that only see the public installed CoinUtilsConfig.h. Add small runtime accessors (CoinBuildInfo::hasZlib/hasBzlib/hasLapack) so consumers can query which optional features are actually present in the linked CoinUtils library, e.g. for Cbc's upcoming 'version' command build-info report. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…irst-call
CoinWallclockTime() previously captured an implicit per-process baseline
(via a function-local static) on its first invocation and returned time
elapsed since that moment. This was fragile in two ways:
1. The baseline depended on whichever code path happened to call this
function first, which could already be well into a solve, not true
process start.
2. In a long-lived process solving multiple independent models (e.g. a
library embedding such as Cbc's C interface or Python-MIP), the
baseline was fixed forever at the very first call in the process's
lifetime, so it could not distinguish "time since this solve started"
from "time since some earlier, unrelated solve started" -- silently
breaking any deadline computed as an absolute
CoinWallclockTime()-based value for a later solve.
Every correct use of this function computes a difference between two
calls (elapsed = now - startTime, where startTime was itself captured via
CoinWallclockTime()) or an absolute deadline (deadline = now + budget,
checked later via now >= deadline). Switching the baseline to the Unix
epoch (i.e. just returning CoinGetTimeOfDay()) is a no-op for all such
differences/deadlines, removes the hidden global state and the
magic-static branch on every call, and makes the value meaningful across
multiple independent solves in the same process.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lpers New CoinMemUsage.hpp header providing a portable way to query the current process's resident memory usage and the total installed physical memory, implemented per-platform for Windows (K32GetProcessMemoryInfo/GlobalMemoryStatusEx), macOS (task_info/sysctl HW_MEMSIZE), and Linux/other (/proc/self/statm + sysconf). Returns 0 when the value can't be determined on a given platform. This is used by Cbc to implement a new -maxMem branch-and-bound memory-limit stopping criterion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CoinDynamicConflictGraph's constructor could run for tens of seconds to minutes past a caller's deadline with no way to abort, since none of its three expensive scanning loops (main per-row scan, deferred tRowElements clique detection, small-clique node processing) ever checked the clock. This was identified as a root cause of CBC significantly overrunning its -seconds time limit on dense/large instances (reproduced on square47: conflict graph build always ran to completion in ~25-45s regardless of the nominal time limit). Add an optional absolute wall-clock deadline (timeLimit, -1.0 disables) threaded through CoinDynamicConflictGraph -> CoinStaticConflictGraph, following the existing CoinFactorization2.cpp pattern: periodic checks every 64 iterations to keep CoinWallclockTime() overhead low, breaking out early and leaving a partial-but-sound graph via timeLimitReached()/CoinStaticConflictGraph::timeLimitReached(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Serialize the conflict graph so a Bron-Kerbosch call can be replayed offline without a Cbc solve. Reaching either BK consumer -- clique separation or clique extension -- costs a full run to reach, which makes iterating on the algorithm itself impractical. The graph is serialized rather than rebuilt on the consumer side because a rebuild does not reproduce it. Cbc builds its graph before CglCliqueStrengthening deletes and adds rows, and checkCGraph() only rebuilds when the *column* count changes, so the graph a separator sees is routinely stale with respect to the matrix it separates on. Measured: rebuilding gives 190900 direct conflicts on trdtaunimep against the 191028 Cbc used, and 3531734 on co-100 against 3531732. That staleness is the behaviour under study. Format is binary, little-endian, fixed-width (uint64_t for every size_t) so a fixture stays readable across builds, with a magic + version header. nodeCliques_ is not stored but rebuilt from cliques_ exactly as the copy constructor does. Written through a single buffer rather than per-element, since the larger instances hold millions of direct conflicts. Also fixes iniCoinConflictGraph(const CoinConflictGraph*), which had `infeasibleImplications_ = infeasibleImplications_;` -- a self-assignment that silently dropped implications on every copy and clone. Independent of the above, but a round-trip test trips over it immediately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fillCandidates() verified every node conflicting with the clique's minimum-degree member against *every* clique element, then greedySelection discarded all but the maxCandidates_ (512) cheapest. On instances whose cliques are long the discarded majority dominated the run: eilA101-2 issued 9.77M conflicting() calls, two binary searches apiece, to keep 568k candidates of which 512 per clique survive. Since the caller keeps only the cheapest 512, verifying in cost order and stopping once 512 are accepted selects the same set -- CompareCost is a strict weak ordering, so "the cheapest k of all verified" and "verify in cost order, take the first k" agree. greedySelection therefore skips its partial_sort when fillCandidates already ranked and truncated; when there were fewer survivors than the cap nothing is skipped and the old path runs unchanged. Extension method 1 (random) keeps the old behaviour by passing costs = NULL: shuffle_array draws uniformly from the verified set, so truncating before the shuffle would change the distribution rather than just the cost. Measured on all 237 `sep` clique-separation fixtures: totalCuts, totalViol, maxViol, avgCutLen, objEnd, objImprove and boundMoved are identical on every one, so this is a pure speedup and not a change of answer. Serial timings, min of 5, on the fixtures that hold ~81% of all separation time: eilA101-2 2.167s -> 0.726s 2.99x eilD76.2 0.738s -> 0.336s 2.20x eilB101.2 1.334s -> 0.768s 1.74x eilC76-2 0.551s -> 0.327s 1.69x total over 15 fixtures 2.00x No clique-size gate: a sweep of thresholds 0/2/4/8/16 found ungated best everywhere. Short-clique fixtures (bnatt400, uct-subprob, ~10ms each) appeared to regress under a 12-way parallel harness and are unchanged when timed serially. mip-sanity suite unchanged: 471/471 PASS, 324 confirmed optima. The one flagged gap movement, pk1, is wall-clock noise -- the baseline binary alone yields bounds of 3.35, 3.41 and 3.98 across repeat runs at a *fixed* 20000-node limit, bracketing the value obtained here.
CoinCutPool's per-column best-score filtering (the fitness contest in updateCutFrequency()/calculateFitness()) was measured to be nearly worthless when few candidate cuts are submitted: instrumenting all three current consumers (CglBKClique, CglOddWheel, CglImpliedClique) with a debug env var (CBC_CLIQUE_POOL_DEBUG) and running the probe across 442 mip-sanity-data fixtures at root (29,398 pool invocations) showed: BKClique candidates filtered 1-5 0.4% 6-20 5.9% 21-100 22.6% >100 68.4% 86% of all BKClique pool calls fall in the <=20-candidate bucket, where filtering removes almost nothing but still pays the full per-cut fitness-scoring cost. ImpliedClique's filtering was negligible at every scale (0.7% overall, 0% even at >100 candidates). Changes: - CoinCutPool gains setFilteringEnabled(bool) (default true, i.e. unchanged behaviour for any caller that doesn't call it), letting callers who know their candidate count up front skip the fitness contest entirely when it isn't worth it. - CoinCutPool now always rejects exact-duplicate candidates (same sorted indices/coefficients/rhs, via a cheap FNV-1a-style hash into a new seenHashes_ set) regardless of filterEnabled_. This was needed because the original best-score contest implicitly rejected duplicates as a side effect (a tie in fitness never dethrones the incumbent), so disabling it would otherwise have reintroduced literal duplicate cuts -- this addresses a related concern that CglImpliedClique/CglBKClique can rediscover the same clique from different search starting points. - Added optional `tag` constructor parameter + CBC_CLIQUE_POOL_DEBUG env-gated destructor print (candidates/kept/filtered%/filterEnabled per pool instance), used for the above measurement and left in for future investigation. See the companion Cgl commit for the threshold wiring in CglBKClique/CglOddWheel/CglImpliedClique, and its commit message for end-to-end benchmark results (real cbc CLI, 442 mip-sanity-data instances): total time -0.29% on instances that succeeded in both configurations, one previously-timing-out instance now completing, dual gap closed +0.03%, primal gap closed -0.25% (noise-level).
Add setMaxParallelism()/filterByParallelism() to CoinCutPool, greedily selecting cuts by descending fitness while rejecting any candidate too cosine-similar to an already-kept cut. This mirrors HiGHS's HighsCutPool (maxpar=0.1) and SCIP's cutsel_hybrid/cutsel_dynamic (minortho=0.9, i.e. maxparallelism=0.1) cut-selection strategies, neither of which CBC previously implemented. Default maxParallelism_ is 1.0 (disabled): a 442-instance real-CLI A/B sweep (thresholds 0.1/0.3/0.5/0.9 vs. off) applied to CBC's clique-cut generators found no threshold gave a net win -- mean dual gap closed was essentially flat (+0.0% to +0.4%) while dual/primal gap-closed-per-second efficiency consistently worsened by ~1-2%, with several instances regressing sharply (e.g. -7.6% dual efficiency on fcnf_random_n15_d3 at 0.1). Unlike CoinCutPool's existing candidate- count-threshold filter (which was validated to help), parallelism filtering doesn't pay for itself for clique cuts in this pipeline -- apparently near-duplicate cliques still help reoptimization more than they cost as extra rows. Also refactor add()/updateCutFrequency() so calculateFitness() is always computed and cached in cutFitness_, even when filterEnabled_ is false, so filterByParallelism() has a meaningful score to sort by regardless of whether the count-threshold filter is active. Kept as opt-in infrastructure (env CBC_CLIQUE_POOL_MAX_PARALLELISM in Cgl's CglBKClique/CglOddWheel/CglImpliedClique) for further experimentation, e.g. combined with a persistent/aging cut pool. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hole
addOddHole() received (nz, idxs) where idxs is the shared tmp_ buffer,
sized cgraph->size() + 1, and only its first nz entries describe the
hole. It pushed the entire vector. Every consumer that asks the stored
hole for its size therefore got 2*numCols + 1 instead of nz, with the
tail carrying stale (mostly zero-initialised) node indices.
Three independent consequences, each measured over 237 replay fixtures
built from clique separation's root-cut-pass-1 dumps:
- oddHoleSize() reported 2*numCols + 1 and oddWheelRHS() reported
floor((2*numCols + 1)/2) = numCols, so CglOddWheel translated the
stale tail into cut coefficients. Every hole then tripped its
repeated-column guard and was discarded: 2528 of 2528 holes, and
the generator emitted no cut at all.
- alreadyInserted() skips a stored hole whose size differs from nz,
which could now never match, so the duplicate check never fired
once (oddHolesDuplicate == 0 over all fixtures).
- searchWheelCenter() rejects a candidate with degree < ohSize, and
with ohSize = 2*numCols + 1 no node in a doubled conflict graph can
qualify. wheelCenters == 0 everywhere, so extending methods 1 and 2
-- the odd-*wheel* half of the generator, and CBC's default -- were
unreachable code.
Store only the nz entries that describe the hole.
Hole finding itself is untouched, which the fixtures confirm as an
accounting identity rather than a tolerance: at --rounds=1 the six
search counters (activeColumns, arcs, spFindCalls, oddHolesShort,
oddHolesRepeatedNode, oddHolesNotViolated) are identical on 237 of 237
fixtures, and oddHolesFound 1227 + oddHolesDuplicate 1301 == 2528
exactly, so every hole that stops being emitted is one the broken
duplicate check was failing to recognise. wheelCenters goes 0 -> 45 and
row cuts added 0 -> 1116 on 32 fixtures.
Also add a Stats struct with the per-stage timers and rejection counters
used to establish the above; it is filled unconditionally, costing a
dozen clock reads against loops quadratic in activeColumns.
CoinOddWheelSeparator runs one shortest path per active node over a graph with 2 * activeColumns nodes, so every find() paid three O(nodes) sweeps -- CoinNodeHeap::reset() plus the previous_/dist_ init loops -- before doing any work. That is quadratic in the active count *regardless of how sparse the graph is*, and the separator's active subgraphs are extremely sparse: 6368 arcs over 36814 nodes on the bab6 fixture, 36 arcs over 3832 on fiball. On bab6 that is 18407 calls x 3 x 36814 slots of pure bookkeeping to find no odd hole at all. Most origins have no outgoing arc, so find() returns immediately and the bookkeeping *is* the cost. Record the slots each call writes and restore exactly those. The arrays end up holding the same values as a full rewrite rather than being valid-only-where- stamped, which is what keeps this invisible to callers: distance(), previous() and the previous() array accessor read the arrays directly and are unchanged, and CoinNodeHeap's contents are identical rather than merely equivalent, so every cost comparison and every tie-break in the heap order still comes out the same way. That last point is not cosmetic -- the arc weights are 1001 - 1000*x, they repeat often, and a different tie-break would silently return a different odd cycle. pos_ restores correctly for the same reason: a node only ever leaves a position by a write to that position, so pos_[x] != x implies position x was recorded. Both classes fall back to the full loop once the recorded set reaches a quarter of the nodes, and stop recording at that same point, so a dense active subgraph -- where most nodes are reached and the replay would lose -- pays at most nodes/4 extra sequential writes against the 2*nodes the old loop did unconditionally. Verified bit-identical on all 237 clique-separation replay fixtures at both --ext-method=2 and --ext-method=0: every one of the 40 non-timing oddwheel-bench CSV fields matches as a string, including the odd-hole and rejection counters, the cut and violation aggregates, and the LP objective before and after reoptimizing.
CoinConflictGraph::conflictInCliques and CoinStaticConflictGraph::nodeInClique both std::binary_search cliqueElements(), but nothing kept that list sorted. CoinDynamicConflictGraph builds a clique from a row whose columns are ordered by coefficient value, not by index, and CoinCliqueList::addClique stored the elements verbatim; CoinStaticConflictGraph then moved them across unchanged and save/load round-tripped them unchanged. So conflicting(a,b) returned *false* for pairs that really do share a clique -- silently, and only on the graphs where the row scan happened to emit out of order. CoinCliqueSet::insertIfNotDuplicate already sorts before inserting, which is where the invariant was already being assumed; this just makes the bare addClique() path establish it too, and sorts on deserialization so a graph read from a pre-fix file does not answer conflicting() differently from a freshly built one. Measured over the 896 serialized graphs in the replay fixture set: 5201 of 29138 cliques were unsorted (17.8%), concentrated in 6 instances -- neos-4532248-waihi, proteindesign121hz512p9, proteindesign122trx11p8, supportcase6, neos-3555904-turama, s100. An audit of every pair that conflictingNodes() reports and conflicting() denies found 163016 such pairs on waihi alone, all of them sharing a clique (0 asymmetric edges, 0 unsorted direct-conflict lists), and 0 after this change. Odd-wheel separation on waihi goes from 1 cut to 2. Direct conflicts were never affected: 0 of the 896 graphs had an unsorted directConflicts list.
The lazy reset in CoinShortestPath::clearState() and CoinNodeHeap::reset() replays a list of touched positions instead of rewriting all of them, which is a large win on a sparse active subgraph -- odd-hole search on bab6 walks 6368 arcs over 36814 nodes, so nearly every find() touched a handful of entries and paid two full O(nodes_) sweeps to undo it. It has one bad case, and it is the only one: when the subgraph is dense enough that most nodes are reached, recording pays fullResetThreshold() push_backs and then the full loop runs anyway. Measured at roughly 15% on the densest odd-wheel fixtures (qap10), against 1.49x on tSearch overall. So treat "this call outgrew the list" as evidence about the graph rather than about the call: stop recording for the next recordBackoff() = 32 uses, then probe again. That leaves the dense-case overhead under 1% while a graph that turns sparse is picked straight back up on the next probe. The skipped_ flag is what keeps the backoff from feeding itself. Without it a deliberately-skipped use ends in the full-rewrite branch and would extend the backoff again, so the state machine would never re-probe -- an off-by-one that turned the periodic probe into a permanent opt-out. Correctness does not depend on any of this: rewriting every entry is always a valid reset, both branches leave the arrays in the same state, and the heap order and tie-breaks are therefore untouched. Which branch runs is invisible to callers; only the cost differs.
prepareGraph() tested every ordered pair of active nodes with cgraph_->conflicting(): icaCount_^2 virtual calls, each doing a binary search. On the larger fixtures that is 10^9-10^10 operations, and the response so far had been to bail out rather than fix it -- CoinUtils 1a9301f is literally "add time check inside prepareGraph() O(n^2) loop", so on a big conflict graph the separator burned its whole budget in graph preparation and returned zero cuts. Walk each active node's neighbourhood instead, mapping neighbours through a posInActive[] array: O(sum of degrees) with no virtual dispatch and no per-pair search. The arc order comes out identical for free, which matters because the arc order decides which of several equal-length cycles the shortest path returns -- conflictingNodes() returns the neighbour set sorted, and fillActiveColumns() builds icaIdx_ in strictly increasing node order, so posInActive is monotone and sorted neighbours map to ascending positions. Three asymmetries between the two paths are handled explicitly rather than assumed away, all documented at buildForwardArcs(): conflicting(n,n) is false while the fast path of conflictingNodes() returns directConflicts() unfiltered; a pair recorded twice in the graph is reported twice by the walk and once by the pairwise loop; and a neighbour list that is not sorted would break the order argument, so it is checked, not trusted -- a backwards mapped position discards the walk and rebuilds pairwise (stats_.prepareUnsorted). Cost-gated at neighborWalkCost() < icaCount_^2, computed without forming the product so it cannot overflow on the 380804-node graphs in the fixture set. The gate is not hypothetical: the analogous sparse rewrite in Gomory was bit-exact and 100x *slower* on wide rows until it was gated. Over 237 fixtures the split is 135 walk / 35 pairwise / 67 no-prep. setVerifyPrepare() builds both ways and compares. It compares per node as *sets*, not position by position, because one extra arc early shifts every later position -- that once reported 164353 mismatches for a real disagreement of 395 arcs and said nothing about its nature. Splitting the symmetric difference by which method has the surplus is the actual diagnosis: arcs only the walk found mean conflicting() denied a real conflict, arcs only the pairwise loop found mean the walk missed one, and those have opposite verdicts. That distinction earned its keep. It is what showed the sole disagreement in the fixture set (neos-4532248-waihi, 790 = 2 x 395 arcs) to be arcs the *walk* found and conflicting() denied -- i.e. the unsorted-clique bug fixed in 315605a4, not a defect here. With that fixed, prepareMismatches is 0 and prepareUnsorted is 0 on all 237 fixtures, and the CSV is bit-identical on 236/237 at both --ext-method=2 and =0, the exception being waihi gaining the arcs the bug had been hiding.
Odd-cycle separation runs a shortest path in the bipartite double cover: each
conflict (x,y) becomes a forward arc (x',y'') and a reverse arc (y'',x'), and a
path from v' to v'' is an odd closed walk. Both copies of an edge must therefore
carry the same length.
They did not. The forward arcs get icaActivity_ = 1001 - 1000*x, which is not
integral, and the reverse loop copied it through
const size_t arcDist = spArcDist_[i2];
so every reverse arc was up to 1.0 shorter than its own forward twin -- and with
weights in [1, 1000] that is up to a 100% error on the cheapest arcs, the ones a
minimum-weight walk is most likely to use. A path was then cheaper the more
reverse arcs it took, which is a direction bias with no meaning in the problem.
spArcDist_ is a vector<double> and the forward path stores the exact value; this
is a typo, not a deliberate rounding.
The fix changes which cycles the search finds, so it is not bit-identical and
lands on its own. Sweeping all 237 replay fixtures with --check-validity:
--rounds=1: oddHoles 1228 -> 1281 (+4.3%), cuts 1117 -> 1119, summed bound
improvement 1086 -> 1088. 3 fixtures better, 2 worse, 232 identical.
--rounds=4: oddHoles 1634 -> 1676, cuts 1488 -> 1451 -- fewer cuts, and yet
summed bound improvement 5460 -> 5514 (+1.0%). 6 better, 2 worse, 229
identical. Best momentum1 +72.0, trdtatl9220 +3.0, trdHardCA4500-8 +0.5;
worst co-100 -21.5 and trdta99 -0.4.
The certificate passes on every one of the 1281 / 1676 wheels, all five certBad*
counters 0, so the extra cycles the corrected lengths expose are valid cuts and
not an artifact of a broken graph.
Odd-wheel lifting fires on 3.5% of odd holes (45 of 1281 over the 237
replay fixtures), and there was no way to tell whether that ceiling is
the graph's or the code's. Eight counters split it.
A wheel centre must conflict with every node of the cycle, so candidates
are drawn from the neighbourhood of the cycle's minimum-degree node -- a
correct superset. Each pooled node is then dropped by exactly one of
four filters or survives, and the counters satisfy
wcPool == wcRejInCycle + wcRejDegree + wcRejAdjacency + wcRejCost
+ wcCandidates
which is asserted by the bench, not assumed.
Measured, rounds=1 --ext-method=2 --max-seconds=0, pool 160,645 nodes:
in cycle 2,581 1.61% validity
degree < |C| 1,079 0.67% validity
not adjacent to all of C 156,768 97.59% validity
x < EPS and rc > MAX_RC 5 0.00% heuristic
survived 212 0.13%
So the ceiling is structural: almost no node in these graphs conflicts
with all five-or-more nodes of an odd hole. The one filter that is a
*heuristic* rather than a validity condition rejects five nodes out of
160,645, on three fixtures, so relaxing it cannot buy anything.
Purely additive: every non-timing CSV field is string-identical to the
pre-instrumentation sweep across all 237 fixtures.
searchOddWheels() runs one Dijkstra over the whole doubled graph per active
node, and on a large conflict graph almost none of them can succeed: over 337
replay fixtures 88.9% of the 945,677 calls end in oddHolesShort or
oddHolesRepeatedNode, i.e. they pay for a full shortest path and hand back a
walk the filters immediately discard. buildFutilityGate() decides that
*before* the call, from four certificates rather than a heuristic, so a skip
can cost time and never a cut:
1. Bipartite component -- admits no odd closed walk at all.
2. Triangle dominance -- any odd closed walk of length L >= 5 through v is
bounded below by acti(v) + max_{0<=k<=4}(h_k[v] + g_(4-k)[v]), with h_k
and g_k the least weight of a k-arc walk out of and into each node. If
some triangle through v beats that bound then the minimum-weight odd walk
through v *is* a triangle, and oddSize < 5 throws it away.
3. Outside the 2-core -- on no simple cycle, hence in no odd hole.
4. Some biconnected block containing v is non-bipartite, which is exactly
"v lies on an odd cycle", read off DFS depth parity in one iterative
Hopcroft-Tarjan pass.
4 is exact and strictly subsumes 1 and 3, so a single test makes the decision
and the two weaker predicates are kept only to attribute it -- which is what
gateBlockOnly prices: 9,538 skips on 49 fixtures that neither weaker
certificate reaches, so the exact test is not decoration. 2 is independent,
and its k-arc bound is the dominant lever: triangle skips go from 78,121 to
207,370 against the 2*actMin form it replaces, which collapsed on any instance
holding a variable at 0 or 1.
Bit-identical, checked rather than argued. On all 337 fixtures every one of the
63 non-timing CSV fields is string-identical with the gate on and off, and the
skipped calls account exactly:
delta spFindCalls == delta (oddHolesShort + oddHolesRepeatedNode) == gateSkipped
Separation 1374.06s -> 120.43s (11.41x) for 1.08s of gate, i.e. 0.9% of what is
left; worst regression on any single fixture -0.003s; 232 of 337 fixtures now
have every call proved futile. Confirmed on the live solver with -cgraph on
-cliqueCuts on -oddWheelCuts on -passC 1, serial, rootCuts/genCuts/root bound
identical on every instance:
chromaticindex1024-7 609.00 -> 32.10s (whole run 853.0 -> 274.8s)
chromaticindex512-7 110.00 -> 4.88s (whole run 177.1 -> 71.1s)
gfd-schedulen180f7d50m30k18 367.00 -> 23.40s (whole run 4197.9 -> 3774.9s)
8 further instances 748.59 -> 45.33s
setUseFutilityGate(false) exists so the "no cut lost" claim stays checkable
rather than asserted. setRecordNodeOutcomes() labels each active node with the
outcome its call reached; no aggregate counter can supply those labels, since
541,356 of 643,697 calls land in a single bucket. Both are off by default and
cost nothing when off.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BinPacking's rhs >= 1.1 guard leaves rows shaped like "x1+x2+...+xk - M*y <= 0" (i.e. "x1 OR x2 OR ... -> y", rhs typically 0) completely unclassified. Add a dedicated CoinRowHubImplication bucket for this common implication-constraint shape: same single-minority-sign-coefficient structure as BinPacking, but with rhs < 1.1 instead of >= 1.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CoinCut/CoinCutPool::add() now accept an optional caller-supplied tag (size_t, default SIZE_MAX) carried through unchanged by pool compaction (removeNullCuts()/filterByParallelism()), since it travels with the CoinCut object itself rather than a position in a parallel array. A new cutTag(i) accessor exposes it. This lets a caller that already has cuts as external objects (e.g. Cbc's OsiCuts collection) recover which of its original cuts survived the pool's fitness-based filtering, without needing to reconstruct fresh cut objects from the pool's post-filter data the way CglBKClique currently does. Used by the new generalized cut-pool filter for Gomory/MixedIntegerRounding2/Twomir/Probing cuts in Cbc (CbcCutPoolFilter.cpp).
nearestRational_ special-cases an integral double by casting straight to int64_t and returning success, skipping the accuracy check that guards every other exit. An integral double can still be far outside int64_t's range, where the conversion is undefined behaviour: on x86 it yields INT64_MIN. That numerator then poisons everything derived from it. OsiCuts::scaleCutIntegral takes labs() of it -- still INT64_MIN -- and feeds it to computeGcd, which evaluates INT64_MIN % -1: a signed-division overflow, i.e. SIGFPE. A CglGMI cut carrying an rhs of 3.6e300 crashed there. Report "no nearby rational" instead. The constructor maps a false return to 0/1, and callers already treat numerator 0 as "cannot scale". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A parameter marked with setAutoAllowed() accepts the keyword "auto" on input, stored as a sentinel (INT_MIN for int, -inf for double) that bypasses the range check. isAuto() reports it and valueString() renders it as "auto" in messages, help and operator<<. The owning application resolves the value from the instance; intVal()/dblVal() still return the raw sentinel, so callers must test isAuto() first. Parameters that do not opt in are unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lookupParam() declares matchCnt, shortCnt and queryCnt with default 0, and every later write checks for NULL, but the three initial zeroing writes did not. Any call that relied on the defaults crashed with a segfault. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CglBKClique, CglOddWheel and CglImpliedClique decide when their CoinCutPool filters (fewest columns, fewest candidates, parallelism limit, always-filter override). They read those from CBC_CLIQUE_POOL_* env vars; this struct carries them instead, so the generators can take them as settings. Defaults are the env-unset values (500, 20, 1.0, off). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v17 Visual Studio projects build with /sdl, which turns MSVC's deprecation warning C4996 for getenv() into an error. Since 44e7a02, CoinUtils next (and everything that builds it with those projects) no longer compiles there. Check CBC_CLIQUE_POOL_DEBUG through a helper that uses _dupenv_s() under MSVC and getenv() elsewhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
next: CoinCutPool: don't call getenv() under MSVC
Adds isDefault(), defaultMode(), and defaultKwd() to CoinParam across all parameter types (keyword, integer, double, string). This allows solvers to distinguish whether a parameter is currently at its default setting or was explicitly modified by the user. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rSet) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
windows-ci.yml is the fixed version proposed for master and the stable branches: requirements, gcc, gfortran, and the MSYS pkgconf are installed into the UCRT64 environment created by setup-msys2 (instead of C:\msys64, which the build never used), and the upload of the build directory of failed builds is fixed. Also add next to the branches for which pushes trigger linux-ci, windows-ci, and windows-msvs-ci, so that commits pushed directly to next are tested too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The operator is declared as a friend of BitVector128 and again at namespace scope, but without COINUTILSLIB_EXPORT, so it is not exported from libCoinUtils when it is built with -fvisibility=hidden (or as a DLL). Bcp uses it (BCP_tm_msg_node_rec.cpp), so linking Bcp then fails with "undefined reference to operator<(BitVector128 const&, BitVector128 const&)". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The first declaration of operator< was the friend declaration in BitVector128, without COINUTILSLIB_EXPORT, and MSVC doesn't accept adding __declspec(dllexport) in a later declaration (error C2375, "redefinition; different linkage"). So declare it, with COINUTILSLIB_EXPORT, before the class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CoinBuildInfo (b8e3bb8) is in the autotools build but not in the Visual Studio project, and Cbc next uses it (CbcParamUtils.cpp), so linking Cbc with the Visual Studio solution fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # .github/workflows/linux-ci.yml # .github/workflows/windows-ci.yml # .github/workflows/windows-msvs-ci.yml
This was referenced Oct 23, 2026
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.
Merges
nextintomaster, as proposed in coin-or/Cbc#821. One change from the proposal: this branch was made frommasterand mergesnext, rather than the other way around, somaster's own history stays the first parent of the merge.This is one of five PRs that have to land together, in dependency order:
next's code in each project needsnext's code in the projects it depends on.To land: fast-forward
masterto this branch (git push origin merge-next:master), or use Create a merge commit. Don't use Squash and merge or Rebase and merge: squashing would turnnext's 35 commits into one, and rebasing would drop the merge.Commits
.coin-orneeds changing.Conflicts
.github/workflows/linux-ci.yml,windows-ci.yml,windows-msvs-ci.yml:masteraddedpaths-ignorefor the README files to the push trigger;nextaddednextto its branches. I keptmaster'spaths-ignoreand dropped thenexttrigger, sincenextgoes away. The workflows are now identical tomaster's.Testing
On Linux with gcc 11, I built the five
merge-nextbranches together, in dependency order, and ran each project'smake test. All five pass.🤖 Generated with Claude Code