Consistent rho > 0 enforcement; report (not silently inflate) zero rhos (#560)#767
Open
DLWoodruff wants to merge 3 commits into
Open
Consistent rho > 0 enforcement; report (not silently inflate) zero rhos (#560)#767DLWoodruff wants to merge 3 commits into
DLWoodruff wants to merge 3 commits into
Conversation
…te) zero rhos (Pyomo#560) Progressive Hedging requires a strictly positive rho for every non-anticipative variable: rho multiplies the proximal/consensus term (rho/2)*(x - xbar)**2, so a non-positive value silently disables (or, if negative, inverts) enforcement of non-anticipativity. There was no central check, and several rho-setting heuristics silently substituted a small value when they computed zero. Enforcement (hard error on rho <= 0): - Validate inputs: defaultPHrho (attach_Ws_and_prox; None allowed when a rho_setter supplies every nonant), --default-rho (decomp._get_rho_setter), and each rho_setter output (_use_rho_setter). - Central guard check_rhos_positive() after Iter0 rho setup and before every solve loop, so any rho-updating extension that drives rho <= 0 is caught before it reaches the solver. Stop silently inflating: coeff_rho, sep_rho, grad_rho, and sensi_rho/reduced_costs_rho now report (rank-0, aggregated, only when the count changes) that they fell back to the positive default for zero-cost / zero-gradient / floored nonants, instead of doing so silently. Zero-cost variables keep a positive default rho (they still need NAC enforcement); they do not error. New helpers in utils/rho_utils.py: check_rhos_positive() and report_zero_rho_fallback() (the no-spam reporter). Tests: new test_rho_enforcement.py (unit + solver-gated farmer integration), wired into run_coverage.bash and test_pr_and_main.yml. Updated the test_grad_rho_bundles.py stub for the expanded option surface _compute_and_update_rho now reads. 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 #767 +/- ##
==========================================
+ Coverage 74.13% 74.15% +0.01%
==========================================
Files 166 166
Lines 21181 21232 +51
==========================================
+ Hits 15703 15745 +42
- Misses 5478 5487 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ive (Pyomo#560) Follow-up to the rho>0 enforcement: replace the "floor small computed rhos up to the default" behavior (most visible in sensi_rho, which floored everything below defaultPHrho) with a shared rule -- a heuristic-computed rho above a numerical-zero tolerance is kept as-is (small positive values are respected), and only a near-zero or negative value falls back to the positive default rho (and is reported). - utils/rho_utils.py: add RHO_ZERO_TOL (1e-8, a numerical-zero tolerance, not a rho floor) and resolve_rho(val, default) -> (rho, used_default). - coeff_rho, sep_rho, grad_rho, sensi_rho/reduced_costs_rho: apply resolve_rho uniformly. For sensi/reduced_costs the fallback is applied to the final post-(cross-scenario)-max value, so a single near-zero scenario no longer inflates the max with the default. grad's negative results (from sign cancellation when grad_order_stat > 0.5) now fall back gracefully instead of erroring at the guard. _SensiRhoBase._minimum_rho renamed to _fallback_rho to match the new semantics. Tests: add Test_resolve_rho and a sensi-rho "small value kept, ~zero defaults" test (drives the real cross-scenario max); update test_sep_rho's zero-cost case to the new "uses default rho" contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Disambiguate the issue reference from PR numbers in code comments and test docstrings. Comment-only change. 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
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.
Issue
Closes #560 — "Consistent/rigorous enforcement of rho > 0 checks" and "should not silently inflate rhos to some small value (e.g., 0.1) if 0 values are encountered."
Background
Progressive Hedging requires a strictly positive rho for every non-anticipative variable: rho multiplies the proximal/consensus term
(rho/2)*(x - xbar)**2, so a non-positive value silently disables (or, if negative, inverts) enforcement of non-anticipativity. There was no central check, and several rho-setting heuristics silently substituted a value when they computed (near-)zero.Note that a zero objective coefficient does not justify a zero rho: rho is on the consensus term, not the objective, so such a variable still needs
rho > 0to reach agreement across scenarios. Those variables fall back to the positive default — but we now say so out loud rather than silently.Changes
Enforcement — hard error on
rho <= 0:defaultPHrho(attach_Ws_and_prox;Noneis allowed when arho_settersupplies every nonant — caught later if not),--default-rho(decomp._get_rho_setter), and eachrho_setteroutput (_use_rho_setter).check_rhos_positive()runs after Iter0 rho setup and before every solve loop (once per PH iteration), so any rho-updating extension that drives rho<= 0is caught before it reaches the solver.Stop silently inflating / respect small computed rhos: a shared rule in
utils/rho_utils.py—RHO_ZERO_TOL = 1e-8(a numerical-zero tolerance, not a rho floor) andresolve_rho(val, default):sensi_rho, which floored everything belowdefaultPHrho);Applied uniformly to
coeff_rho,sep_rho,grad_rho, andsensi_rho. For the sensi family the fallback is applied to the final value after the cross-scenario max, so a single near-zero scenario no longer inflates the max with the default.grad_rho's negative results (sign cancellation whengrad_order_stat > 0.5) now fall back gracefully instead of reaching the guard.Tests
New
mpisppy/tests/test_rho_enforcement.py— the guard, theresolve_rhorule (incl. "small positive kept, ≈0/negative defaulted"), a sensi-rho test driving the real cross-scenario max, the no-spam reporter, and--default-rhovalidation; plus solver-gated farmer integration tests. Wired intorun_coverage.bashand.github/workflows/test_pr_and_main.yml.test_sep_rho.py's zero-cost case now asserts the "uses default rho" contract; thetest_grad_rho_bundles.pystand-in PH was extended for the option surface_compute_and_update_rhonow reads.All rho-related tests pass; ruff clean; no regressions in
test_ef_ph, aircondgeneral_rho_setter, APH, farmer CI, orgeneric_cylinders.🤖 Generated with Claude Code