Skip to content

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

Open
bernalde wants to merge 2 commits into
pr/docs-examplesfrom
pr/paper-ocp-validation
Open

PR 8/8: Validate paper OCP direct transcription#32
bernalde wants to merge 2 commits into
pr/docs-examplesfrom
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.

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