ADMM doc cross-refs, BFs flow-through, stage2ef hard error#685
Merged
Conversation
admmWrapper.rst and stoch_admmWrapper.rst both dove straight into the custom-driver pattern, even though the recommended workflow is now generic_cylinders with --admm / --stoch-admm. Add an .. important:: block at the top of each that names the corresponding generic_cylinders flag, links to :ref:`generic_admm`, and frames the rest of the page as documentation of the underlying wrapper for users writing their own driver or extending the wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tage2ef
setup_stoch_admm hardcoded BFs=None into Stoch_AdmmWrapper, so the
wrapper's "already-multistage" branch in create_node_names was dead
and cfg.branching_factors stayed at whatever the user passed. The
only downstream consumer of cfg.branching_factors on the ADMM path is
xhatshuffle's stage2ef option, which then either index-errored or
silently produced a wrong stage2 count. Users had to hand-encode the
augmented tree as `--branching-factors "<num_stoch_scens>
<num_admm_subproblems>"` to make stage2ef run at all.
Make the wrapper the source of truth for the augmented tree:
* Stoch_AdmmWrapper.create_node_names: unify the two branches into
one path that always sets self.BFs = original_BFs +
[num_admm_subproblems], with defensive list copying so the
caller's list isn't mutated. For 2-stage origin the "original
BFs" defaults to [num_stoch_scens], which matches the previous
explicit construction byte-for-byte (verified against
sputils.create_nodenames_from_branching_factors).
* setup_stoch_admm: pass cfg.get("branching_factors") into the
wrapper, then quick_assign cfg.branching_factors back from
admm.BFs. xhatshuffle/stage2ef now sees the wrapper's true tree
without user-side encoding.
Add examples/stoch_distr/stoch_admm_stage2ef.bash showing the result:
--stoch-admm + --xhatshuffle + --stage2-ef-solver-name with no
--branching-factors flag needed. Converges in 15 PH iterations to
0.8% gap on stoch_distr with N=3.
Semantics change documented prominently in doc/src/generic_admm.rst
under a new "Branching factors with --stoch-admm" subsection: anyone
who previously passed `--branching-factors "<N_stoch> <N_admm>"` as a
workaround must now pass only the original problem's BFs (or omit the
flag for 2-stage-origin problems); otherwise the wrapper's append
produces an incorrectly deep tree.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without stage2_ef_solver_name, xhatshuffle fixes nonants only along the picked scenario's tree path, leaving ADMM consensus variables in *other* stochastic outcomes unconstrained. The resulting inner bound violates the ADMM consensus constraints — which are a real part of the problem, not a relaxation — and has no valid interpretation. Silently producing such a number is a foot-gun that's easy to miss because PH itself converges and the run looks healthy. Add the check to _check_admm_compatibility, matching the existing pattern (every other check there is a hard error). The message tells the user exactly what to add and offers --xhatxbar as the alternative inner-bound spoke that doesn't have this issue (xhatxbar fixes nonants to the PH xbar, which IS the consensus value). Tests cover the four configurations: stoch_admm+xhatshuffle without stage2ef errors, the same with stage2ef passes, stoch_admm+xhatxbar without stage2ef passes, and deterministic --admm + xhatshuffle without stage2ef passes (deterministic ADMM is flat 2-stage so the picked scenario's path covers every consensus var). Doc note added next to the existing FWPH-incompat note in generic_admm.rst. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-admm
The two wrappers behave differently with respect to the user's
attach_root_node call in scenario_creator:
- AdmmWrapper (deterministic --admm) calls attach_root_node itself
during assign_variable_probs (utils/admmWrapper.py:148, comment
"this will overwrite the nonants already there"). Any user call
is redundant.
- Stoch_AdmmWrapper (--stoch-admm) reads the user's
_mpisppy_node_list and appends an ADMM-consensus stage to it
(utils/stoch_admmWrapper.py:235). The user must call
attach_root_node with the original problem's first-stage vars or
the wrapper assertion-fails.
The doc previously instructed all ADMM users to call attach_root_node
with an empty varlist, which is misleading for --admm and incomplete
for --stoch-admm (the right varlist is the original first-stage vars,
not empty). Update generic_admm.rst Step 3 to drop the call for
--admm and add a "Extending to Stochastic ADMM" note that spells out
the --stoch-admm requirement and points at stoch_distr.scenario_creator
as the canonical pattern.
Also remove the dead commented-out attach_root_node lines in
examples/distr/distr.py and replace with a one-line comment
explaining why no call is needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #685 +/- ##
==========================================
- Coverage 71.05% 71.02% -0.04%
==========================================
Files 154 154
Lines 19252 19252
==========================================
- Hits 13679 13673 -6
- Misses 5573 5579 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced May 13, 2026
DLWoodruff
added a commit
that referenced
this pull request
May 13, 2026
…nization (#687) Four review items from the distr.py walkthrough: - typo: "sclale" -> "scalable" in the --scalable CLI description. Also broaden the description to actually explain what the flag does ("generate pseudo-random data parameterized by --mnpr instead of using the hardwired 2/3/4-region datasets") and apply the same description to stoch_distr.py for consistency. - dead code: scenario_denouement had four lines of pprint/print after an unconditional `return`. Drop them. - misleading kwarg defaults: scenario_creator declared all four kwargs (inter_region_dict, cfg, data_params, all_nodes_dict) as `=None` and then assert-not-None on entry. For inter_region_dict and cfg this is misleading: kw_creator always passes them and the None defaults imply optionality. Make them required positional kwargs; let Python's missing-arg error report the problem naturally instead of an opaque assertion. Keep `=None` on data_params / all_nodes_dict since they really are scalable-only. Rewrite the (stale) docstring to match. - go.bash modernization: switch to `generic_cylinders --admm` matching the documented recommendation at the top of admmWrapper pages from PR #685. The legacy custom driver `distr_admm_cylinders.py` is still in the directory; a comment in go.bash points at the doc for the rationale. Verified `bash go.bash` (with SOLVERNAME=gurobi_persistent for local solver availability) — converges to 2.1% gap. test_admm_bundler still 26/26. test_admmWrapper has a pre-existing test_values subprocess-capture failure (returncode=1, same flaky pattern as test_stoch_admmWrapper::test_values; verified failing on clean main too). Co-authored-by: Claude Opus 4.7 (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
9ad8963e): top ofadmmWrapper.rstandstoch_admmWrapper.rstnow points users atgeneric_cylinders --admm/--stoch-admmas the recommended workflow, with the wrapper pages framed as documentation of the underlying class for users writing custom drivers.78d68272):setup_stoch_admmnow passescfg.branching_factorsintoStoch_AdmmWrapperand republishes the augmented BFs back to cfg, so xhatshuffle's stage2ef path sees the wrapper's true tree shape automatically. Unifies the previously-dead "multistage origin" branch increate_node_names. New exampleexamples/stoch_distr/stoch_admm_stage2ef.bash. Semantics change documented in a new "Branching factors with --stoch-admm" subsection ofgeneric_admm.rst: anyone previously hand-encoding--branching-factors "<num_stoch_scens> <num_admm_subproblems>"must now pass only the original problem's BFs.58955c0e):--stoch-admm+--xhatshufflewithout--stage2-ef-solver-namenow raises a clearRuntimeError(instead of silently producing an invalid inner bound that violates ADMM consensus in non-picked stochastic outcomes). 4 new tests intest_admm_bundler.pycover the four configurations.attach_root_nodecontract (24bf5802): the two ADMM wrappers behave differently —AdmmWrappercallsattach_root_nodeitself and overwrites any prior call;Stoch_AdmmWrapperreads the user's existing node list and appends to it.generic_admm.rstStep 3 now says to skip the call for--admm, and a new "Extending to Stochastic ADMM" note spells out the--stoch-admmrequirement withstoch_distr.scenario_creatoras the canonical pattern. Dead commented-out lines removed fromexamples/distr/distr.py.Test plan
python -m pytest mpisppy/tests/test_admm_bundler.py— 26/26 passpython -m pytest mpisppy/tests/test_stoch_admmWrapper.py— 10/10 non-deselected pass (pre-existingtest_valuesflaky under subprocess capture, unchanged here)examples/stoch_distr/stoch_admm_stage2ef.bash— converges in ~15 PH iters to 0.8% gap with stage2ef firingexamples/distr/viageneric_cylinders --admm(gurobi_persistent, 3 regions) — convergesruff check .— clean for touched filescd doc && make html— builds cleanly (no new warnings)Follow-up
Phased automation work tracked in design doc PR #684 (
admm-api-automation-design); phase A will supersede commit24bf5802's doc note when the newfirst_stage_cost/first_stage_varlisthooks land.🤖 Generated with Claude Code