A3: store NaN/Inf/-Inf as strings in reference vectors; ADR-0007

[pdlourenco]

Summary

Implements plan item A3 of the review-v2 remediation plan (docs/plans/2026-10-04-review-v2-remediation-plan.md, decided 2026-10-08, §0.3). Lands ADR-0007, which records the A2 and A3 decisions together and supersedes ADR-0002.

The defect (analysis §4.1 F3):

• jsonencode writes NaN, Inf and -Inf all as null, so the §3.3 Inf sentinel can't be stored distinctly.
• The Octave validator's any(absDiff > thresh) is false for any NaN, so a NaN on either side passed.

Now:

• On disk: non-finite values are the JSON strings "NaN", "Inf" and "-Inf". That is still valid JSON, and the three strings are now reserved.
• Generator: writeJSON encodes through testdata/refEncodeNonFinite.m and errors if a null or a bare token would reach the file.
• Octave/MATLAB validator:
  • decodes through testdata/refDecodeNonFinite.m (string → bare token → jsondecode), so arrays keep their shape;
  • fails any vector holding a null;
  • compares non-finite values exactly: NaN only with NaN, Inf only with Inf of the same sign;
  • starts every run with a self-test that round-trips the encoding through the generator's real encoder and checks that every non-finite mismatch fails.
• Python consumer:
  • _load decodes the strings and rejects null.
  • Every comparison now goes through _assert_allclose, which compares real and imaginary parts separately wherever a value is non-finite. numpy alone treats 5+nanj as equal to 3+nanj.
  • _to_complex no longer turns a finite real part into NaN next to an infinite imaginary part.

No committed vector holds a non-finite value today, so no vector bytes change. This unblocks degenerate-input cross-vectors (D4, A6).

Spec sections touched

None. This is no algorithmic change; it changes the reference-vector format and the consumers.

For the maintainer

• ADR-0007 fully supersedes ADR-0002. It carries the five rules forward unchanged, because the decisions README has no way to supersede part of an ADR.
• The bot's always-bypass is not removed here. §0.3 assigns its removal to "the A2 ADR". ADR-0007 states the removal as a consequence, but leaves ADR-0005's status to the A1 ADR, and notes the bypass can go only once A2's CI changes are in place. Removing it earlier would block the bot's push of regenerated vectors to main. This is a small departure from §0.3's wording; tell me if you'd rather ADR-0007 handle ADR-0005 too.
• CONTRIBUTING.md (a governance doc) gets only a pointer to ADR-0007 and one paragraph stating the accepted encoding rule. No rule is changed.
• Follow-up, not done: python/tests/conftest.py's unused load_reference doesn't decode. It's scheduled for removal under A8 / §2.2 Add edge case and input validation tests (SPEC compliance batch 4) sidFreqBT (Tests 15-16): - WindowSize M=1 rejected with error per SPEC §2.2 - Near-zero input (1e-15 amplitude) handled without crash sidDetrend (Tests 9-11): - High polynomial degree (Order=5) correctly removed - Order >= N gracefully clamped to N-1 (fits data perfectly) - SegmentLength not dividing N: last short segment detrended sidCompare (Test 10): - InitialState parameter: correct x0 gives ~100% fit, wrong x0 gives degraded fit per SPEC §15.4 sidModelOrder (Tests 13-14): - White noise (no system): returns n well below Hankel dimension - Threshold tuning: strict threshold (0.5) gives smaller n than loose threshold (0.001) sidLTVdisc (Tests 19-20): - NaN in X and Inf in U both rejected with sid:nonFinite error - N=2 minimum viable data produces valid output https://claude.ai/code/session_01XFBhUKcxba3w4AjLaw7qDY #35, and python/CONTRIBUTING.md:403 still documents it as the shared loader.

Type of change

• Docs / tooling / CI only (test infrastructure and the vector format; no algorithm or public API change)

Checks

