B1: threshold-method test asserts the true order (both ports)

[pdlourenco]

Summary

Implements plan item B1 of the review-v2 remediation plan (docs/plans/2026-10-04-review-v2-remediation-plan.md; analysis §3.2).

The defect:

• test_sidModelOrder.m Test 5 was widened to 1 <= n <= 12 to absorb the lag-0 bug and never restored. It still cited closed issue sidModelOrder builds the Hankel from lag 0 — model order overestimated by one for generic strictly proper plants #139.
• The Python twin asserted 1 <= n <= 10.

Root cause: the fixture, not the bound. A threshold of 0.01 sits inside the noise floor of the noisy BT estimate. The relative singular values are 1, 0.32, and then a floor starting at about 0.017, so the count was noise:

Engine	Count at 0.01
MATLAB R2024a	4
Octave 11.3 (its randn stream differs)	7
Python, 5 seeds	6

Departure from the plan: the plan offered two options.

• Restore <= 6: this would fail the Octave leg.
• Assert == 2 on Test 15's analytic plant: Test 15 already does exactly that.

So this PR fixes the fixture instead. Both ports now:

• Exact order: use a threshold of 0.1, which lies between the structural singular values and the noise floor, and assert n == 2 exactly. The test also asserts σ₂ > 0.1 > σ₃, so it fails loudly if a future engine moves either value across the threshold.
• Counting rule: at 0.01, assert that n equals the number of σ_k/σ_1 > 0.01 (SPEC §8.12.12) and that n > 2. The gap method also returns 2 on this fixture, so without this check an ignored Threshold option would pass.

The #139 marker is removed. The remaining #139 references explain a fixed bug (Test 15, ADR-0004); they are not deferred-bug markers.

Spec sections touched

None. Tests only.

Type of change

• Bug fix (test)

Checks

