Clear outer_bound when a solve produces none, instead of keeping a stale one

[DLWoodruff]

The bug

A solve that reports no outer bound left the subproblem's previous bound in
place. The comment defending this (spopt.py) said:

The stale bound is still a valid outer bound (any Lagrangian dual value bounds
the original problem)

That is true per scenario and irrelevant to the sum Ebound actually
forms. Ebound computes Σ_s p_s · outer_bound_s, and a Lagrangian bound is
valid as a sum only when every scenario uses weights from one generation
satisfying Σ_s p_s W_s = 0:

Σ_s p_s L_s(W'_s) ≤ Σ_s p_s [f_s(x*) + W'_s^T x*] = OPT + (Σ_s p_s W'_s)^T x̄*

Mixing one scenario's stale bound with the others' fresh ones leaves the trailing
term in place, so the result is not an outer bound at all.

Why nothing catches it

Ebound has a careful collective missing-bound check — any(... outer_bound is None) Allreduce'd with MAX — and it cannot help here. The fallback deliberately
leaves a number behind, so the check passes. It catches "never computed"; the
fallback converts "not computed this time" into "computed earlier", which
is precisely the state it cannot distinguish.

Worse, the hub latches the best outer bound it has seen (spcommunicator.py,
new > old when minimizing). So a single too-good value is permanent — no
later honest bound ever corrects it.

How reachable is it

Narrow, but real. no_outer_bound_results screens only infeasible,
infeasibleOrUnbounded, unbounded, and a missing results object, so a
subproblem has to go bad at iteration k having been fine earlier. Two routes:

1. lagrangian() calls receive_nonant_bounds() before every solve. Tightened
   nonant bounds from another spoke change the feasible region, so a
   subproblem genuinely can newly become infeasible. (New weights alone could
   not do this — they only change the objective.)
2. An unbounded subproblem has Lagrangian value -inf, so a retained finite
   number over-claims regardless of any weight mixing.

The first solve is safe either way: _set_initial_bounds starts everything at
None, so the exposure begins at the second solve.

The fix

Set outer_bound = None when a solve produced no bound — the state
_set_initial_bounds already documents as "not computed". Ebound propagates
the None and the bound spokes decline to send, exactly as they already do for a
first solve that produces no bound.

The same exposure existed off the outer_bound_only path: the
not_good_enough_results branch also left outer_bound untouched, just without
a comment claiming that was fine. Both branches are fixed.

Column generation, and why it is in this PR

Clearing the bound made a latent path in column generation reachable, and CI
found it immediately:

File "mpisppy/cgbase.py", line 567, in add_columns_to_mp_from_results
    sum_redcosts+=red_cost
TypeError: unsupported operand type(s) for +=: 'int' and 'NoneType'

build_columns_from_subproblem_solutions already leaves red_cost at None
when a subproblem produced no outer bound — it tests the value for finiteness for
exactly that reason, and its comment says so — but the consumer added it
unconditionally. Nothing had reached that path before because outer_bound was
never None there.

Skipping the missing term would be worse than the crash. The caller computes
rmp_obj_val + sum(reduced costs) and treats the result as an outer bound, so
dropping a term overstates it — the same unsoundness this PR is about, one
subsystem over. So add_columns_to_mp_from_results returns None when any
reduced cost is missing, and both consumers (cgbase.CG and opt/dualcg) leave
the incumbent best bound and the convergence metric untouched for that iteration
rather than computing from a partial sum. The columns are still added; they are
useful whether or not a bound came with them.

While guarding dualcg, found that update_dual_center compared
outer_bound_candidate against the stored center before testing it for None.
The old ordering only avoided a TypeError while the center was still unset, so
a None candidate would have raised there once a center existed. The None
check now comes first.

CI is where this surfaced because its cplex is the size-limited community build,
whose subproblem solves fail where a full license succeeds.

Callers

Checked all four Ebound callers. LagrangianOuterBound, the subgradient hub,
and PHBase's trivial_bound already guard against None.
PHBase.post_solve_bound returns the value directly and can now return None,
so its docstring says so — that is the one caller-visible behavior change.

Tests

test_outer_bound_only.py (already in run_coverage.bash and CI, so no wiring
changes) gains four tests; the failing solve is stubbed, so no solver is
required. All four fail on the unfixed code.

The CG tests are deterministic rather than solver-driven, because the CI trigger
does not reproduce locally: add_columns_to_mp_from_results only calls
self.add_column_for_scenario, so a recorder stands in for the CG object and the
None path is exercised directly — no solver, no MPI. Both new assertions fail
against the unfixed code with the TypeError above.

Also run locally: test_ef_ph, test_vss, test_cvar, test_cg_main,
test_dualcg_main, mpiexec -np 2 test_with_cylinders.py,
test_dualcg_with_cylinders.py.

Note test_cg_with_cylinders.py fails locally with
-111936.54 != -109499.52 within 2000 delta — identically on clean main,
same number and same delta. It is solver-dependent and pre-existing (it passes in
CI), and untouched here.

Provenance

Noticed while writing the design in #838, but independent of it and based on
main rather than stacked. Ipopt reports Lower_bound = -inf rather than
None, so it takes the other branch and never reaches this path.

One thing worth a maintainer's eye: the original comment says "which is what the
pre-outer_bound_only path did here", so this looks like behavior preserved
through a refactor rather than a considered decision about the sum. If it was in
fact deliberate — some consumer that needs a spoke to keep reporting rather than
go quiet — I would rather hear that than guess.

[codecov]

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.78%. Comparing base (29671ec) to head (ffffe12).

Files with missing lines	Patch %	Lines
mpisppy/opt/dualcg.py	86.66%	2 Missing ⚠️
mpisppy/cgbase.py	95.23%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #839   +/-   ##
=======================================
  Coverage   77.77%   77.78%           
=======================================
  Files         177      177           
  Lines       23764    23779   +15     
=======================================
+ Hits        18482    18496   +14     
- Misses       5282     5283    +1     

☔ 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.

[Copilot]

Pull request overview

Prevents stale scenario bounds from producing invalid aggregate bounds and safely propagates missing bounds through column generation.

Changes:

• Clears unavailable outer bounds instead of retaining stale values.
• Propagates missing reduced costs through CG and DualCG.
• Adds deterministic regression tests and updates return documentation.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
mpisppy/spopt.py	Clears stale bounds after unsuccessful solves.
mpisppy/phbase.py	Documents nullable post-solve bounds.
mpisppy/cgbase.py	Handles unavailable reduced-cost sums.
mpisppy/opt/dualcg.py	Guards bound and dual-center updates.
mpisppy/tests/test_outer_bound_only.py	Tests stale-bound clearing.
mpisppy/tests/test_cg_main.py	Tests missing reduced-cost handling.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.