Skip to content

Complete the paper benchmark migration to LyoGDP-Benchmarks - #151

Open
bernalde wants to merge 1 commit into
mainfrom
feat/issue-150-paper-benchmark-migration
Open

Complete the paper benchmark migration to LyoGDP-Benchmarks#151
bernalde wants to merge 1 commit into
mainfrom
feat/issue-150-paper-benchmark-migration

Conversation

@bernalde

Copy link
Copy Markdown
Member

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)

  • Models: lyopronto/pyomo_models/paper_ocp.py, paper_gdp.py (no package __init__ export existed for either)
  • Examples: paper_optimal_control_replication.py, paper_gdp_validation.py, pseudosteady_limit_study.py
  • Tests: test_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.py
  • Notebooks: docs/examples/paper_optimal_control_replication.ipynb, docs/examples/pseudosteady_frozen_layer.ipynb
  • Baselines: benchmarks/results/pseudosteady_limit/{ipopt,pounce}.json (plus their .gitignore un-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 redirected to LyoGDP-Benchmarks: docs/reference.md, docs/how-to-guides.md, docs/dev.md, examples/README.md, benchmarks/README.md, mkdocs.yml nav.

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-tree of 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

  • The POUNCE solver lane and the pounce extra: the lane runs the remaining pyomo-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.sh itself needed no edit.
  • test_current_main_joint_optimizer_comparison.py::test_joint_pyomo_rate_limited_paper_extension_solves and the current-main comparison family: production optimizers exercised on paper-inspired cases, no import of the migrated modules.
  • The general benchmark-hygiene guidance in 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.py contains classify_convergence_quality and its ipopt.json records convergence_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.py
  • examples/paper_optimal_control_replication.py + tests/test_paper_optimal_control_replication.py + its notebook

The canonical source for syncing these is this repo at 5e0b744 (the pre-removal main tip). 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).

Check Result
actionlint .github/workflows/pyomo-tests.yml passed
ruff check lyopronto tests examples main.py passed
pytest tests/test_packaging_config.py -n 0 16 passed
fast lane -m "not slow and not notebook and not pyomo" 314 passed (was 362; the delta is the migrated fast tests)
pytest tests/test_pyomo_models tests/test_pyomo_solver.py -n auto 101 passed
notebook lane collection -m notebook --collect-only 4 collected (was 6; the two migrated notebook executions)
solver-lane node-list sample --collect-only 21 collected, no stale references
mkdocs build clean; no removed page built, no dangling links in the site
repo-wide sweep for paper_ocp/paper_gdp/pseudosteady/Srisuma only migration-pointer prose and the production functions.py pseudosteady-physics docstring remain

Branch Hygiene

Base main, branched from origin/main at 5e0b744. Not stacked; no prerequisite PRs. No other open PR exists in this repository at branch time.

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
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bernalde
bernalde marked this pull request as ready for review August 31, 2026 19:09

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_quality imports 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_expressions fail, 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: true and 111 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 build is clean with only warnings pre-existing at main; 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
bernalde requested a review from parkyr September 1, 2026 00:09

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. main remains at 5e0b744, 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, and mkdocs 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.

Comment thread benchmarks/README.md
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move paper OCP/GDP control-policy benchmarks to gdplib

1 participant