Fix knapsack rational coefficient scaling - #1754
Conversation
The scaling by lcm(den)/gcd(num) is done in floating point, so a coefficient whose exact value is integral can come out an ulp below it, e.g. 135.45 scaled by 100 yields 13544.999999999998. Both callers treat the returned row as integral, which is what makes the knapsack cover test sum_C a_j > beta equivalent to sum_C a_j >= beta + 1. Snap the integer-variable coefficients onto their integers after scaling and report failure when one is not within tolerance. Signed-off-by: yboucher <yboucher@nvidia.com>
The dynamic program truncated each item weight with floor() to fit an integer table while comparing the resulting minimum weight against the untruncated capacity. Every truncated item bought the DP up to one unit of extra capacity, and the separation capacity carries exactly one unit of margin (sum_j a_j minus beta + 1), so a single truncated item is enough for the DP to return a set that does not fit. The caller takes the complement of that set as a cover, and sum_C a_j > beta then fails to hold, yielding an invalid cover inequality. Carry the weights as f_t, as the lifting dynamic program alongside already does, and assert the returned set respects the capacity. Signed-off-by: yboucher <yboucher@nvidia.com>
is_minimal_cover only tests minimality, so nothing verified that the separated set satisfies sum_C a_j > beta, which is what makes sum_C x_j <= |C| - 1 valid. The final gate before the cut is added only tests violation by x*, so a set that was not a cover produced a cut with no valid inequality behind it. Assert the property where the set leaves the separator and where the minimalization loop shrinks it, and drop the cut instead of emitting it if the property does not hold. Signed-off-by: yboucher <yboucher@nvidia.com>
Adds two test-only entry points alongside the existing clique and odd-cycle ones: one for the rational scaling of a row, one that runs the knapsack separator over a single row and returns the cut it emits. The unit tests use a row whose two-decimal coefficients do not land on integers when scaled, assert the scaled row is integral, and check the emitted cut against every binary point of the row. The end-to-end test solves a five variable model whose LP relaxation is uniquely fractional on that row, with only knapsack cuts enabled; before the fix the separator fixed a variable that is zero in the optimum and the solve reported 4 instead of 3. Signed-off-by: yboucher <yboucher@nvidia.com>
The end-to-end regression exercises the separator through solve_mip, so the two test-only functions in cuts.cpp and the unit tests over them are not worth the surface they add to the cut interface. Signed-off-by: yboucher <yboucher@nvidia.com>
aliceb-nv
added
bug
| // dp(j, v) = minimum weight using first j items to get value v | ||
| dense_matrix_t<i_t, i_t> dp(n + 1, sum_value + 1, INT_INF); | ||
| // dp(j, v) = minimum weight using first j items to get value v. | ||
| // The weights are carried at full precision: rounding one down would let the DP return a set |
There was a problem hiding this comment.
I'm not sure I understand why we need to use floats here. Where is the "rounding down" occurring that is causing the issue? And why does floats fix it?
There was a problem hiding this comment.
Ah ok. Reading further, it sounds like we tried to convert an inequality with floating point coefficients into a knapsack constraint with integer coefficients, but when we do this we can make some small error if we round down. Then when we do dynamic programming we can think we are under the weight limit, when we are actually over.
But I'm not sure the fix should be use floating point in the DP table. That can make the table a lot bigger. Maybe we should reject inequalities with large errors when converting to integer coefficients in our classification of knapsack constraints, so we don't arrive at this case in the first place? Or if this occurs when the error is quite small, maybe we just need to do a check at the end to see if the selected weights violate the capacity (similar to what is done in the assert now)
There was a problem hiding this comment.
Or just use ceil(weights[j-1]) instead of floor(weights[j-1])
There was a problem hiding this comment.
Yeah I think the typical solution here is careful rounding.
| // The odd cycle over z1, z2, z3 makes z = (0.5, 0.5, 0.5), w = 0 the unique LP optimum, which | ||
| // puts y at 0.412078. Integrality moves the optimum to w = 1 with y = 0 and objective 3. | ||
| // | ||
| // The capacity coefficients are load bearing: scaling that row to integers multiplies by 100, |
There was a problem hiding this comment.
Nit: "load bearing" sounds like an AI generated phrase. Could we reword?
|
|
||
| rational_inequality.scale(scalar); | ||
|
|
||
| // The scaled product can land an ulp off the integer it represents. Callers rely on the |
There was a problem hiding this comment.
This code is a bit strange. First, the integral_tol used here, doesn't match our default integer_tol. It would probably be best to pass settings in and use settings.integer_tol. Unless, this should be a separate tolerance.
Second, I think this function is used by more than knapsack cuts. So it's a bit weird to reference knapsack cuts in this code. I think this code is just trying to generate an inequality with rational coefficients, whereas knapsack needs an inequality with integer coefficients. Maybe this code should be moved into a post-processing step, say in a function called integer_coefficients (that first calls rationalize_coefficients)
Finally, rounding introduces some error. That just multiplying by a scalar does not. So, we probably don't want to do this in all cases. Maybe we also want to track the total amount of error introduced (instead of just tracking the error on individual coefficients).
| // Take item j-1 if possible | ||
| if (v >= scaled_values[j - 1]) { | ||
| i_t candidate = | ||
| dp(j - 1, v - scaled_values[j - 1]) + static_cast<i_t>(std::floor(weights[j - 1])); |
There was a problem hiding this comment.
Seems like the floor is the big issue here. Because it causes us to underestimate the weight. Could the issue be solved by just using std::ceil(weights[j-1]) here? If weights[k] is an integer than ceil(weights[k]) == floor(weights[k]). But if it isn't exact, we will round up, overestimate how much weight we can carry, and thus still produce a valid solution.
3 participants
Knapsack cover cuts could be invalid: rational_coefficients scales rows in floating point so coefficients land an ulp low, and the separation DP floored weights, so it returned oversized sets whose complement is not a cover.
Description
Issue
Checklist