Fix #3170: discard a redundant variable bound instead of substituting it as if tight

[EamonHetherton]

Fixes #3170.

The first commit adds the two reproducers as a failing regression test, on its own, so the defect
is visible without any fix applied. The second commit contains the fix.

The models

• check/instances/3170-1.mps — 39x30, 10 binaries. Solved incorrectly with presolve=off alone:
  reports -21262719.4302 against an optimum of -21446579.364727, a relative error of 0.86%,
  about 86x the default relative gap tolerance, so not a tolerance artefact.
• check/instances/3170-2.mps — 40x24, 8 binaries. Needs the tighter option set used in the test;
  reports -516096644.644 against an optimum of -516307714.782682.

Both optima were verified independently of the MIP search, by enumerating every binary assignment
and solving the resulting LP for each.

The defect

HighsTransformedLp::transform() substitutes a variable upper bound x <= a*z + b by the slack
y = a*z + b - x, and passes upper[j] = ub - lb to the cut generator as y's upper bound. That
is sound only if the variable bound is at least as tight as the simple bound, i.e.
maxValue() <= ub, so the code calls cleanupVub() to re-establish that when it does not hold.

cleanupVub() only tightens a variable bound that is not redundant. When
minValue() >= ub - feastol it reports redundant = true and deliberately leaves the bound
unchanged, for the caller to discard — which is what cleanupVarbounds() does. The caller here
ignored that flag and substituted the bound anyway, so the slack could range far above the
ub - lb it was declared to have, and the cut derived from it was not globally valid. Such a cut
can remove the optimum, after which the search closes and reports the incumbent as Optimal at a
zero gap.

cleanupVlb() has the identical contract and the identical caller bug.

The fix

Discard the variable bound when cleanup reports it redundant, and defensively whenever the
assumption still fails afterwards, falling back to the simple bound. Nothing is lost, since a
redundant variable bound is by definition no stronger than the simple bound.

      if (redundant ||
          bestVub[col].second.maxValue() > ub + mip.mipdata_->feastol) {
        bestVub[col].first = -1;
        ubDist[col] = simpleUbDist[col];
        boundDist[col] = std::min(lbDist[col], ubDist[col]);
      }

and symmetrically for the variable lower bound.

Two things to be aware of when reviewing

Only the variable upper bound path is exercised by the tests. The variable lower bound half is
fixed by symmetry, since cleanupVlb() has the same contract and the same caller bug, but neither
attached model reaches it.

The second disjunct is belt-and-braces. After a successful cleanupVub() the invariant holds,
so || maxValue() > ub + feastol should not fire; the redundant flag alone is what these models
hit. Happy to drop it if you would rather the change were minimal.

Adverse effects

The concern with this change is that it discards a variable bound that would otherwise have been
used for cut generation, which could weaken cuts. Measured on latest:

	result
35 MIP models in check/instances	identical status, objective, node count and LP iteration count — every one
400 generated MIPs	identical objectives, none differing
9 models attached to open correctness issues	identical
full unit test suite	1259332 assertions in 335 test cases, pass

So there is no collateral change: no weakened cuts, no extra nodes, no extra iterations anywhere
the defect is not present. The fix appears to be inert unless the defect actually fires. That is
not a proof that it never activates — those 400 models come from the same generator that produces
triggers for this defect at roughly 1 in 500, so most likely none of them hit it.

One thing that might look like a regression but is not: on the larger model this was first found
on, the node count rises with the fix (7 to 18). That is the search doing its job — without the fix
an invalid cut was closing the tree early.

Testing

• the new issue-3170 test fails on latest at the first commit and passes at the second;
• both attached models return the brute-force optimum under every option set tried;
• the numbers in the table above.

[jajhall]

Thanks for this. We don't accept AI-generated code, but I expect that @Opt-Mucca and @fwesselm will identify what needs to be changed and fix it themselves.

[EamonHetherton]

