Repository navigation
E1c: IO CostHistory and the (1 × 3) Cost breakdown (spec + both ports) - #209
Open
pdlourenco wants to merge 3 commits into
Open
pdlourenco wants to merge 3 commits into
pdlourenco wants to merge 3 commits into
Conversation
…5 breakdown SPEC.md §8.12.9: the per-iteration history of `sidLTVdiscIO` is now `CostHistory` (every J of the alternation across all §8.12.4 stages, the returned estimate's J appended when it differs from the last entry; length Iterations or Iterations + 1; one entry on the full-rank fast path). `Cost` becomes the §8.5 (1 x 3) [total, data_fidelity, regularization] at the returned estimate, with data fidelity = observation + dynamics terms and regularization = the N·lambda smoothness term, so Cost(1) == CostHistory(end). §8.12.2 states the monotone decrease for the mu = 0 alternation only (the trust-region mu > 0 stages are not a descent); §8.12.11's usage example and the §8.12 Verified-by line follow. spec/cosmic/output.md and EXAMPLES.md §3.11 (v1.0.2, PATCH) follow the rename. Decided by the maintainer on 2026-10-08 (review-v2 plan §0.3, item E1c). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implements SPEC.md §8.12.9 as amended in 02865c3: - sidLTVdiscIO / ltv_disc_io return the per-iteration history as CostHistory / cost_history (contents unchanged) and Cost / cost as [total, data_fidelity, regularization] at the returned (best) estimate. evaluateFullCost / _evaluate_full_cost return the terms; the inner loop keeps them with the best iterate, so the breakdown is the returned estimate's, and Cost(1) == CostHistory(end) exactly. - Docs: function headers and Changelog blocks, sidResultTypes, both READMEs, exampleOutputCOSMIC.m and example_output_cosmic.ipynb. - Tests: the history assertions move to CostHistory; new test_sidLTVdiscIO.m Test 32 and TestLTVDiscIOCostBreakdown check the shape, Cost(1) == Cost(2) + Cost(3) == CostHistory(end), the history length (Iterations or Iterations + 1; 1 on the fast path) and recompute the §8.12.2 fidelity and N*lambda smoothness terms independently, on the EM, fast and trust-region paths. - testdata: reference_ltv_io keeps its payload and its 'Cost' key (the history). The generator writes CostHistory under it, and the Octave validator and the Python consumer map it, until the vector is re-derived with the new key. No vector bytes change. - CHANGELOG.md: breaking entry under [Unreleased]. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Cost-breakdown tests (MATLAB Test 32, Python TestLTVDiscIOCostBreakdown) use a non-identity R in every case, so an observation term weighted by R instead of R^-1 fails; the trust-region case uses TrustRegionTol 1e-2 and MaxIter 20 (5.7 s -> 1 s in Python). - Test 32 restores the warning state when an assertion fails. - Changelog entries carry "by Author"; sidResultTypes states when the returned J is appended; stale "(8 tests)" docstring dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pdlourenco
commented
Oct 8, 2026
pdlourenco
left a comment
Owner
Author
There was a problem hiding this comment.
Review of PR #209 at b60d4ec: E1c, IO CostHistory and the (1 × 3) Cost
Posted as a comment review because this session's GitHub identity is the PR author. Verdict: approve. There are no blockers. Two should-fix test items, each one line per port, should land before merge. #209 conflicts textually with #210 and #211 in one hunk; the resolution is under Merge notes.
Method. Two reviewers with fresh context each took one lens: (1) spec conformance and code, and (2) tests and governance. An independent verifier then tried to refute each finding. A separate agent trial-merged all five open PRs and ran both full suites.
The work included:
- Python and Octave 8.4 runs: the IO tests,
validate_reference.mand the full Python suite; - 9 mutants per port against the new tests;
- a constructed trust-region case where the best iterate is not the last one;
- variable-length and cross-port checks.
Disclosure: this session wrote the E1c brief (plan §5 item 3); it did not write this PR.
Should-fix
- The "independent recomputation" weights the observation term with the R the port returned, not the R passed in (
python/tests/test_ltv_disc_io.py:510,matlab/tests/test_sidLTVdiscIO.m:1127).- A port that silently ignores the user's
R, using and returningeye(py), passes every IO test in both ports:- Python mutant: 15 passed.
- MATLAB mutant under Octave: 32/32 passed, Test 32 included.
- No other IO test pins
R. The brief asks for the recomputation "from the inputsY,U,H,R". - Fix: in Python,
Rinv = np.linalg.inv(R), the parametrised input; in MATLAB,Rinv32 = inv(cases32{c, 4});. With that change:- Python catches the mutant (2 failed; the fast H = I case cannot see R).
- MATLAB catches it ("EM path: data fidelity 0.229… vs recomputed 0.371…").
- The unmutated head still passes.
- Optionally also assert that
res.r/r32.Requals the input, to pin §8.12.9's "R: noise covariance used".
- A port that silently ignores the user's
- "Appended only when it differs from the last entry" is not pinned in Python (
test_ltv_disc_io.py:505; alsotest_sidLTVdiscIO.m:1121-1123).- The new length check accepts
Iterations + 1without checking the condition, so an unconditional append passes. - In Python only the
reference_ltv_iovector length catches this, and A6 will re-derive that vector. In MATLAB, Test 24 happens to catch it. - Fix: in both new tests,
if hist.size == res.iterations + 1: assert hist[-1] != hist[-2], and the MATLAB equivalent.
- The new length check accepts
Nits
- SPEC.md:1591 still says the uncertainty fields are "computed at final iteration". Both ports compute them from the returned (best) iterate (
sidLTVdiscIO.m:223-224,ltv_disc_io.py:408), and the rows just above now say "at the returned estimate". Change it to "computed at the returned estimate". - The §8.12 Verified-by line (
:1833) says the unit tests pin theCostbreakdown. It should also record that best-versus-last is unpinned. On every fixture the returned iterate is the last one evaluated: a "last-evaluated breakdown" mutant passes 15/15, as the PR body says. Append "(the breakdown being the best rather than the last-evaluated iterate's:manual)". The §8.12.4 line already uses that form. SPEC.md:1478cites "the monotone decrease claimed in §8.12.3", but the claim is in §8.12.5 item 1 (Proposition 1). This predates the PR, but the sentence was rewritten here.- Scale: the IO
Costis 2N timessidLTVdisc'sCostat H = I. Measured in Python and Octave with N = 40, the ratio is [80, 80, 80]. The CHANGELOG's "as forsidLTVdisc" invites a comparison of values. Add a scale clause, e.g. "same layout assidLTVdisc'sCost, on the Output-COSMIC objective's scale (§8.12.2)". Leave the pre-existing SPEC.md:1480 "N times f(C)" error (it should be 2N) to E1a, which already tracks it. - Fast-path case: with square H = I, the weighted-LS state equals y, so the observation term is exactly 0 and the "every case uses a non-identity R, so R instead of R⁻¹ also fails" claim doesn't hold for that case. Only the EM and trust-region cases catch it. Use a tall full-column-rank H for the fast case (e.g.
[1 0; 0 1; 1 1]with a 3×3 non-identity R), or reword the comment. - PR description: the
spec/cosmic/output.mdedit is in §5.1, not §3.
Merge notes (cross-PR)
- Conflict with #210 and #211: both conflict with this PR in exactly one hunk,
TestCrossValidationLTVdiscIO.test_ltv_ioinpython/tests/test_cross_validation.py. Whichever PR lands second should resolve it so that both changes survive:_assert_allclose( # The vector key "Cost" holds the history (SPEC.md §8.12.9 # CostHistory) until the vector is re-derived with the new key. np.asarray(result.cost_history).ravel(), _to_array(ref["output"], "Cost").ravel(), err_msg="LTV-IO cost history mismatch vs MATLAB", **_tol(ref, "Cost"), )
validate_reference.mandgenerate_reference.mauto-merge and keep both sides. - Trial merge: all five PRs merged with that hunk resolved. Python 500 passed, 4 skipped. The Octave validator gave 32/32, and
test_sidLTVdiscIO32/32. - Follow-up for A6: when A6 re-derives the vector, remove all three shims together: the generator mapping, the
validate_reference.mresult.Cost = result.CostHistoryline, and the Pythoncost_historymapping.
Verified
- Spec first: commit
02865c3(spec only) comes before the code. It updates every location in the brief: §8.12.2 (monotone claim limited to μ = 0), the §8.12.9 rows, §8.12.11, the Verified-by line (no plan ids),output.md, andEXAMPLES.md§3.11 (bumped to 1.0.2, with a §6.5 entry). There is no SPEC.md version bump, as planned. - The §8.12.9
CostHistoryrow matches both ports exactly: one entry per inner iteration across all stages, the returnedJappended only if it differs, and a single entry withIterations = 0on the fast path. - No double evaluation:
Jparts/J_partscome from the same evaluation asJand are carried with the best iterate. The history is unchanged to the bit. - Best ≠ last, constructed independently: a trust-region run with 1200 iterations gives n_h = 1201. In both ports
Cost(1) == CostHistory(end)holds exactly and the recomputed terms match. So both ports report the best iterate's breakdown. The case is too slow and engine-sensitive to use as a fixture. - Variable-length input (EM, TR and fast paths): the invariants hold, and an independent recomputation matches to ≤ 4e-16 relative.
- Cross-port, on the
reference_ltv_ioinputs: both ports take 14 iterations,Costagrees, and the history matches the vector within 5.3e-5 (Octave) and 2.9e-4 (Python) against the 1e-2 tolerance. - Key mapping and payload: the key mapping keeps
reference_ltv_io's payload andCostkey, and no JSON changed. Running the generator block on the stored inputs gives 14 entries and keysA/B/Cost. - Old field meaning: nothing in either port's library, the examples, notebooks, READMEs, docs, docsite or the spec still reads the old meaning of
Cost. - Tests catch real mutations: swapped terms, λ instead of N·λ, the dynamics term moved into regularization, R in place of R⁻¹ (EM/TR cases), and a broken fast-path breakdown.
- Docs and changelog: CHANGELOG
[Unreleased]uses the 0.2.0 heading convention. The README tables are updated, and the v0.2.0 RELEASE_NOTES are untouched. Changelog blocks follow both guides.ruff, MISS_HIT and the header checkers are clean apart from 2 pre-existing warnings.
Generated by Claude Code
This was referenced Oct 8, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements E1c of the review-v2 remediation plan (
docs/plans/2026-10-04-review-v2-remediation-plan.md§5 item 3), the IO cost-field decision taken on 2026-10-08 (plan §0.3). This is the LOCAL session's assignment, and it unblocks A6.sidLTVdiscIO/ltv_disc_ionow return the per-iteration history asCostHistory/cost_history. The contents are unchanged.Cost/cost: the §8.5(1 × 3)[total, data_fidelity, regularization]at the returned (best) estimate.N·λsmoothness term.Cost(1) == CostHistory(end)exactly..Cost/.costchanges meaning. There is an entry inCHANGELOG.mdunder[Unreleased].evaluateFullCost/_evaluate_full_costnow also return the three terms, and the inner loop keeps them with the best iterate. So the breakdown is the returned estimate's, not the last evaluated one's.reference_ltv_iokeeps its payload and itsCostkey, which holds the history. The generator writesCostHistoryunder that key, and the Octave validator and the Python consumer map it, each with a comment. A6 renames the key when it re-derives the vector. No vector bytes change in this PR.Spec sections touched
The spec was updated first, in
02865c3; both ports follow in the same PR.SPEC.md§8.12.9: theCostHistoryandCostrows.IterationsorIterations + 1, and 1 on the full-rank fast path.SPEC.md§8.12.2: the monotone claim is limited toμ = 0alternations; the trust-regionμ > 0stages are not a descent (§8.12.4 step 2).SPEC.md§8.12.11: the usage example plotsCostHistory.Verified by:line.spec/cosmic/output.md§3.spec/EXAMPLES.md§3.11: version 1.0.2 (PATCH) with a §6.5 entry.No
SPEC.mdversion bump here. Per the plan, E1b's version row covers that if it is accepted; theCHANGELOG.mdentry records the change either way.Type of change
Checks
test_sidLTVdiscIO.mTest 32 and PythonTestLTVDiscIOCostBreakdown.Cost(1) == Cost(2) + Cost(3) == CostHistory(end); the history length.N·λsmoothness terms independently, with a non-identityR. That catches swapped or mis-scaled terms, andRused in place ofR⁻¹.μ = 0pass; on all three paths the returned iterate is also the last one evaluated.spec/SPEC.md. Not applicable: none.b60d4ec:Rin the breakdown tests;sidResultTypeswording.example_output_cosmic.ipynbruns under nbmake.validate_reference.m: 32/32, includingreference_ltv_iothrough the key mapping.generate_reference.monce on R2024a.reference_ltv_io.jsonstill has the keysA/B/Cost, withCostthe 14-entry history, within 7e-5 of the committed R2025a bytes.git restore -- 'testdata/reference_*.json'.🤖 Generated with Claude Code