• Tests written against the spec rule (§8.12.12's counting rule), not against another port's output.

• Pre-push review: approve with changes, all applied in 8b2d364:

  • add the counting-rule check;
  • fix the margin wording;
  • state the departure from the plan (above).

  One observation is out of scope: the n = max(n, 1) floor in both ports is not stated in §8.12.12.

• Local CI: green.

  • MATLAB R2024a: 464/464 cases.
  • Octave 11.3: 432/432.
  • Python: test_model_order.py 11/11. ruff clean.

🤖 Generated with Claude Code

[pdlourenco]

Review of PR #212 at 8b2d364: B1, the threshold-method test

Posted as a comment review because this session's GitHub identity is the PR author. Verdict: approve. There are no blockers. One should-fix item makes the MATLAB discriminating check robust to the noise draw; I'd land it before merge.

Method. A reviewer with fresh context checked the new assertions against SPEC §8.12.12 and the plants' analytic Hankel spectra. It ran the changed tests in Python and Octave 8.4, swept seeds on both fixtures, and ran 8 sidModelOrder / 7 model_order mutants against the old and new tests. An independent verifier then tried to refute each finding. Disclosure: this session wrote the remediation plan (item B1); it did not write this PR.

Should-fix

1. The MATLAB leg that distinguishes the threshold rule from the gap rule only works because of a lucky noise draw (matlab/tests/test_sidModelOrder.m:109-116).
   • The τ = 0.01 leg is the only check in the MATLAB file that fails when Threshold is ignored and the gap rule runs instead. Tests 12 and 14 both pass under that mutant.
   • The leg relies on n_low > 2, which needs σ₃/σ₁ > 0.01. On this fixture that is a property of the random draw, not of the plant. Over seeds {500, 1..39} under Octave 8.4, 12 of 40 seeds (30 %) give nnz(s > 0.01) = 2. With rng(500) the margin is 0.0163 vs 0.01, about 1.6×.
   • The precondition is also fused into the spec assertion (n_low == nnz(...) && n_low > 2). If the RNG stream or engine changes, a fixture drift would read as a counting-rule failure. The obvious "fix", deleting && n_low > 2, would then quietly remove the discrimination.
   • Fix: use τ = 1e-3 for the counting-rule leg; the minimum count over 40 seeds is 14, against a gap answer of 2. Split the precondition into its own guard:

     assert(nnz(s_low > 1e-3) > 2, 'fixture: threshold not inside the noise floor (%d values above it)', nnz(s_low > 1e-3));
     assert(n_low == nnz(s_low > 1e-3), 'Threshold 1e-3: n = %d, expected nnz(sigma/sigma_1 > 1e-3) = %d', n_low, nnz(s_low > 1e-3));

   • The Python fixture is already robust: its minimum count at 0.01 is 4 over 41 seeds. The same split guard in python/tests/test_model_order.py:107-109 is still recommended, for symmetry.

Nits

2. The fixture: guard also covers a structural claim (test_sidModelOrder.m:104-106, test_model_order.py:102). σ₂/σ₁ > 0.1 follows from the plant's analytic Hankel spectrum: 0.327 for the MATLAB plant and 0.744 for the Python plant. A regression that shrinks σ₂ would be reported as a fixture problem. Consider asserting s(2) > 0.1 with a structural message, and keep the fixture: label for s(3) < 0.1.
3. Test 5's plant is defined only implicitly, inherited from Test 1's a1/a2/b1. Test 1's comment gives the poles as 0.6 ± 0.2i; they are 0.6 ± 0.374i (|p| = 0.707). Both points predate this PR, but Test 5 now quotes plant-specific numbers. Define the plant at the top of Test 5 and correct the comment.
4. PR description:
   • The "remaining #139 references" list is incomplete. More references exist in sidModelOrder.m:149,168, model_order.py:189,204, test_model_order.py:192 and CHANGELOG.md:55. All of them explain the fixed bug, so the substantive claim holds.
   • The figures "1, 0.32, floor ~0.017" describe the MATLAB fixture only. The Python figures are 1, 0.716, then a floor from 0.034.
5. For the maintainer (outside this PR): the n = max(n, 1) clamp is spec-silent and untracked. It is at sidModelOrder.m:223 and model_order.py:265. §8.12.12 says only "the number of σ_k/σ₁ > τ", and Threshold accepts any positive scalar. With τ ≥ 1 both ports return 1 where the spec gives 0. The PR rightly scopes this out, but nothing tracks it (ADR-0003, visible debt). I'd recommend an issue that either states the floor in §8.12.12 or restricts Threshold to (0, 1) with a typed error; that is a contract choice for you.
6. Follow-up, outside the diff: MATLAB Test 12 cannot tell the count rule from the gap rule on its fixture (both give 2), and Test 14 only asserts n_strict <= n_loose. After this PR, Test 5's new leg is what discriminates.

Verified

• Scope: only the two test files change, and "Spec sections touched: none" is accurate.
• Assertions follow from the spec: both new count assertions encode §8.12.12's rule. The analytic order is 2 for both plants, with no pole-zero cancellation. n == 2 at τ = 0.1 follows from the plant, not from the current output.
• Comment figures reproduce: Octave with rng(500) gives s = [1, 0.3227, 0.0163, …], n@0.1 = 2, n@0.01 = 7. Python with seed 42 gives [1, 0.7159, 0.0336, …], n@0.01 = 6.
• The 0.1 leg is robust: over 20 Octave seeds and 41 Python seeds, σ₂/σ₁ ≥ 0.317 and σ₃/σ₁ ≤ 0.067.
• Mutation tests:
  • The new tests kill count − 1, count + 1, an absolute threshold, the gap rule swapped in, a σ₂-relative threshold, and a squared-ratio count. The old Test 5 and the old Python test killed none of these.
  • Only >= in place of > and a ⌊m/2⌋ cap survive. Neither can differ on these fixtures.
• Departure from the plan: the PR does not restore <= 6. That is justified, because Octave gives 7 at that seed. The plan's alternative (== 2 on Test 15's analytic plant) is already asserted at :307-309.
• #139: the marker is gone and the issue is closed (by #157).
• CI: green on the head. MATLAB R2025a logs "Test 5 passed: threshold method returned n = 2", and Octave 7.1.0 logs the same.
• Cross-PR: merges cleanly with the other four open PRs. On the trial merge, Octave Test 5 and the Python test pass.

---

Generated by Claude Code