@jajhall ok, I have two other issues I'm working through for a minimal reproduction, one is already logged as an issue with more detail to come. Essentially I am verifying a larger model across hundreds of thousands of solves with different inputs and occasionally HiGHS was returning a noticeably sub-optimal solution and found three discrete issues causing them. Would you rather I just report the issue as best I can or open a pull request with a demonstrated fix (albeit with AI assistance) for you to cherry pick from?

[jajhall]

If you can generate a fix, then it it certainly helps. Our developers may well implement it as it is, but any adverse effects need to be considered

[EamonHetherton]

Thanks, I'll continue as is then; log the issue with as much detail as I can gather and open a PR with a failing test followed by a proposed fix. Cherry pick as you like for what goes upstream. (and i'll rebase to latest too)

[Opt-Mucca]

Nice fix! The changes I'd request:

• Remove the extra test. The instances have pretty large objective coefficient ranges, and I'd rather not introduce a test that may cause problems later down the road.
• Please remove the comments between the if (redundant and the tightening of the variable bounds. I don't think this needs any explanation.
• Please remove the additional || check for whether the max values make the variable bound redundant. This case will only trigger when epsilon < feastol. That is, make the if statement only have if (redundant)

Edit: I've tested locally and removing the additional || check still fixes the attached instance.

[EamonHetherton]

Thanks.

• Removed the regression test and both instances.
• Removed the comments in that block.
• The if is now if (redundant) only, for both the variable upper and lower bound.

Some AI commentary on the changes though that are worth hearing (I will create a new issue and PR probably tomorrow for the new bug mentioned in there around HPresolve::scaleMIP...bed time shortly)

On the dropped ||. Agreed it is unreachable in any sane configuration, but the reason is the other way round from the one given: the two conditions can only hold together when feastol < epsilon, not epsilon < feastol. After a successful cleanupVub() the bound satisfies maxValue() == ub, and otherwise maxValue() <= ub + epsilon, so maxValue() > ub + feastol needs feastol < epsilon. That is reachable in principle, since epsilon is small_matrix_value (default 1e-9) while mip_feasibility_tolerance can legally be set down to kMinimumMipTolerance = 1e-10. Not worth guarding against on the evidence I have, and the redundant flag is the actual defect either way — just flagging it so the reasoning on record is right.

On removing the test. Understood on the objective coefficient ranges, and I think the instinct is right: I have since traced a separate wrong-answer defect on this same class of model to HPresolve::scaleMIP scaling rows down, where the feasibility tolerance enforced on the presolved row corresponds to a violation of feastol / |scale| on the original row. On one model that turned a 1e-7 tolerance into 1e-5 of real violation, so solutions were discarded as having untransformed violations and the optimum was lost. Wide coefficient ranges genuinely do make these models tolerance-sensitive, so a test pinned to an exact objective on one is a fair flake risk.

The trade-off is that nothing now guards this fix against regression. If it would help, I am happy to supply a numerically tamer instance for the test suite, or leave it out entirely — your call. Both models remain attached to #3170 for anyone who wants to reproduce it.

[Opt-Mucca]

@EamonHetherton Thanks for the fast response and fix! I'm opening an equivalent PR in #3177 Sorry if this does not credit you as the author of the change. In general we try not to merge AI generated code (@jajhall would have the full take if this is a problem for you). I'll see if I can somehow get GitHub to still credit you.

A proper thank you for distilling the issue into something nice.

[EamonHetherton]

all good, don't need the credit, just the bugs out of my way :)

[jajhall]

@EamonHetherton Thanks for the fast response and fix! I'm opening an equivalent PR in #3177 Sorry if this does not credit you as the author of the change. In general we try not to merge AI generated code (@jajhall would have the full take if this is a problem for you). I'll see if I can somehow get GitHub to still credit you.

A proper thank you for distilling the issue into something nice.

Indeed, thanks from me. The issue with AI-generated code is that it may be valuable to say that we don't accept it, and if someone objects to their AI-generated PR being rejected, we don't want a precedent that they can point to.