Surface convergence quality in the pseudosteady limit study

[bernalde]

Summary

paper_ocp has recorded metadata["convergence_quality"] since #145, but nothing read it, and the report that most needed it said the opposite. examples/pseudosteady_limit_study.py decided each rung from the termination alone and printed the solver message only on failure, so all 11 converged rungs of the committed IPOPT baseline — every one of them Solved To Acceptable Level — printed as optimal/ok converged.

Each rung now carries and prints the convergence level, the baseline is regenerated on the same IPOPT line, and the cross-solver mapping question is settled by measurement rather than deferred.

Closes #146.

Acceptance criteria

• RungResult carries the convergence quality and the ladder report prints it per rung. The field is populated from the extracted metadata rather than re-matching solver text. Both per-rung surfaces print it: the format_results table (quality=…) and the line streamed while the ladder is still solving.
• benchmarks/results/pseudosteady_limit/ipopt.json regenerated consistently, with any solver-version change stated rather than silent. Regenerated on IPOPT 3.14.16 — opt.version() reports 3.14.16.0, matching the recorded solver_version, so there is no provenance change to state. Verified field by field against the previous artifact: the solver, nlp_scaling_method and discretization blocks are identical; both ladders stop at the same rung (7 and 6); every pre-existing per-rung value is unchanged. The only delta is the added field.
• tests/test_pseudosteady_limit_study.py asserts an acceptable-level rung is reported as such rather than only as converged. Plus a test that a tolerance-converged rung and an acceptable-level one — both optimal/ok — do not read alike, and that a rung with no solve claims no tolerance.
• A decision recorded on whether classify_convergence_quality needs a per-solver mapping. Settled by measurement; see below.

The cross-solver mapping: measured, not deferred

POUNCE was not installed when #145 was written, so what it puts in solver.message was unverified. Measured here on one NLP through the ipopt ASL interface, forcing each outcome with tol/acceptable_tol/acceptable_iter:

Solver	Outcome	solver.message	Label before	Label now
IPOPT 3.14.16	met tol	Ipopt 3.14.16: Optimal Solution Found	converged_to_tolerance	unchanged
IPOPT 3.14.16	acceptable_tol	Ipopt 3.14.16: Solved To Acceptable Level.	accepted_at_acceptable_tol	unchanged
POUNCE 0.9.0	met tol	POUNCE 0.9.0: SolveSucceeded	unknown	converged_to_tolerance
POUNCE 0.9.0	acceptable_tol	POUNCE 0.9.0: SolvedToAcceptableLevel	unknown	accepted_at_acceptable_tol

So unknown was not an acceptable answer: every POUNCE solve landed there, fully converged ones included, which would have left the field carrying no information in exactly the comparison it was added for.

Neither is a per-solver table the right shape. POUNCE is an IPOPT port and reuses IPOPT's ApplicationReturnStatus vocabulary — it writes the enum names where IPOPT writes prose. One vocabulary, two spellings. classify_convergence_quality now matches the status with punctuation and digits dropped, so both binaries classify through one path with no solver identity threaded in. A solver outside that vocabulary still reads unknown, which is the honest answer rather than a mapping guessed from unseen text.

This changes only the label; the success gate is untouched and still accepts both levels, for the reasons in paper_ocp._is_successful_termination.

Scope note: the tutorial notebook

Not named in the issue, but the same defect on the most reader-facing surface. docs/examples/pseudosteady_frozen_layer.ipynb teaches this study and reports the committed baseline through a termination column reading optimal on all 11 converged rungs, with its live rerun printing (converged) per rung. The table now shows convergence_quality instead — every converged rung reports optimal by construction, so that column said nothing while implying these solves reached tol — and section 2 explains why acceptable-level is expected here, before the column's first appearance.

