Skip to content

E1c: IO CostHistory and the (1 × 3) Cost breakdown (spec + both ports) - #209

Open
pdlourenco wants to merge 3 commits into
mainfrom
claude/e1c-io-cost-fields
Open

pdlourenco wants to merge 3 commits into
mainfrom
claude/e1c-io-cost-fields

Conversation

@pdlourenco

Copy link
Copy Markdown
Owner

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.

  • Rename: sidLTVdiscIO / ltv_disc_io now return the per-iteration history as CostHistory / cost_history. The contents are unchanged.
  • New Cost / cost: the §8.5 (1 × 3) [total, data_fidelity, regularization] at the returned (best) estimate.
    • Data fidelity is the observation + dynamics terms of §8.12.2; regularization is the N·λ smoothness term.
    • Cost(1) == CostHistory(end) exactly.
  • Breaking: the released .Cost / .cost changes meaning. There is an entry in CHANGELOG.md under [Unreleased].
  • How the ports do it: evaluateFullCost / _evaluate_full_cost now 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_io keeps its payload and its Cost key, which holds the history. The generator writes CostHistory under 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: the CostHistory and Cost rows.
    • History length is Iterations or Iterations + 1, and 1 on the full-rank fast path.
  • SPEC.md §8.12.2: the monotone claim is limited to μ = 0 alternations; the trust-region μ > 0 stages are not a descent (§8.12.4 step 2).
  • SPEC.md §8.12.11: the usage example plots CostHistory.
  • The §8.12 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.md version bump here. Per the plan, E1b's version row covers that if it is accepted; the CHANGELOG.md entry records the change either way.

Type of change

  • Algorithmic change (touches a spec contract — spec updated first)

Checks

  • Implementations derive behaviour independently from the spec, not ported copy-by-copy between languages
  • Tests written against spec requirements, not against the other language's current output.
    • New: MATLAB test_sidLTVdiscIO.m Test 32 and Python TestLTVDiscIOCostBreakdown.
    • On the EM, fast and trust-region paths, they check: the shape; Cost(1) == Cost(2) + Cost(3) == CostHistory(end); the history length.
    • They also recompute the §8.12.2 fidelity and N·λ smoothness terms independently, with a non-identity R. That catches swapped or mis-scaled terms, and R used in place of R⁻¹.
    • Not pinned: the best-versus-last requirement. As the brief notes, no existing fixture rejects the guarded μ = 0 pass; on all three paths the returned iterate is also the last one evaluated.
  • Shared helpers touched? Not applicable: none.
  • New defaults / bounds / thresholds are covered by spec/SPEC.md. Not applicable: none.
  • Major / architectural decision made? It was taken by the maintainer (plan §0.3); the plan recommends no ADR.
  • Pre-push review: no blocking findings. The optional points are fixed in b60d4ec:
    • non-identity R in the breakdown tests;
    • a cheaper trust-region case (5.7 s → 1 s);
    • warning state restored when an assertion fails;
    • Changelog "by Author";
    • sidResultTypes wording.
  • Local CI: green.
    • MATLAB R2024a: 41/41 files, 465/465 cases.
    • Python 3.14: 418 passed, 4 skipped; example_output_cosmic.ipynb runs under nbmake.
    • Octave 11.3 validate_reference.m: 32/32, including reference_ltv_io through the key mapping.
    • Lint: ruff, MISS_HIT lint/style/metric and both header checkers are clean (the two header warnings are pre-existing).
  • Generator check (PR CI never compares regenerated vectors):
    • Ran generate_reference.m once on R2024a. reference_ltv_io.json still has the keys A/B/Cost, with Cost the 14-entry history, within 7e-5 of the committed R2025a bytes.
    • Then discarded the regenerated vectors with git restore -- 'testdata/reference_*.json'.

🤖 Generated with Claude Code

pdlourenco and others added 3 commits October 8, 2026 13:08
…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 pdlourenco left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.m and 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

  1. 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 returning eye(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 inputs Y, 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.R equals the input, to pin §8.12.9's "R: noise covariance used".
  2. "Appended only when it differs from the last entry" is not pinned in Python (test_ltv_disc_io.py:505; also test_sidLTVdiscIO.m:1121-1123).
    • The new length check accepts Iterations + 1 without checking the condition, so an unconditional append passes.
    • In Python only the reference_ltv_io vector 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.

Nits

  1. 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".
  2. The §8.12 Verified-by line (:1833) says the unit tests pin the Cost breakdown. 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.
  3. SPEC.md:1478 cites "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.
  4. Scale: the IO Cost is 2N times sidLTVdisc's Cost at H = I. Measured in Python and Octave with N = 40, the ratio is [80, 80, 80]. The CHANGELOG's "as for sidLTVdisc" invites a comparison of values. Add a scale clause, e.g. "same layout as sidLTVdisc's Cost, 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.
  5. 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.
  6. PR description: the spec/cosmic/output.md edit 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_io in python/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.m and generate_reference.m auto-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_sidLTVdiscIO 32/32.
  • Follow-up for A6: when A6 re-derives the vector, remove all three shims together: the generator mapping, the validate_reference.m result.Cost = result.CostHistory line, and the Python cost_history mapping.

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, and EXAMPLES.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 CostHistory row matches both ports exactly: one entry per inner iteration across all stages, the returned J appended only if it differs, and a single entry with Iterations = 0 on the fast path.
  • No double evaluation: Jparts / J_parts come from the same evaluation as J and 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_io inputs: both ports take 14 iterations, Cost agrees, 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 and Cost key, and no JSON changed. Running the generator block on the stored inputs gives 14 entries and keys A/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 branch has not been deployed

No deployments
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