Complete the paper benchmark migration to LyoGDP-Benchmarks - #151
Complete the paper benchmark migration to LyoGDP-Benchmarks#151bernalde wants to merge 1 commit into
Conversation
The paper OCP/GDP validation surface — models, examples, tests, notebooks, and the pseudosteady-limit IPOPT/POUNCE baselines — now lives in SECQUOIA/LyoGDP-Benchmarks. Remove the duplicated copies here, prune the Pyomo workflow's path filter and solver-lane node list in lockstep with the packaging contract guard, and point every docs surface at the new home. No released tag ships the removed modules and nothing outside the migrated set imports them, so they are removed outright rather than shimmed. The POUNCE lane stays: it reruns the remaining pyomo-marked suite through the ipopt shim, so its non-paper coverage is unchanged. Closes #150
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bernalde
left a comment
There was a problem hiding this comment.
Full initial review at 79436be (25 files: 16 deletions, 9 edits). The local changed-file set matches GitHub's file list exactly, and the branch sits at the current main tip (5e0b744), so head checks and merge-result checks coincide.
Linked issue
#150's remaining scope — per the issue owner's re-scoping comment — is exactly this: remove the duplicated LyoPRONTO-side copies, update docs and CI, and direct users to LyoGDP-Benchmarks. Closes #150 is correct for that scope. The destination-lag items (the #148 POUNCE baseline and the never-ported files) are disclosed in the body with the canonical source ref (5e0b744) rather than silently dropped; they are a LyoGDP-Benchmarks task, not a gap in this PR.
What I verified beyond the diff
- No surviving consumer: a ref-pinned grep at the base tip for
paper_ocp/paper_gdp/pseudosteady_limit_study/classify_convergence_qualityimports matches only files this PR deletes (the grep's own hits on in-set files are the positive control). The package__init__never exported the removed modules, and no release tag (v1.0–v1.1.2) ships them, so removal owes no shim — the issue's open design choice, settled with evidence. - Removed-test coverage moves with its artifacts: every deleted test guards a migrated model, example, baseline, or notebook; none guarded a surviving surface.
- CI contract stays two-sided: the packaging guard was mutation-probed — re-adding a migrated path to the workflow made
test_ci_workflows_use_documented_test_lane_expressionsfail, which is the intended outcome; restored, all 16 packaging tests pass. - The POUNCE lane still runs for real: on this head's post-ready wave the lane reports
RUN_POUNCE: trueand111 passed, 363 deselected in 65.79s(log-verified, not badge-verified). The drop from 183 is exactly the migrated tests, and the strict xfail is gone because the test that carried it migrated with its model. - Docs:
mkdocs buildis clean with only warnings pre-existing atmain; no removed page is built and the site contains no dangling link to the migrated notebooks. All five prose surfaces point at the same destination and issue. - Local lanes at this head: fast 314 passed; Pyomo construction 101 passed; notebook collection 4; ruff and actionlint clean. Post-ready CI: all four workflows completed successfully; the earlier draft-wave "green" heavy lanes were draft skips and are superseded by this wave.
Summary
Blocking: 0. Nonblocking: 0. Questions: 0. Merge state CLEAN, base current, no approval gate reported by GitHub. Posted as a COMMENT because this account authored the PR. Merge-ready as-is; the one follow-up that outlives this PR is the LyoGDP-Benchmarks sync from 5e0b744, which the body records.
bernalde
left a comment
There was a problem hiding this comment.
This review supersedes my prior review of the same head (79436be). A fresh independent pass found a blocking migration gap.
Scope and linked issue
#150's LyoPRONTO-side cleanup is coherent, and no surviving source consumer depends on the removed modules. However, the destination still lacks active copies of part of the surface deleted here. The inline finding covers the resulting acceptance-criteria and documentation mismatch.
Verification
- The local and GitHub changed-file sets match exactly.
mainremains at5e0b744, which is the merge base, so the PR-head and merge-result trees coincide. - Local exact-head checks passed:
tests/test_packaging_config.py(16), fast non-Pyomo lane (314), Pyomo construction/solver-availability cohort (101), notebook collection (4), Ruff, actionlint, andmkdocs build. The generated site has no links to either removed notebook. - Exact-head Actions are successful. The post-ready POUNCE job actually ran (
RUN_POUNCE=true; 111 passed, 363 deselected), the IPOPT solver job passed 40 tests, and the CI construction job passed 72 with 29 solver-dependent skips. The later successful notebook run supersedes the historical draft skip. - No other open LyoPRONTO PR overlaps this change.
Summary
Blocking: 1 inline. Nonblocking: 0. Questions: 0.
I would not merge this until the blocking issue above is addressed.
| where it stops is the measurement, and it is a property of the solver version as | ||
| much as of the model. | ||
| The paper-reference pseudosteady-limit continuation study, its recorded | ||
| IPOPT/POUNCE baselines (`pseudosteady_limit/`), and their regeneration and |
There was a problem hiding this comment.
Blocking: This migration statement is not true of the destination's current main. LyoGDP-Benchmarks contains only benchmarks/results/pseudosteady_limit/ipopt.json; it has no pounce.json, no equivalent of test_pounce_solver_comparison.py, and no equivalent of test_pseudosteady_limit_study.py or POUNCE dependency/CI path, while this PR deletes those artifacts here. That leaves #148's release-boundary baseline and regression coverage only in history and does not satisfy #150's equivalent-tests/docs migration criterion. Port the missing baseline and both regression suites, and wire them into the destination's docs/CI, before deleting them here. If that sync must remain separate, retain these artifacts and use Refs #150 plus accurate singular-baseline wording until a tracked destination change lands.
Summary
The paper OCP/GDP control-policy benchmark surface migrated to SECQUOIA/LyoGDP-Benchmarks; this PR finishes the LyoPRONTO side of #150: remove the duplicated copies, prune CI in lockstep with its contract guard, and direct users to the new home.
Closes #150.
What is removed (16 files)
lyopronto/pyomo_models/paper_ocp.py,paper_gdp.py(no package__init__export existed for either)paper_optimal_control_replication.py,paper_gdp_validation.py,pseudosteady_limit_study.pytest_pyomo_models/test_paper_ocp.py,test_pyomo_models/test_paper_gdp.py,test_paper_optimal_control_replication.py,test_paper_gdp_validation.py,test_pseudosteady_limit_study.py,test_pseudosteady_frozen_layer.py,test_pounce_solver_comparison.pydocs/examples/paper_optimal_control_replication.ipynb,docs/examples/pseudosteady_frozen_layer.ipynbbenchmarks/results/pseudosteady_limit/{ipopt,pounce}.json(plus their.gitignoreun-ignore rules)What is updated
.github/workflows/pyomo-tests.yml: migrated paths pruned from the Pyomo-scope filter and the solver-lane node list.tests/test_packaging_config.py: contract assertions pruned in lockstep, plus an anti-resurrection guard asserting the migrated surface stays out of the workflow. Mutation-proven: I re-added a migrated path to the workflow and the guard failed, which is the intended outcome; restored, it passes.docs/reference.md,docs/how-to-guides.md,docs/dev.md,examples/README.md,benchmarks/README.md,mkdocs.ymlnav.Design decision: removal, not deprecation shims
The issue left the transition strategy to the implementer. Evidence for outright removal: no released tag ships the modules (
git ls-treeof every tag, v1.0 through v1.1.2, finds zero paper files),lyopronto.pyomo_models.__init__never exported them, and a ref-pinned grep at the pre-removal tip finds no consumer outside the migrated set itself — so no compatibility is owed, and a shim would import from a repository that is not a pip dependency.What deliberately stays
pounceextra: the lane runs the remainingpyomo-marked suite through the ipopt shim (run_local_ci.sh pounce, marker-based), so its non-paper coverage — parameter estimation, design space, multi-vial, DAE optimizers, solver-backed notebooks — is unchanged.run_local_ci.shitself needed no edit.test_current_main_joint_optimizer_comparison.py::test_joint_pyomo_rate_limited_paper_extension_solvesand the current-main comparison family: production optimizers exercised on paper-inspired cases, no import of the migrated modules.benchmarks/README.md(only the pseudosteady sections migrated).Cross-repo coordination: destination lag
LyoGDP-Benchmarks (last pushed 2026-08-26) already carries the #147 convergence-quality work — verified: its
paper_ocp.pycontainsclassify_convergence_qualityand itsipopt.jsonrecordsconvergence_quality— but predates #148 (merged 2026-08-31). Not yet in the destination and removed here:benchmarks/results/pseudosteady_limit/pounce.json(the POUNCE 0.10.0 baseline)tests/test_pounce_solver_comparison.py(pins the IPOPT/POUNCE baseline agreement and the release boundary)tests/test_pseudosteady_limit_study.pyexamples/paper_optimal_control_replication.py+tests/test_paper_optimal_control_replication.py+ its notebookThe canonical source for syncing these is this repo at
5e0b744(the pre-removalmaintip). That sync is a LyoGDP-Benchmarks task and is not performed by this PR.Validation
Interpreter:
/home/bernalde/.local/bin/python(Python 3.13, Pyomo 6.9.5, IPOPT 3.14.16).actionlint .github/workflows/pyomo-tests.ymlruff check lyopronto tests examples main.pypytest tests/test_packaging_config.py -n 0-m "not slow and not notebook and not pyomo"pytest tests/test_pyomo_models tests/test_pyomo_solver.py -n auto-m notebook --collect-only--collect-onlymkdocs buildpaper_ocp/paper_gdp/pseudosteady/Srisumafunctions.pypseudosteady-physics docstring remainBranch Hygiene
Base
main, branched fromorigin/mainat5e0b744. Not stacked; no prerequisite PRs. No other open PR exists in this repository at branch time.