This notebook does commit its rendered outputs, and mkdocs publishes them — the gitignored docs/examples/*_output.ipynb that the notebook lane writes is a validation artifact, not the published one. An earlier revision of this PR changed the cell sources without re-executing, so the published page kept showing the pre-#146 rendering under corrected sources; the notebook lane stayed green throughout because it never compares the two. The tracked notebook is now regenerated with a no-parameter papermill run (matching what its committed metadata.papermill records) and test_committed_outputs_render_the_convergence_level fails if the committed outputs and their sources ever disagree again.

The POUNCE baseline artifact remains out of scope and stays with #140, which is waiting on a tagged release containing jkitchin/pounce#519. What this PR settles is the mapping that baseline will need when it lands, which is what #146 asked for.

Validation

Interpreter: /home/bernalde/.local/bin/python (Python 3.13, Pyomo 6.9.5, IPOPT 3.14.16, POUNCE 0.9.0).

Check	Result
ruff check lyopronto tests examples main.py	passed
pytest tests/ -n auto -m "not slow and not notebook and not pyomo"	362 passed
pytest tests/ -n auto -m pyomo	179 passed
./run_local_ci.sh full	412 passed
pytest tests/ -n 0 -m notebook	6 passed, 587 deselected
mkdocs build	built, no new warnings
python -m mypy lyopronto	fails on a numpy stub in this environment, identically at origin/main — pre-existing and advisory per AGENTS.md

Mutation evidence

The new tests were mutation-proven, one probe per defect class, each against an unfiltered run of both changed test modules:

Mutation	Result
study drops the extracted quality	caught
format_results omits the quality column	caught (3 tests)
streamed progress line omits the quality column	caught (2 tests)
streamed progress line hardcodes the tighter level	caught (2 tests)
rung with no solve claims a tolerance it never met	caught
classifier matches IPOPT prose only (pre-fix)	caught (2 tests)
classifier checks the tighter level first	caught
SolveSucceeded mapped to the looser level	caught

The streamed-line probe survived on the first pass: that line lived in a closure inside main() and no test could reach it. format_rung_progress() was extracted beside format_results() and pinned, which is what the second and third rows above verify.

Branch Hygiene

Base main, branched from origin/main at c732848. Not stacked; no prerequisite PRs. No other open PR touches these files (there are none open).

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[bernalde]

Reviewed the full changed-file set at d6681ef (10 files, verified against the
paginated pull-files endpoint). The mechanism is right and the classifier change is
well-scoped; one finding blocks, because the PR ships five new statements about a
tracked artifact and pins none of them.

Blocking

The new convergence-level claims about the committed baseline are unpinned

Five artifacts now assert that every converged rung of
benchmarks/results/pseudosteady_limit/ipopt.json is accepted_at_acceptable_tol:

• examples/pseudosteady_limit_study.py module docstring;
• examples/pseudosteady_limit_study.py, RungResult docstring;
• benchmarks/README.md;
• docs/how-to-guides.md;
• docs/examples/pseudosteady_frozen_layer.ipynb, section 2.

All five are true at this head — I verified the regenerated artifact reads
accepted_at_acceptable_tol on all 11 converged rungs. Nothing enforces them.

That matters because regeneration is a routine, instructed operation, not a rare
one: benchmarks/README.md says to regenerate "when the models change, when the
solver or its version changes, or when adding a solver". A regeneration whose
rungs come back converged_to_tolerance would leave all five documents wrong
with nothing failing, which is the same defect class this PR exists to close —
a report describing the baseline as something it is not.

The repository already guards the sibling case and states the intent plainly.
tests/test_pseudosteady_frozen_layer.py::test_baseline_still_supports_the_documented_headline
pins the notebook's quoted -0.622% / -0.080% to the artifact, and the module
docstring says "regenerating the baseline without updating the notebook text must
fail loudly rather than ship stale teaching numbers". This PR adds claims of
exactly that kind and does not extend the guard.

Two concrete parts:

1. Extend tests/test_pseudosteady_frozen_layer.py with an assertion that every
   rung of the committed baseline with converged: true carries
   convergence_quality == "accepted_at_acceptable_tol", and every non-converged
   rung carries "unknown", naming the documents that go stale if it fires. That
   is a fast-lane test over a committed file — no solver runs.
2. The notebook's own guard cell ("Hold the live rerun to the claim this notebook
   teaches") asserts all(r.converged for r in rungs) but not the level, while
   section 3 now claims the live rungs carry the same label as the baseline table.
   Add the level to that assertion so the notebook holds its own new claim.

Nonblocking

One fixture hand-writes the quality it should classify

_rung was deliberately changed to derive convergence_quality from its own
message, with the comment "Classified rather than hard-coded so the fixture cannot
claim a quality its own message does not support". The one fixture that builds the
tighter level bypasses that: test_the_report_separates_the_two_convergence_levels
sets solver_message and convergence_quality independently, so a fixture whose
two fields disagree would still pass. They agree today, so nothing is wrong — but
build it through classify_convergence_quality like _rung does, so the
discipline holds on both sides of the comparison the test exists to make.

Question

Is folding the tutorial into this PR the scope you want?

The tutorial change is not in #146's acceptance criteria. The argument for it: it
is the same defect on the most reader-facing surface — the baseline table printed
termination: optimal on all 11 converged rungs, and swapping that column for the
recorded quality is strictly more informative, since termination_condition is
optimal for every converged rung by construction. The argument against: #146
names three artifacts and this is a fourth, and the tutorial merged only days ago
in #144. Splitting it costs a second PR and leaves the tutorial stating the thing
this PR is fixing everywhere else. My call was to fold it in; say if you would
rather it moved.

Checks I ran

Beyond reproducing the PR's own lanes, two checks the diff does not show:

Two-sided classifier sweep. The widened match could over-claim as easily as the
old one under-claimed, so I swept both implementations over all 20 IPOPT
ApplicationReturnStatus values in both spellings (39 messages). Exactly two
verdicts move, both intended:

Solve_Succeeded              unknown -> converged_to_tolerance
Solved_To_Acceptable_Level   unknown -> accepted_at_acceptable_tol

Every non-success status still reads unknown in both spellings, and every prose
message IPOPT actually writes classifies as before. No existing IPOPT-only result
changes label.

Baseline regeneration is a pure schema addition. Compared the regenerated
artifact field by field against its predecessor: solver, nlp_scaling_method and
discretization identical; both ladders stop at the same rung; every pre-existing
per-rung value unchanged. opt.version() reports 3.14.16.0, matching the recorded
solver_version, so #146's provenance concern does not arise.

Local suites at this head: ruff clean; 360 fast, 179 pyomo, 410 full non-Pyomo,
notebook lane 1 passed; mkdocs build clean. mypy fails on a numpy stub in this
environment identically at origin/main — pre-existing and advisory per AGENTS.md.

GitHub checks at this head: all 9 pass, including the Notebook lane, which
skips on drafts and only ran once the PR was marked ready. The four
branch-protection contexts (Static analysis, Fast SciPy lane, Full non-Pyomo
validation, Pyomo import and construction lane) are green.

Linked issue

Closes #146 is the right keyword: all four acceptance criteria are met at this
head, with criterion 4 answered by measurement rather than deferred. The POUNCE
baseline artifact correctly stays with #140.

Summary

• Blocking: 1
• Nonblocking: 1
• Question: 1

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

[bernalde]

Blocking — this sentence, and the four others like it in this PR, state a property of a tracked artifact that nothing enforces. This file's own regeneration instructions a few lines below make that a recurring risk rather than a one-off fact. See gh-review-pr:finding=unpinned-baseline-quality-claim in the review body for the two-part fix.

[bernalde]

Fixed in 384a6f2. tests/test_pseudosteady_frozen_layer.py::test_baseline_still_records_the_documented_convergence_level now requires every converged: true rung of this artifact to record accepted_at_acceptable_tol and every unsolved rung to record unknown, and its failure message names the four documents that go stale so a regeneration says what to update. Mutating one rung to converged_to_tolerance makes it fail, which is the outcome that shows it guards.

One correction: the review counted five artifacts by listing the study's module and RungResult docstrings separately; they are one file, so the guard names four paths. docs/reference.md was checked and does not make the claim.

[bernalde]

Nonblocking — _rung derives the quality from its message so a fixture "cannot claim a quality its own message does not support"; this one sets the two independently. Build it with paper_ocp.classify_convergence_quality(...) on the same message so both sides of the comparison follow the same rule.

[bernalde]

Fixed in 384a6f2 — the tighter fixture now classifies from its own message, the rule _rung already followed, so neither side of the comparison can claim a quality its message does not support.

[bernalde]

Addressed the review at 384a6f2.

Blocking: unpinned-baseline-quality-claim — fixed

Four documents state that every converged rung of the committed baseline is accepted_at_acceptable_tol, and nothing enforced it. benchmarks/README.md instructs regenerating that artifact whenever the models or the solver version change, so a regeneration reaching tol would have left all four quietly wrong — the same defect class #146 exists to close.

tests/test_pseudosteady_frozen_layer.py::test_baseline_still_records_the_documented_convergence_level now reads the committed artifact and requires every converged: true rung to record accepted_at_acceptable_tol and every unsolved rung to record unknown. Its failure message names the four documents that go stale, so whoever regenerates is told what to update rather than left to find it. The recorded rung count (13) is pinned too, so a truncated artifact cannot satisfy the loop vacuously. No solver runs — it is a read of a committed file in the fast lane.

The notebook's own guard cell now holds the live rungs to the level section 2 claims for them, not just to converging.

One correction from doing this: the review said five artifacts carry the claim, listing the study's module docstring and its RungResult docstring separately. They are two statements in one file, so the guard names four paths. docs/reference.md was checked and does not make the claim — it only lists the vocabulary — so it is deliberately not in the list.

Nonblocking: fixture-hardcodes-quality — fixed

test_the_report_separates_the_two_convergence_levels now classifies the tighter level from its own message, the rule _rung already followed.

Question: notebook-scope-call — needs your decision

Left as posed. The tutorial change stays folded in, and nothing about it is blocking; if you would rather it moved to its own PR, say so and I will split it. Note that splitting means main briefly carries a tutorial whose baseline table prints termination: optimal on all 11 acceptable-level rungs, which is what this PR fixes everywhere else.

Mutation evidence

Each new guard was probed one defect class at a time, restoring the tree after each. "Caught" means the mutation made the guard fail, which is the intended, reassuring outcome:

Mutation	Result
a converged rung regenerates at tol	caught
an unsolved rung claims a tolerance	caught
the artifact is truncated by one rung	caught
notebook guard demands a level the live rungs cannot have	caught

Checks at this head

ruff clean; 361 fast, 179 pyomo, 1 notebook, mkdocs build clean. All 9 GitHub checks pass, including the Notebook lane. mypy still fails on a numpy stub in this environment identically at origin/main — pre-existing and advisory per AGENTS.md.

[bernalde]

Re-review of 384a6f2, after the prior review at d6681ef.

The earlier baseline-claim guard and fixture-consistency findings are addressed: the committed baseline is now pinned to the convergence level the documents state, the live notebook guard checks that level, and the tighter fixture derives its quality from its own message. Keeping the tutorial in this PR remains the right scope call.

One new blocking finding is posted inline on the notebook source. The implementation and fresh notebook execution produce the intended labels, but the tracked outputs were not refreshed, so the built reader-facing page still publishes the old termination: optimal and (converged) output.

Checks

• python -m ruff check lyopronto tests examples main.py — passed.
• python -m pytest tests/test_pyomo_models/test_paper_ocp.py tests/test_pseudosteady_limit_study.py tests/test_pseudosteady_frozen_layer.py -n 0 -q -m "not notebook" — 80 passed, 1 deselected.
• python -m pytest tests/test_pseudosteady_frozen_layer.py -n 0 -q -m notebook — 1 passed, 4 deselected; the fresh output contains accepted_at_acceptable_tol as intended.
• mkdocs build — completed; inspecting the generated HTML reproduced the stale committed-output finding.
• Direct POUNCE 0.9.0 probes returned SolveSucceeded at tol and SolvedToAcceptableLevel at acceptable_tol; both classify correctly at this head.
• All nine GitHub checks currently pass at 384a6f2, including the solver lane that executes this notebook.

Summary

• Blocking: 1 inline
• Nonblocking: 0
• Questions: 0

Closes #146 remains appropriate once the tracked notebook output is refreshed. I would not merge this until the blocking issue above is addressed.

[bernalde]

Blocking — these cell sources now print convergence quality, but the tracked outputs were not refreshed. The committed cell-8 output still renders a termination column containing optimal, and cell 12 still renders (converged). I confirmed that mkdocs build publishes both stale outputs, so the reader-facing notebook still shows the exact conflation this PR fixes. A fresh papermill run produces the correct accepted_at_acceptable_tol output, but its ignored *_output.ipynb does not update this tracked notebook. Re-execute and commit the outputs here, correct the PR body's claim that no executed artifact is committed, and extend test_notebook_ships_executed_outputs (or add a focused guard) to require the committed outputs to carry the quality rather than the stale termination/converged rendering.

[bernalde]

Confirmed and fixed in f6b8d51 (outputs) and d6dd8cc (guard). You are right, and my earlier check was aimed at the wrong file: I verified pseudosteady_frozen_layer_output.ipynb regenerated identically and concluded the notebook was fine, but that file is gitignored — the tracked notebook carries its own committed outputs, and those are what mkdocs publishes.

Regenerated with a no-parameter papermill run matching the committed metadata.papermill, verified outputs-only (every cell source byte-identical to 384a6f2; only cells 8 and 12 move). The built page now renders accepted_at_acceptable_tol 19 times and (converged) zero times. test_committed_outputs_render_the_convergence_level now fails if sources and committed outputs disagree — reintroducing the stale outputs makes it red, which is the check that was missing. PR body corrected too.

[bernalde]

Addressed the stale-outputs review at d6dd8cc. Commits: f6b8d51 (regenerated outputs), d6dd8cc (guard).

Blocking: committed notebook outputs were stale — fixed

The finding is correct, and my earlier reasoning was wrong in a way worth stating plainly. I checked pseudosteady_frozen_layer_output.ipynb for staleness, found papermill reproduced it byte-identically, and concluded the notebook was fine. That is the wrong file. It is gitignored — a validation artifact. The tracked notebook carries its own committed outputs (8 cells), and that is what mkdocs publishes.

So editing the cell sources in d6681ef did nothing for a reader: the published page kept rendering a termination column of optimal and (converged) per rung — the conflation of #146, still on the page, under sources that had already been fixed. Confirmed before fixing: cells 8 and 12 at 384a6f2 had sources containing convergence_quality and outputs containing neither.

Nothing caught it. The notebook lane passed, all nine checks passed, and the full local suite passed, because no check compared the committed outputs against the sources that produce them.

Regenerated (f6b8d51) with a no-parameter papermill run, matching what the committed metadata.papermill records — parameters: {}, repo-relative paths, no injected-parameters cell. Verified outputs-only: every cell source is byte-identical to 384a6f2, and cells 8 and 12 are the only outputs that move. The built site now renders accepted_at_acceptable_tol 19 times and (converged) zero times; the one remaining termination on the page is the explanatory code comment, not an output.

Guarded (d6dd8cc) with test_committed_outputs_render_the_convergence_level: exactly two cells must render the level, and no committed output may still carry the pre-#146 form. Its docstring records the regeneration command so this is not re-derived.

PR body corrected

The body claimed "no executed artifact is committed", which is what let this through. It now says the tracked notebook does commit its outputs, names the gitignored file as the validation artifact rather than the published one, and points at the new guard. The stale local test counts were refreshed at this head. The same wrong sentence is in d6681ef's commit message, which is pushed history and left as-is.

Mutation evidence

Probed one defect class at a time; "caught" means the mutation made the guard fail, which is the intended, reassuring outcome:

Mutation	Result
sources edited but notebook never re-executed — the exact regression	caught
only the baseline table refreshed, live rerun forgotten	caught
outputs cleared entirely (nbconvert ClearOutput)	caught by the new guard and the pre-existing test_notebook_ships_executed_outputs

CI

All 9 checks green at d6dd8cc. The Pyomo solver lane failed on the first attempt in its Install Pyomo solver extensions step — http.client.RemoteDisconnected fetching the IDAES binary, before any test ran — which is environmental and not attributable to these commits: the same lane passed at 384a6f2, and this push changes only a notebook's outputs and one test file. One rerun of the failed jobs; attempt 2 passed, and its log confirms 128 tests actually ran rather than the lane going green on a skipped install.

Local at this head: ruff clean; 362 fast, 179 pyomo, 412 full non-Pyomo, 6 notebook; mkdocs build clean.

Still open

The notebook-scope-call question is unchanged and still yours to decide. It is worth noting that this finding is an argument on both sides: it is more work carried by folding the tutorial in, and it is also a defect that only became visible because the tutorial is in scope here.