• Tests written against the contract (ADR-0007's comparison rules), not against another port's output.

• Major / architectural decision made? It was taken by the maintainer 2026-10-08 and is recorded as ADR-0007.

• Pre-push review: 8 findings, 0 blocking. Fixed in bf71afe:

  • the Python complex-NaN hole and _to_complex;
  • ADR-0007 missing the non-committing check on main;
  • the bypass ordering;
  • the jq claim and an attribution;
  • CONTRIBUTING pointer;
  • a stray self-test print.

  Deferred: the unused conftest loader, noted above.

• Local CI: green.

  • Python: 451 passed, 4 skipped.
  • validate_reference.m: 32/32 under both Octave 11.3 and MATLAB R2024a, with the self-test passing in both.
  • generate_reference.m runs through the new encoder with no null, no tokens and no key changes; the output was discarded.
  • Mutation check under Octave: injecting "NaN", and separately null, into reference_siso_bt.json's expected Response_real fails the validator, 31/32 each time. Before this change a stored null passed silently.
  • ruff clean. No files under matlab/ changed.

🤖 Generated with Claude Code

[pdlourenco]

Review of PR #210 at bf71afe: A3 (non-finite sentinels) and ADR-0007

Posted as a comment review because this session's GitHub identity is the PR author. Verdict: request changes. Two items need to be settled before merge:

• #7, a shape check the rewrite quietly dropped;
• #8, one sentence in ADR-0007. Accepted ADR text can't be edited after merge, so it has to be settled now.

Everything else can follow. The encoding itself is correct: the encode/decode round trip and the exact non-finite comparison both check out.

Method. Two reviewers with fresh context each took one lens: (1) encoding, decoding and comparison, and (2) ADR-0007, the docs, governance and the test-suite rewrite. An independent verifier then tried to refute each finding. A separate agent trial-merged all five open PRs.

The work included:

• Octave 8.4 round trips over 17 value shapes;
• a mutation matrix over both consumers, including the old validator to confirm the blind spot;
• an AST diff of test_cross_validation.py before and after;
• jq and node checks of the ADR's claims.

Disclosure: this session wrote the remediation plan (A2/A3 rows); it did not write this PR.

Fix before merge

7. _assert_allclose drops numpy's shape check for complex inputs (python/tests/test_cross_validation.py:85-108, np.broadcast_arrays at :99).
   • The docstring says the helper is assert_allclose with exact non-finite handling. For complex inputs, though, it broadcasts both sides and compares masked, flattened subsets. Shapes like (1, n) vs (n,) or (1,) vs (n,) now pass where numpy fails them.
   • Mutation: make freq_btfdr return G[np.newaxis, :] for SISO. The PR-head test_btfdr_response passes, while origin/main's version fails on "(shapes (1, 128), (128,) mismatch)". The full suite still catches this mutant, but only incidentally, through an IndexError in test_known_first_order.
   • The Octave validator checks only numel, so for complex fields the Python cross-check was the shape check. REVIEW_CONTEXT treats a silently weakened cross-validation as strict.
   • Fix: before broadcast_arrays, add if a.shape != d.shape and a.ndim and d.ndim: raise AssertionError(f"{err_msg}\n(shapes {a.shape}, {d.shape} mismatch)"). That is numpy's own rule. Add a complex (1,n) vs (n,) case to test_comparison_is_exact_on_non_finite. With the check in place, all 104 tests pass and the mutant fails.
8. ADR-0007 gives the bot-bypass removal to A1's ADR, but nothing orders A1 after A2 (ADR-0007…md:74-79).
   • Plan §9 runs A1 and A2 in parallel, and §0.3's "Lands with" gives the bypass to the A2 PR.
   • If A1 lands first, its ADR can't record a ruleset without the bypass, because the bot still pushes to main until rule 1 is implemented. This accepted text would then be false, and the removal would have no owner.
   • The PR body surfaces the departure from §0.3, which satisfies CLAUDE.md §4. Even so, the ordering assumption is written into immutable text.
   • Recommended: give the removal to the PR that implements rule 1 (plan A2), e.g. in the "Not closed by this ADR" bullet: "the reference bot's always-bypass in .github/rulesets/main.json is removed by the PR that implements rule 1 (plan item A2), after which the maintainer re-applies the ruleset; ADR-0005's status changes only through the ADR that supersedes it". Alternatively, keep §0.3's assignment. This is the maintainer's choice.

Should-fix

2. The Octave/MATLAB validator still builds the complex input as re + 1i*im (validate_reference.m:168). That is the 0·Inf defect this PR fixes in Python's _to_complex: under Octave, 1 + 1i*Inf gives NaN + Inf i. The two consumers would feed sidModelOrder / model_order different inputs if a non-finite input were ever stored, which is the degenerate case A3 exists to unblock. Fix: resp = complex(input.Response_real, input.Response_imag);.
3. The MATLAB side of the encoding never runs in CI on R2025a. selfTestNonFinite runs only inside validate_reference.m, which CI runs only under Octave (cross-validate.yml:81). The MATLAB job runs the generator on data with no non-finite values, so the token rewrite never executes on the canonical engine. The PR's MATLAB evidence is a local R2024a run. Plan §10.3 row A3 names the fix: add a run('testdata/validate_reference.m') step to the MATLAB job in tests.yml.

Nits

1. Reserved strings aren't enforced. A text field holding "NaN", "Inf" or "-Inf" is written without error and decoded as a number by both consumers. No current field does this, and ADR-0007 records the reservation as a cost. A one-line guard in refEncodeNonFinite.m would enforce it, e.g. regexp(json, '[\[,:]\s*"(NaN|-?Inf)"\s*[,\]\}]') raising refEncodeNonFinite:reserved. That regex matches no committed vector.
2. Bare NaN/Infinity tokens aren't rejected by either consumer. Only the generator guards against them. Widen the validator's null regex (:61) to (null|NaN|-?Infinity), and use json.load(..., parse_constant=...) in Python.
3. The encoder's rewrite and guard regexes have no \s* (refEncodeNonFinite.m:16-18), unlike the decoder and the validator. That is latent while writeJSON stays compact.
4. Malformed sentinel strings are handled differently. Python accepts "nan", "Infinity" or "0.99" through numpy's float conversion. Octave aborts the whole run in cell2mat instead of recording a FAIL.
5. load_reference doesn't decode or reject null. conftest.load_reference is documented as the shared loader (python/CONTRIBUTING.md:403) but neither decodes nor rejects null. It is unused today, and the PR defers it to A8.
6. Two living-doc citations still name only the superseded ADR-0002: python/CONTRIBUTING.md:548 and testdata/README.md:104. Append ", carried forward by ADR-0007", as CONTRIBUTING.md:125-126 does.
7. testdata/README.md's Format section still says arrays are "JSON arrays of numbers". Mention the three strings there, or move the "Non-finite values" subsection under Format.

