distr example cleanup: typo, dead code, kwarg defaults, go.bash modernization#687
Merged
Merged
Conversation
…nization
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 Pyomo#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>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
=======================================
Coverage 71.02% 71.03%
=======================================
Files 154 154
Lines 19252 19252
=======================================
+ Hits 13674 13675 +1
+ Misses 5578 5577 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Summary
Four review items from a walkthrough of
examples/distr/distr.py, bundled into one PR. Independent of the phase-A work in PR #686.Typo + vague description. The
--scalableCLI option's help string said"decides whether a sclale model is used". Fix the typo and broaden the wording to explain what the flag actually does:"generate pseudo-random data parameterized by --mnpr instead of using the hardwired 2/3/4-region datasets". Apply the same description toexamples/stoch_distr/stoch_distr.pyfor consistency.Dead code in
scenario_denouement. Five lines after an unconditionalreturn(pprint/printcalls) — removed.Misleading kwarg defaults.
scenario_creatordeclaredinter_region_dict=None, cfg=None, data_params=None, all_nodes_dict=Noneand thenassert ... is not Noneon entry. Forinter_region_dictandcfgthis is misleading —kw_creatoralways passes them and the=Nonedefaults imply optionality. Make them required kwargs and let Python's missing-arg error report the problem naturally. Keep=Nonedefaults ondata_params/all_nodes_dict(genuinely scalable-only). Rewrite the (stale) docstring to match.go.bashmodernization (Move and generalize license re-try loop logic in PHBase #3 from the review list). Switch togeneric_cylinders --admm, matching the recommendation we put at the top ofadmmWrapper.rstin PR ADMM doc cross-refs, BFs flow-through, stage2ef hard error #685. The legacy custom driverdistr_admm_cylinders.pystays in the directory for reference;go.bashhas a comment pointing at the doc rationale.Test plan
bash go.bash(SOLVERNAME=gurobi_persistent locally) — converges to 2.1% gap (below the 5% target)python -m pytest mpisppy/tests/test_admm_bundler.py— 26/26 passpython -m pytest mpisppy/tests/test_admmWrapper.py— 6/7 pass; the 7th (test_values) is a pre-existing flaky subprocess-capture failure that also fails on clean main (same pattern astest_stoch_admmWrapper::test_values)ruff check examples/distr/distr.py examples/stoch_distr/stoch_distr.py— clean🤖 Generated with Claude Code