Skip to content

PR 8/8: Validate paper OCP direct transcription#32

Merged
bernalde merged 5 commits into
pr/benchmarksfrom
pr/paper-ocp-validation
Jun 1, 2026
Merged

PR 8/8: Validate paper OCP direct transcription#32
bernalde merged 5 commits into
pr/benchmarksfrom
pr/paper-ocp-validation

Conversation

@bernalde
Copy link
Copy Markdown
Member

Summary

  • add an experimental paper-reference Pyomo OCP benchmark for Paper Problem 1
  • validate the upstream-resolution n_z=20 direct-transcription solve with orthogonal collocation and IPOPT
  • document the upstream MATLAB/GEKKO active-policy strategy and add Pixi-managed GEKKO for reference verification
  • expose the benchmark helpers through lyopronto.pyomo_models

Validation

  • python -m ruff check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py tests/test_pyomo_models/test_init.py
  • pytest tests/test_pyomo_models/test_paper_ocp.py tests/test_pyomo_models/test_init.py -q -n 0
  • /home/bernalde/.pixi/bin/pixi run python GEKKO smoke solve

Closes #26

@bernalde bernalde marked this pull request as ready for review May 10, 2026 23:30

model.Rp = pyo.Expression(model.t, rule=resistance_rule)

def sublimation_flux_rule(m, t):
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.

Nonblocking: Nw is an unconstrained expression, but sublimation_flux() says the Pyomo model enforces nonnegative flux. With the default lower temperature bound, Pw can fall below chamber_water_pressure (about 220.92 K for 3 Pa), so the NLP still admits negative sublimation/deposition states. Either add a constraint such as m.Pw[t] >= config.chamber_water_pressure or a nonnegative flux variable, or narrow the docstring to match the implemented smooth expression.

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.

Addressed in 6da96cf. Added a Pyomo nonnegative_sublimation_flux constraint, updated the helper docstring/scaling, and added test_problem1_model_constrains_sublimation_flux_nonnegative.

Comment thread docs/PAPER_OCP_VALIDATION.md Outdated

As a local verification against the cloned upstream repository, the MATLAB
Policy 1 segment for Problem 1 (`Case2`) was run from
`/home/bernalde/repos/simDAE-optimalcontrol-lyo`. It detected the
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.

Nonblocking: This absolute local path makes the validation note hard for another maintainer to reproduce. Please replace it with the upstream repository URL/commit plus the command or script used, or move the local-only detail to the tracking issue until the reproducible workflow lands.

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.

Addressed in 6da96cf. Replaced the local path with the upstream repository, commit 5bcfece23128be7e5be51b73693dc6674223ccc6, and the MATLAB script path used for the validation note.

Copy link
Copy Markdown
Member Author

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Reviewed the Paper OCP benchmark implementation, surrounding Pyomo conventions, docs, package exports, and tests. I found only nonblocking follow-ups; this is merge-ready from my review. GitHub does not allow this account to approve its own PR, so I am submitting this as COMMENT rather than APPROVE.

@bernalde
Copy link
Copy Markdown
Member Author

Addressed review comments in commit 6da96cf.

Commits pushed:

  • 6da96cf Address Paper OCP review comments

Main changes made:

  • Added nonnegative_sublimation_flux to the Paper OCP model so the NLP enforces Pw >= chamber_water_pressure and keeps Nw nonnegative.
  • Updated the sublimation_flux() docstring and optional IPOPT scaling for the new constraint.
  • Added regression coverage for the new nonnegative-flux constraint.
  • Replaced the local upstream validation path in the docs with the upstream repository, commit, and MATLAB script path.

Tests run and results:

  • python -m ruff check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py lyopronto/pyomo_models/__init__.py: passed.
  • PYTHONPATH=/tmp/LyoPRONTO_pr32 pytest tests/test_pyomo_models/test_init.py tests/test_pyomo_models/test_paper_ocp.py -n 0: 17 passed.
  • PYTHONPATH=/tmp/LyoPRONTO_pr32 pytest tests/ -n auto -v -m "not notebook and not pyomo" --ignore=tests/test_pyomo_models --maxfail=5: 261 passed.

Comments intentionally not addressed:

  • The top-level review summary was informational only; no code or docs change needed.

Remaining risks or follow-up items:

  • No known blocking risk. The new constraint narrows the feasible space to physically nonnegative sublimation driving force; the existing validated solves still pass.

Copy link
Copy Markdown
Member Author

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Focused follow-up review of 6da96cf found no actionable issues. The nonnegative-flux constraint addresses the model concern, the added regression test covers the constraint shape, the Paper OCP slow solves still pass locally, and the validation doc now points to the upstream repository commit and script path. I cannot approve this PR from the author account, so this is submitted as COMMENT.

@bernalde bernalde force-pushed the pr/docs-examples branch from 6d19246 to 9f33d10 Compare May 30, 2026 22:11
@bernalde bernalde force-pushed the pr/paper-ocp-validation branch from 6da96cf to baa929b Compare May 30, 2026 22:11
@bernalde bernalde changed the base branch from pr/docs-examples to pr/benchmarks June 1, 2026 03:54
Copy link
Copy Markdown
Member Author

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Blocking issues: None found.

Nonblocking issues: None found.

Questions: None.

Tests run and outcomes:

  • python -m ruff check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py tests/test_pyomo_models/test_init.py: passed.
  • pytest tests/test_pyomo_models/test_paper_ocp.py tests/test_pyomo_models/test_init.py -q -n 0: passed, 17 tests.
  • Import/export smoke for lyopronto.pyomo_models and policy-initialization behavior: passed.
  • CI-style non-Pyomo suite, pytest tests/ -n auto -v -m "not notebook and not pyomo" --ignore=tests/test_pyomo_models --cov=lyopronto --cov-report=xml:coverage.xml: reached 99% with no failures printed, then I interrupted it after the final long optimizer worker exceeded the local review window. Treat this as incomplete local coverage, not a PR failure.

Merge-readiness: I did not find actionable code issues in this PR. Because this review is being submitted by the PR author account (bernalde), GitHub only permits COMMENT; the formal approval or change-request verdict must come from another maintainer. I would consider the PR merge-ready after CI or another maintainer completes the broad suite.

@bernalde bernalde merged commit 89e85c4 into pr/benchmarks Jun 1, 2026
2 checks passed
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.

1 participant