Merge notes (cross-PR)

• Conflict with #209: this PR conflicts with #209 in one hunk of test_cross_validation.py. Whichever PR lands second resolves it to _assert_allclose(np.asarray(result.cost_history).ravel(), ...), keeping A3's wrapper and E1c's field. The exact text is in the #209 review.
• Trial merge: all five PRs merged clean. Python 500 passed, 4 skipped; the Octave validator gave 32/32 with the self-test clean.
• #211 is stacked on this branch. Once this PR merges, the bot re-stamps all 32 single-line vectors. #211's 10 edited vectors will then conflict as whole files. See the #211 review for the resolution.

Verified

• Committed vectors: none of the 32 holds null, a bare token or a reserved string, and no vector bytes change in this PR.
• Round trip: encode → decode → jsondecode keeps class and size for 17 value shapes. That includes N-D arrays, all-NaN arrays, nested structs, logicals, ints, empty arrays and variable-length cell trajectories. The decoder rewrites values only, never keys.
• Comparison semantics, in both consumers:
  • the Inf sign is checked;
  • NaN matches only NaN;
  • a finite entry next to a matched NaN still uses rtol/atol (a 1e-11 perturbation fails, 1e-18 passes);
  • a null fails both consumers.
• Old blind spot closed: origin/main's validator passed a stored null and an actual NaN against a finite expected value. This PR fails both.
• Self-test: disabling the mismatch check or dropping the Inf sign in the decoder makes the self-test fail.
• Test-suite rewrite: an AST diff shows all 68 original tests present. All 84 assertion calls keep identical arguments; only the callee changed. No tolerance was changed and no test was skipped (apart from finding #7 above).
• ADR-0007: it has every section the README requires. ADR-0002's change is a single Status line, and the index is updated.
  • The A2/A3 content matches §0.3, and the ADR says plainly that rules 1–2 are not yet implemented.
  • The Context facts (21 of 32, 4.5e-5, re-stamps) and the jq/JSON.parse claims check out.
  • ADR-0005 stays Accepted, which is consistent with today's state.
• Commit order: ADR first, then the code, then the pre-push fixes.
• Reproduced PR claims: 451 passed, 4 skipped, and ruff clean.

---

Generated by Claude Code