Skip to content

Add Paper Problem 2 OCP benchmark#34

Open
bernalde wants to merge 2 commits into
fix/issue-27-upstream-referencefrom
fix/issue-29-problem-2-ocp
Open

Add Paper Problem 2 OCP benchmark#34
bernalde wants to merge 2 commits into
fix/issue-27-upstream-referencefrom
fix/issue-29-problem-2-ocp

Conversation

@bernalde
Copy link
Copy Markdown
Member

Summary

  • Add Paper Problem 2 defaults, model construction, solve wrapper, and result extraction for T <= 240 K, dS/dt <= 2.8e-7 m/s, and 228 K <= Tb <= 260 K.
  • Add the interface-velocity path constraint, Policy 3 classification, and a policy-sequenced initializer for the expected Policy 3 -> Policy 1 -> Policy 2 trajectory.
  • Document first-pass Problem 2 tolerances, the coarse mesh validation scope, and known limitations.

Tests run

  • pytest tests/test_pyomo_models/test_paper_ocp.py -m "not slow" -q -> 16 passed in 4.39s.
  • pytest tests/test_pyomo_models/test_paper_ocp.py::test_problem2_coarse_solve_reaches_terminal_target_and_classifies_policy -q -> 1 passed in 4.45s.
  • python -m ruff check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py -> all checks passed.
  • python -m ruff format --check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py -> 2 files already formatted.
  • PYTHONPATH=. pytest tests/ -n auto -v -m "not notebook and not pyomo" --ignore=tests/test_pyomo_models -> 261 passed in 371.19s.

Notes

  • An initial broader-suite run without PYTHONPATH=. failed because local xdist workers could not import top-level benchmarks and namespace examples; direct Python imports from the repo root worked, and the same suite passed with PYTHONPATH=..
  • Did not run a docs build or mypy. The changed Pyomo module is excluded by the repo mypy configuration, and the validation doc change is prose-only.

Closes #29

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.

Review summary:

Blocking issues:

  • The new Problem 2 paper OCP helpers are not exposed through the package-level lyopronto.pyomo_models API, while the existing Problem 1 helpers are. This makes the new solver harder to discover and breaks the established import pattern.

Nonblocking issues:

  • _initialize_problem1_model() has a duplicate assignment.
  • lyopronto/pyomo_models/README.md should be updated to match the new Problem 2 support documented in docs/PAPER_OCP_VALIDATION.md.

Questions:

  • None.

Tests run and outcomes:

  • python - <<'PY' ... from lyopronto.pyomo_models import solve_paper_problem2 ... PY failed with ImportError, confirming the package-level API gap.
  • pytest tests/test_pyomo_models/test_init.py tests/test_pyomo_models/test_paper_ocp.py -m "not slow" -q: 21 passed.
  • pytest tests/test_pyomo_models/test_paper_ocp.py::test_problem2_coarse_solve_reaches_terminal_target_and_classifies_policy -q: 1 passed.
  • PYTHONPATH=. pytest tests/ -n auto -v -m "not notebook and not pyomo" --ignore=tests/test_pyomo_models: 261 passed.
  • python -m ruff check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py: passed.
  • python -m ruff format --check lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_paper_ocp.py: passed.

I would not merge this until the blocking issue above is addressed.

Note: I attempted to submit this as REQUEST_CHANGES, but GitHub rejected it because the authenticated account owns this PR (Review Can not request changes on your own pull request). Submitting as COMMENT instead with the blocking issue preserved.

)


def solve_paper_problem2(
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 adds solve_paper_problem2(), but the package-level API in lyopronto/pyomo_models/__init__.py still only imports and exposes the Problem 1 paper helpers. That makes from lyopronto.pyomo_models import solve_paper_problem2 fail while the equivalent Problem 1 import works, and tests/test_pyomo_models/test_init.py does not catch the missing export. Please add the Problem 2 helpers (create_paper_problem2_model, generate_problem2_policy_initialization, and solve_paper_problem2) to the conditional imports and __all__, and extend the init test to cover them.

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 1788e7f. lyopronto.pyomo_models now exports the Problem 2 helpers, and tests/test_pyomo_models/test_init.py verifies both __all__ membership and actual attributes.

Comment thread lyopronto/pyomo_models/paper_ocp.py Outdated
discretization = model._paper_discretization
derived = model._paper_derived
settings = model._paper_problem_settings
settings = model._paper_problem_settings
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 assignment duplicates the line immediately above it. It is harmless, but removing the extra line keeps this initializer easier to scan.

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 1788e7f. Removed the duplicate _paper_problem_settings assignment from _initialize_problem1_model().

MATLAB. GEKKO is available in the repo-local Pixi environment for upstream
policy-segment verification.

Problem 2 is now available as a second paper-reference OCP:
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: Since this doc now says Problem 2 is available, the package README should be kept in sync. lyopronto/pyomo_models/README.md still lists only the Problem 1 paper OCP functions and describes classify_paper_policies() as inferring Policy 1/Policy 2 regions. Please update that section to mention the Problem 2 model, initializer, solver, and Policy 3 support.

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 1788e7f. Updated lyopronto/pyomo_models/README.md to list the Problem 2 model, initializer, solver, and Policy 3 classification support.

@bernalde
Copy link
Copy Markdown
Member Author

Pushed commit:

  • 1788e7f Address Problem 2 review feedback

Main changes:

  • Exported create_paper_problem2_model, generate_problem2_policy_initialization, and solve_paper_problem2 from lyopronto.pyomo_models.
  • Extended tests/test_pyomo_models/test_init.py to verify both __all__ membership and actual package attributes for exported Pyomo APIs.
  • Removed the duplicate _paper_problem_settings assignment in _initialize_problem1_model().
  • Updated lyopronto/pyomo_models/README.md to document the Problem 2 helpers and Policy 3 support.

Tests run:

  • python - <<'PY' ... from lyopronto.pyomo_models import create_paper_problem2_model, generate_problem2_policy_initialization, solve_paper_problem2 ... PY: passed.
  • pytest tests/test_pyomo_models/test_init.py tests/test_pyomo_models/test_paper_ocp.py -m "not slow" -q: 21 passed.
  • pytest tests/test_pyomo_models/test_paper_ocp.py::test_problem2_coarse_solve_reaches_terminal_target_and_classifies_policy -q: 1 passed.
  • PYTHONPATH=. pytest tests/ -n auto -v -m "not notebook and not pyomo" --ignore=tests/test_pyomo_models: 261 passed.
  • python -m ruff check lyopronto/pyomo_models/__init__.py lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_init.py tests/test_pyomo_models/test_paper_ocp.py: passed.
  • python -m ruff format --check lyopronto/pyomo_models/__init__.py lyopronto/pyomo_models/paper_ocp.py tests/test_pyomo_models/test_init.py tests/test_pyomo_models/test_paper_ocp.py: passed.

Comments intentionally not addressed:

  • None.

Remaining risks or follow-up items:

  • No local follow-up identified from these review threads.

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