Enforce same bound types used in path-mixing transform

[Opt-Mucca]

This fixes #3171

This should not be merged until further testing, and should wait to be reviewed by @fwesselm

General idea: When stacking transform functions in in HighsPathSeparator, ensure that we only use the previous boundType.

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (df4f2cd) to head (f600d1b).
⚠️ Report is 188 commits behind head on latest.

Files with missing lines	Patch %	Lines
highs/mip/HighsTransformedLp.cpp	75.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #3179      +/-   ##
==========================================
+ Coverage   73.16%   73.19%   +0.02%     
==========================================
  Files         430      432       +2     
  Lines      103967   105521    +1554     
  Branches    16719    16993     +274     
==========================================
+ Hits        76067    77232    +1165     
- Misses      27624    28013     +389     
  Partials      276      276              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EamonHetherton]

you asked for an AI perspective over on #3176; here is my AI guided review.

I have built and tested this branch, since the PR asks for further testing before merge. Summary
up front: it fixes both cases I have, the measured adverse effects are close to nil, and I think
this approach is better than the one I proposed in #3176.

It fixes the defect

Built at 2edd0aac82 and compared against its own base, 04024d701f, so the only difference is
this change. Run with random_seed = 0, threads = 1, parallel = off:

model	base 04024d701f	this PR
3171-1.mps, the reproducer attached to #3171	42332.2356068	42215.5250005 (correct)
the 10404 x 18899 model from the report, presolve=on	-12370583.0955	-12373561.5756 (correct)

The second one is a production model, run with the pinned option set from the issue.

I also have three generated models that I had previously confirmed lose the optimum to this
specific defect. They came out of a search of about 1.25 million models: 21 trigger the substitution
flip at all, and only these 3 have the resulting invalid cut actually remove the optimum rather than
cut nothing off. The first of them is the model 3171-1.mps above was reduced from, so only two are
independent of the case already listed.

model	base 04024d701f	this PR
the one 3171-1.mps came from	42332.2356068	42215.5250005
second	43911.6794028	43534.4142255
third	63200.6892844	63031.1829455

All three are corrected, to exactly the values the change in #3176 produces, and the other 18 are
left untouched — which is what you would want, since the flip was benign in those.

Why I think this is the better fix

#3176 recorded the substitution used for each coefficient and restored it immediately before
untransform(), and then refused to build the cut when two base rows of a path disagreed about a
shared column. This prevents the disagreement from arising at all, so the coefficients are computed
against one substitution throughout and a valid cut is still produced where mine discarded it. That
is strictly more cut kept for the same correctness guarantee, and it puts the invariant in
HighsTransformedLp where it belongs rather than in the separator.

What the approach rests on

One thing that is worth stating explicitly, because it is what makes this safe and it is not
written down anywhere in the PR.

The enforcement branch ends in i++; continue;, which skips the switch below it. For a column
whose retained bound type is kVariableUb or kVariableLb that would be wrong: the variable bound
substitution happens inside that switch — vectorsum.add(bestVub[col].first, ...), the sign flip,
and the tmpRhs adjustment — so the coefficient for that row would never be substituted, while
untransform() would still back-substitute it. That is the same class of defect this PR fixes.

It is not reachable, because the condition guarding the branch

ub - lb <= 1.5 || boundDist[col] != 0.0 || simpleLbDist[col] == 0 || simpleUbDist[col] == 0

is a property of the column, not of the row being transformed, and is constant for the lifetime
of a HighsTransformedLp. A column that took the variable bound branch on one base row therefore
cannot enter the enforcement branch on a later one, so only columns using simple bounds ever have
their type held fixed.

That is a real invariant rather than an accident, but it is load bearing and invisible, so it may
be worth a comment at that branch — if the condition is ever changed to depend on the row, the
enforcement branch silently becomes unsound.

Adverse effects

Built this branch against its own base (04024d701f) and compared every MIP model in
check/instances, with determinism pinned as above:

	result
objective differences	0 of 26
node count differences	0 of 26
LP iteration differences	1 of 26: dcmulti, 9682 -> 9798 (+1.2%), same nodes, same objective
full unit test suite	1259125 assertions in 328 test cases, pass

On the TODO about relaxed-out columns

The open item in the description is not to enforce the same bound type when the column is going to
be relaxed out. The table above is the measurement of what that currently costs: on this set,
holding the bound type where the column would otherwise have been relaxed away changes no
objective and no node count, and moves LP iterations on a single model by about one percent.

That is not a MIPLIB run, so it does not settle the question, but it suggests the TODO may not need
to be resolved before merging.

The kUnused asserts

The three assert(false) guards never fired on a build with asserts enabled, across 581 models:
all of check/instances, the reproducer, and four corpora of generated MIPs with heavy aggregation
path activity. Nor on the 10404 x 18899 production model, which makes about 94000 path attempts in
a single solve. One assertion did trip, on nan0.mps:

HPresolve.cpp: void presolve::HPresolve::removeFixedCol(HighsInt, double):
Assertion `std::isfinite(model->offset_)' failed.

That is presolve code this PR does not touch, and it reproduces on a build without this patch, so
it is pre-existing and unrelated. Flagging it only so it is not attributed to this change.

One case I could not rule out: in the third loop of transform(), a column that is not integral
and was not assigned a bound type in the first loop would skip the second loop, which begins with
if (!lprelaxation.isColIntegral(col)) continue;, and reach case BoundType::kUnused with
assert(false); return false;. I could not construct it — every column of the row is typed in the
first loop, and the columns the vectorsum introduces are the binaries of variable bounds, so they
are integral — but since it is an assert(false) on a path that is silent in release builds, it
may be worth confirming rather than assuming.

Two small things

The branch is based on master (04024d701f) while the PR targets latest, so it may want a
rebase before merge.

If a regression test would be useful, 3171-1.mps from #3171 is 85 rows x 99 columns, 49 binaries,
300 nonzeros, and is solved incorrectly under default options — it reports Optimal at Gap 0%
with 42332.2356068 against an optimum of 42215.5250005, decided at the root in zero nodes. The
optimum is confirmed without trusting any patched build: fixing the 49 integer columns to the better
point and solving the remaining LP with unmodified HiGHS returns Optimal 42215.5250005. It is a
tamer model numerically than the two attached to #3170, so the concern raised on #3172 about
objective coefficient ranges does not apply to it. Happy for it to be used here, and happy to close
#3176 in favour of this.

[Opt-Mucca]

This is good to go, although it should probably wait for a review. I can't see it being anything other than performance neutral.