Maximization: verify/guarantee max across all features (or a clear error)#769
Open
DLWoodruff wants to merge 6 commits into
Open
Maximization: verify/guarantee max across all features (or a clear error)#769DLWoodruff wants to merge 6 commits into
DLWoodruff wants to merge 6 commits into
Conversation
…inders Add serial end-to-end maximization tests (mpisppy/tests/test_maximization.py) for EF, PH, subgradient, L-shaped, and FWPH. Each solves the farmer example in both senses and checks that the maximize answer is the negation of the minimize answer and that bounds bracket the optimum on the correct, sense-dependent side (outer bounds are lower for min but upper for max). Add test_lagrangian_max to test_with_cylinders.py for the hub->spoke outer-bound path under maximization. Fix the stale 'assumes minimization' note in mpisppy/opt/lshaped.py (it negates the objective internally and supports max). Wire the new test into run_coverage.bash and the regression job in test_pr_and_main.yml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…for max The confidence-interval gap correction was silently wrong for maximization due to three compounding bugs: (1) correcting_numeric read cfg key 'pyo_opt_sense' but pyomo_opt_sense() writes 'pyomo_opt_sense'; (2) the setter tested objs[0].is_minimizing (a method object, always truthy) instead of calling it; (3) no pipeline path populated the sense at all. The gap was thus always computed as if minimizing. Fixes: - correcting_numeric takes an explicit sense= argument; gap_estimators and multi_seqsampling pass it from ev.is_minimizing, and gap_estimators returns the sense. The cfg fallback now reads the correct key. - mmw_ci reports the optimality-gap magnitude, so the maximize bound matches the minimize magnitude instead of coming out negative. - Fixed the is_minimizing() call and rank-gated the wrong-sign warning prints. - Added a farmer_maximize option to the test farmer to drive the pipeline. Sequential sampling (SeqSampling / IndepScens_SeqSampling) now raises a clear error for maximization: its BM/BPL stopping criteria and sample-size rules assume a non-negative, shrinking gap. Supporting max there needs a vetted gap-magnitude formulation; until then it fails loudly rather than silently. Tests: correcting_numeric sense handling (explicit + cfg fallback), end-to-end maximize gap_estimators and MMW on farmer, and a seqsampling-maximize-raises test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agnostic guests hardcoded the objective sense as minimize even though their PH-augmentation and bound logic already branched on it, so a maximize model was handled silently wrong. Pyomo guest (verified): pyomo_guest.py and the loose farmer_pyomo_agnostic.py now read/propagate the real objective sense; a farmer_maximize option is plumbed through examples/farmer/agnostic/farmer.py. New test_agnostic_pyomo_PH_maximize confirms the maximize bound and objective are the negation of the minimize case. AMPL guest: the bare 'assume minimization' assert is replaced with a clear RuntimeError so a maximize AMPL model fails loudly and early (the guest splices the PH term into a 'minimize' objective, so it is minimize-only for now). The gurobipy and GAMS guests need analogous sense-sign fixes but cannot be verified locally (gurobipy PH is solver-fragile off-CI; GAMS needs gamspy); they are left for a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rror)
add_cvar previously raised NotImplementedError for maximization. It now
branches on the objective sense: minimization linearizes CVaR of the upper
(cost) tail as before; maximization uses the mirrored lower (reward) tail,
constraining delta_s >= eta - Reward_s and subtracting beta/(1-alpha)*delta_s
from the (maximized) objective. delta_s stays non-negative and eta stays a
root-stage nonant, so EF, PH, Lagrangian, and xhat all inherit it unchanged.
Verified by a closed-form maximize EF test (rewards {10,20,30,40}, alpha=0.6:
E=25, VaR=20, lower-tail CVaR=13.75, so max E[R]+CVaR=38.75). The old
maximize-raises test becomes a maximize-supported structural test, and a new
EFClosedFormMaxTests class mirrors the minimize closed-form checks.
Reconciled doc/designs/cvar_design.md (code sketch, design note, and phase
status) to the shipped lower-tail mirror.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…urobipy guests Adds doc/designs/maximization_support.md recording the per-feature maximization support matrix, the minimize-only/error list, and the tests added by the audit. Also makes the GAMS and gurobipy agnostic guests fail loudly on a maximization model (they previously could silently mis-sign it): the GAMS guest raises when its model solves 'maximizing', and the gurobipy guest raises on GRB.MAXIMIZE. This matches the AMPL guest's min-only guard, so the agnostic guests are now uniformly minimize-only with a clear error (the Pyomo guest, which reads the real sense, is the one exception). A construction-time gurobipy guard test is added; the GAMS guard is exercised by the CI agnostic job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #769 +/- ##
==========================================
+ Coverage 73.73% 74.09% +0.35%
==========================================
Files 165 165
Lines 21121 21140 +19
==========================================
+ Hits 15574 15664 +90
+ Misses 5547 5476 -71 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… guard Two CI failures from the maximization branch, both in areas not runnable in the dev env: 1. Confidence intervals: the SeqSampling min-only guard built a probe scenario via scenario_creator(name, **kw_creator(cfg)) to read the objective sense, which crashes for multistage models (aircond) that need sample-tree context -- breaking a *minimization* test. The probe is now wrapped in try/except: if a representative scenario can't be built from cfg alone, skip the check and let minimization (the common case) proceed. The 2-stage maximize guard (and its test) still fire. 2. Agnostic: the new GAMS min-only guard broke test_agnostic_cylinders_gams, whose model (farmer_average.gms) is a *maximization* model the GAMS guest was built to run by negating it to a minimize. That is an intentional, tested (smoke-test) path, so the guard is reverted. The GAMS test asserts no objective/bound value, so the sign-correctness of reported bounds under max remains unverified -- documented as such. The gurobipy guard stays (its farmer defaults to minimize; the guard only fires on explicit GRB.MAXIMIZE). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Audits and hardens maximization (
sense=pyo.maximize) support across mpi-sppy so that every feature either works correctly for a maximization problem or raises a clear error — never silently produces a wrong answer. ("Runs without error" is not the bar; correctness — e.g. bounds bracketing the optimum on the correct, sense-dependent side — is.)The single source of truth for sense is
SPBase.is_minimizing(mixed-sense scenarios already raise). The core turned out to be far more sense-aware than a "min-biased" reading suggests; the real risks were untested paths (now tested) and two clusters of genuinely-silent wrongness (now fixed or guarded).Five logical commits, preserved for phase-by-phase review:
Phase 1 — tests for the correct-but-untested core. End-to-end maximize tests for EF, PH, subgradient, L-shaped, FWPH (
mpisppy/tests/test_maximization.py) plus a cylinders Lagrangian-bound test, each checkingmax == -minand that bounds bracket the optimum on the correct (sense-dependent) side. Fixes a stale "assumes minimization" comment inopt/lshaped.py(it negates the objective internally and supports max).Phase 2 — confidence-interval silent-wrongness (bug fix). The MMW / sequential-sampling gap correction was computed as if minimizing for every problem, due to three compounding bugs: a config-key typo (
pyo_opt_senseread vspyomo_opt_sensewritten),is_minimizingused without calling it, and the sense never being populated by the pipeline.gap_estimators/mmw_cinow pass and use the real sense (gap sign + optimality-gap magnitude), the wrong-sign warnings are rank-gated, and sequential sampling raises a clear error on maximization (its BM/BPL stopping rules assume a non-negative, shrinking gap).Phase 3 — Pyomo agnostic guest. Reads the model's real objective sense instead of hardcoding minimize; the AMPL guest's bare assert becomes a clear min-only
RuntimeError.Phase 4 — CVaR maximization.
utils/cvar.pynow supports maximization via the mirrored lower-(reward-)tail Rockafellar–Uryasev formulation (δ_s ≥ η − Reward_s, subtractβ/(1-α)·δ_s), verified against a closed-form maximize EF.Phase 5 — doc + guest guards. Adds
doc/designs/maximization_support.md(per-feature support matrix). The GAMS and gurobipy agnostic guests now raise a clear min-only error (they previously could silently mis-sign a maximization model).Result
Notes for the reviewer
run_coverage.bashand theregressionjob oftest_pr_and_main.yml; cylinder/CI/CVaR/agnostic tests extend already-CI-wired files.test_agnostic_gurobipy_PHfailure (solver-fragile, already marked "Test Case is failing") is untouched by this work.🤖 Generated with Claude Code