Stage 3a: sample_traits<double>, named Q ladder, fft_arith trait, decimate re-pin

[tap]

What this changes

Stage 3a of docs/audit-fft-and-code-smells.md (Part 3, decision D1; review amendments in Part 13): the sample-format substrate gains sample_traits<double>, named Q-ladder constants, a tightened sample_type concept, and the butterfly arithmetic trait fft_arith<Sample> (include/tap/dsp/fft/fft_arith.h) whose docstring is the Stage 3b fixed-point kernel's specification. quantize.h selects its algorithm by a trait property and its ±1 correction is rail-aware. decimate instantiates over double, its Q15/Q31 cross-precision pins point at the double golden model, and every fixed-point coefficient table is bit-pinned in-tree.

Why

CLAUDE.md says double is the golden model of every primitive, but the substrate had no double specialization and the test pinned that as intentional, so the one traits-based primitive (decimate) had a float golden model and a traits-based FFT could not instantiate its golden profile. The Q ladder was bare literals (16384.0, (1 << 13)) >> 14, >> 16), the concept omitted k_coeff_scale that quantize.h already required, and the FIR trait's mac/finalize shape is the wrong one for a butterfly (audit Part 2, Part 6 N6/N13/N15, Part 7).

Details:

• sample_traits<double>: coeff = double, accum = double, k_coeff_scale = 1.0, identity conversions. test_sample_traits.cpp's static_assert(!sample_type<double>) flips, with the reason rewritten.
• Concept: now requires coeff, accum, k_coeff_scale (const double), a new k_is_fixed_point (const bool), and documents that accum{} is the additive identity, static_asserted for all four types in the header against silence(). Every member is constexpr.
• Named ladder: k_sample_frac_bits (15/31), k_coeff_frac_bits (14/30), k_accum_pre_shift (0/16); k_accum_frac_bits (29/45) and k_finalize_shift (14 = coeff frac − pre-shift, both profiles) are derived and static_asserted with the derivation in the header comment. k_coeff_scale = 2^k_coeff_frac_bits.
• fft_arith (spec in the docstrings, every number pinned by tests/test_fft_arith.cpp):
  • fft_arith<std::int32_t> (the kernel's data type, Q0.31 or the widened Q2.29): make_coeff(double) -> Q1.30 (round_sat); mul_coeff(int32, Q1.30) -> int32 = exact int64 product, >> 30 with one round-half-up, saturating (with |w| ≤ 1.0 only INT32_MIN × −1.0 saturates); saturating add/sub (int64, clamped); shr_round(x, bits) round-half-up in int64, @pre 0 <= bits <= 31 asserted; headroom_bits(x, n) = redundant sign bits shared by the block (countl_zero of the OR of one's-complement magnitudes, minus the sign bit; 31 for zero/empty and for {-1} — "no information" — 0 for INT32_MAX and INT32_MIN); widen/narrow identities; k_coeff_one = 2^30; k_fixed_scaling_input_pre_shift = 1.
  • fft_arith<std::int16_t> (Q15 I/O width): widen = << 14 (two guard bits), narrow = (x + 2^13) >> 14 in int64, saturating; coeff/make_coeff are the int32 trait's Q1.30 twiddles; work = fft_arith<std::int32_t>; k_fixed_scaling_input_pre_shift = 0.
  • fft_arith<float> / <double>: the same names over plain arithmetic (shr_round is an exact power-of-two scale, headroom_bits is 0).
  • The file header states, as the kernel's spec: fixed scaling is shift-before-butterfly (2 bits per radix-4 stage, 1 per radix-2, 1 before the real post-pass) and under it the complex magnitude never grows, so the bound is √2 × full scale — 2^29.5 for widened Q15 (1.5 bits below the rail, which is why two guard bits suffice) and 2^31.5 for Q31, hence the one-bit Q31 input pre-shift (bound 2^30.5); a complex twiddle product is two roundings per component (one per mul_coeff), and an int64-accumulate-then-shift-once variant is a different, non-bit-identical design; SMMULR is not a bit-exact seam (rounds at bit 32, not 30) and an SMMULR build would be a separately pinned Arm profile; a BFP kernel never consumes the full reported headroom (−2^20 reports 11 and << 11 is INT32_MIN); twiddles are generated in double via std::cos/std::sin and rounded once by make_coeff, with the host-libm identity caveat (the 3b battery pins the table checksum per certified N).
  • Concept fft_sample_type over the four types requires the I/O pair, the twiddle type/generator, k_is_fixed_point, k_guard_bits, k_coeff_frac_bits, k_coeff_one, k_fixed_scaling_input_pre_shift, and every operation noexcept.
• quantize.h: if constexpr (!tr::k_is_fixed_point) instead of is_floating_point_v; each ±1 step goes to the largest remainder among taps that can still move in that direction, so the row sum is preserved whenever one exists (a saturated tap has the largest remainder by construction and would otherwise take and wrap on every step); the loop breaks only when every tap is at the needed rail. Saturation threshold stated correctly (2 − 2^-15 / 2 − 2^-31) and the cost stated (one pass per residual LSB). Tests: NearRailRowStillPreservesItsSum ({1.99997, 0.3, 0.3} → 42598; Q31 → 2791728742), CorrectionNeverWrapsATapAtTheRail (small overshoot both rails, plus the all-at-rail break path), DoubleIsPlainConversion. \pre → @pre.
• decimate: docstring lines calling float "the golden model" updated to the house rule; the decimate_by_* aliases stay float (deployed profile) and the docstring says why. test_decimate.cpp: chunking/DC/group-delay typed over float and double; Q15TracksDoubleWithinTheFormatFloor (measured 1.57e-4, pinned 3.2e-4 — the same number as the former float pin, since float sits 2.5e-8 from double here); Q31TracksDoubleWithinTheFormatFloor (2.45e-9, pinned 4.9e-9); DoubleMatchesNumpyReferenceToItsFloatRounding (5.75e-8 over all six ratio/profile pairs = the float32-rounded reference's own rounding; pinned 1.15e-7); FixedPointTablesAreBitPinned: row sum (2^14 / 2^30) and a byte-wise FNV-1a-64 of every Q15/Q31 coefficient table, both profiles, M = 2/3/6 — measured on this substrate and identical on the pre-3a substrate. MatchesNumpyReference (float, 3e-5) is unchanged.
• test_fir_kernels.cpp typed over the four formats. README FIR-substrate section: double row, the ladder constants, an fft_arith.h subsection, the quantize rule.

Verification

• cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DTAP_DSP_WERROR=ON && cmake --build build -j8 && ctest --test-dir build --output-on-failure: 189/189 pass, zero warnings (GCC 13.3, Linux). The touched TUs also compile clean under clang++ -std=c++20 -Wall -Wextra -Wpedantic -Wconversion -Wshadow -Werror.
• Bit identity: a throwaway harness (scratchpad, not in the repo) dumped the Q15 and Q31 decimate outputs for M = 2/3/6 (coefficients, the half-scale noise stream, and the full-scale step), plus every quantized row of the 160×32 prototype and the unity-DC Gaussian row for Q15 and Q31 — 7696 lines — before the change, after bcd742f, and after the review-fix commit. All three identical (md5 a4bb7a3f…). The in-tree form of this evidence is now Decimate.FixedPointTablesAreBitPinned. Every pre-existing test passes unchanged.
• clang-format --dry-run --Werror clean on every touched file (clang-format 18.1.3); the CI tidy job's exact loop is clean on the touched TUs.
• Not run here: the TAP_DSP_Q15_SMLALD path of fir_kernels.h (Arm-only). Read by eye: it uses only tr::mac and tr::finalize on an std::int64_t acc, whose signatures are unchanged (now constexpr), and the concept change only adds requirements the four specializations satisfy. Review A confirmed the same reading. CI's M55 leg compiles only tap_dsp_fft; the first real gate for that path is the Stage 1 QEMU legs (Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17) or a consumer's Arm build.
• The notebooks do not measure the substrate; nothing to re-execute.

Notes for the reviewer

• Contract change. The substrate's sample_type concept now requires k_coeff_scale (as const double), k_is_fixed_point (as const bool), coeff and accum. The trait surface consumers call (make_coeff, mac, finalize, silence, k_coeff_scale) is unchanged in signature and bit-identical; every member is now constexpr, which is additive. Consumers: SampleRateTap and RatioTap are the substrate's consumers and neither is on disk in this session, so their impact is not verified here. On bump: (1) delete or flip any static_assert(!sample_type<double>) carried from SampleRateTap's test_fixed_point.cpp (the lineage of the line that flips here) or anything else asserting double is not a sample format — that is a compile failure, not a behaviour change; (2) a non-derived own sample_traits specialization additionally needs k_is_fixed_point (a derived one inherits it; verified by review B's probe). quantize_row_preserving_sum changes behaviour for one input class: a row with a tap at or beyond the format's rail no longer wraps it and preserves the sum through the other taps. decimate (MuTap-side) sees no numeric change; its decimate_by_* aliases are untouched.
• Scoped spill-over, declared: CLAUDE.md:15 (sample_traits line lists double) and README.md:173 (decimate paragraph: double golden, float embedded) / README.md:229 (the "Five headers" intro now names the fft_arith trait with a forward note to the Stage 3b README rewrite). The process review established that no other wave-1 branch touches those lines.
• fft/fft_arith.h is documented under the FIR substrate until the Stage 3b README rewrite moves it to the FFT section's profiles table (stated in the README).
• Recorded follow-up (not in this PR): review A Add Claude Code web SessionStart hook (canonical Tap House setup) #4 — emit the decimate numpy reference as double alongside the float arrays so basic_decimator<double, M> can be pinned near 1e-12 instead of at the reference's float32 rounding; for the reference-script owner (tools/reference/make_frontend_reference.py). The numpy pin was frozen for this change, and the test's name says what it measures.
• Not a change to fft.h, ci.yml, or root CMake; tests/CMakeLists.txt gains one source line (test_fft_arith.cpp, alphabetical, after test_fft_backend.cpp).
• Merge position (review B fft: align the vDSP split buffers — root cause and fix for the MuTap #31 float32 flake #9): after Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17 (whose filter becomes negative per Part 13, so nothing is needed here) and before stage5; git merge-tree is clean against every other wave-1 branch. For the Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17 owner: with double now fir_kernels_test/0, that file's comment mapping /0,1,2 to float/Q15/Q31 is stale.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

[tap]

Hostile review B (process / downstream)

Reviewed bcd742f against origin/main (5ca3b1c), the plan (docs/audit-fft-and-code-smells.md Stage 3a / Part 7 / Part 12 / D1–D3), CLAUDE.md, STYLE.md and the PR template. Built in a private scratch dir with -DTAP_DSP_WERROR=ON (GCC 13.3): 187/187, zero warnings; clang-format --dry-run --Werror clean on all nine touched files; the three touched headers plus decimate.h compile clean under clang++ 18 -std=c++20 -Wall -Wextra -Wpedantic -Wconversion -Wshadow -Werror. CI: both runs on the head SHA green (linux/windows/macos/M55-compile/clang-tidy/drift).

Findings

1. [should-fix] Cross-PR: the re-typed decimate tests silently leave PR #17's on-target run, and the new FftArith battery never enters it.
Evidence: claude/wave1-stage1-legs:tests/bare_metal_main.cpp:36-54 bakes in a gtest filter that selects "Decimate.*:" and lists typed suites explicitly (yin_test/0.*, fir_kernels_test/*.*, ...). This PR converts ChunkingIsBitIdentical, DcGainIsUnity, GroupDelayIsHalfTheTaps into TYPED_TEST_SUITE(decimate_test, floating_types, ), whose gtest names are decimate_test/0.* and decimate_test/1.* — matched by neither Decimate.* (case and /) nor any other pattern. git merge-tree reports a clean auto-merge of the two branches, so after both land the three tests drop from all four QEMU legs with no red anywhere (the selected < 60 guard in that file cannot see a loss of three). FftArith.* is also absent from the filter, so the int32 shift/countl_zero/saturation contract the Q15/Q31 kernel is written against is never executed on the Cortex-M4 leg that is its reason to exist (Part 7 "Target notes"; Part 12 says the fixed-point battery runs on the M4 legs). Two smaller consequences of the same collision: the filter's comment fir_kernels_test/0,1,2 = float, int16 (Q15), int32 (Q31) becomes wrong (now /0 = double, /1 = float, /2 = Q15, /3 = Q31), and Decimate.DoubleMatchesNumpyReferenceToItsFloatRounding / Decimate.Q31TracksDoubleWithinTheFormatFloor run double-profile decimators on the M4 soft-float leg although every other typed suite there is filtered to /0 (float only). Host cost is 0.02 s and 0.01 s, so this is a convention issue, not a timeout risk (the leg has a 1800 s timeout; WidenNarrowRoundTripsEveryInt16 is integer-only and negligible).
Fix: this PR merges after #17 (see finding 9) and, in its rebase, adds "decimate_test/0.*:FftArith.*:" to the filter, corrects the fir_kernels_test comment, and either accepts the two double-profile Decimate.* tests on-target or renames them out of the Decimate. prefix (DecimateDouble.*) so the float-only convention holds. State the request to the #17 owner in the PR body now; whichever branch merges second carries the edit.

2. [should-fix] The contract-change note under-states what SampleRateTap must do.
The note correctly names SampleRateTap and RatioTap and correctly says a non-derived own specialization must add k_is_fixed_point. Verified with a probe (g++ -std=c++20, against this branch): a consumer template srt::sample_traits<T> : tap::dsp::sample_traits<T> inherits k_coeff_scale/k_is_fixed_point with decltype(...) still const double/const bool, so tap::dsp::sample_type<T> and a refined srt::sample_type<T> are satisfied for all four types; a new-type specialization written to the old concept (no k_is_fixed_point) is rejected by sample_type, i.e. it becomes a constraint failure at the first quantize_row_preserving_sum/kernel use. So the inheritance claim holds. What the note omits: tests/test_sample_traits.cpp is "ported and extended from SampleRateTap's test_fixed_point.cpp", and the pinned line that flips here (static_assert(!sample_type<double>)) was pinned by that lineage. If SampleRateTap's own battery still carries that assertion (or anything else that asserts double is not a sample format), its submodule bump fails to compile, which is a different action from "add k_is_fixed_point". MuTap: grep -rn 'sample_type<\|sample_traits<' MuTap MuTap-Max outside submodules/ finds nothing, and MuTap does not instantiate decimate yet (only the ML tooling mentions the word), so the MuTap statement in the note is correct.
Fix: add to "Notes for the reviewer": "SampleRateTap/RatioTap: on bump, delete or flip any static_assert(!sample_type<double>) carried from test_fixed_point.cpp; a non-derived specialization additionally needs k_is_fixed_point."

3. [should-fix] The bit-identity evidence lives in a scratch harness; make it a committed pin.
The PR's md5 a4bb7a3f… over a 7696-line dump is not reproducible from the repo. I reproduced the claim independently: a fresh harness dumping every Q15/Q31 decimate coefficient table (both profiles, M = 2/3/6), a half-scale noise stream and a full-scale step through each, plus every quantized row of a 160x32 prototype for both formats, compiled once against origin/main's headers and once against this branch — 37,976 lines, identical (cmp clean, same md5). So the claim is true, and nothing in-tree will notice if it stops being true: the only fixed-point decimate pins are tolerance pins (3.2e-4, 4.9e-9) and DcGainIsExactInQ15. A wave-2 or SMMULR/SMLALD backend change could move a coefficient LSB inside those tolerances unseen.
Fix: add TEST(Decimate, FixedPointTablesAreBitPinned) that checks, per (format, M, profile), the row sum and a 64-bit FNV-1a over the coefficient bytes. Values measured on this branch (identical on main): Q15 economy/transparent by 2: taps 81/259, sum 16384/16384, fnv 0x39cce82702646d63 / 0x22b3f3fe1f876217; by 3: 121/389, 0x4732917f54baba12 / 0x8f416893d8fc21c5; by 6: 239/773, 0x86138e3ad4ba8683 / 0x90a975140075933b. Q31 by 2: sum 1073741824 both, 0x3ef2130af3590f8c / 0x3ad417dfbf86e7d3; by 3: 0x8f433531606559b0 / 0xeb46f416372613dd; by 6: 0x1470212d03b35d6a / 0x89b4855f506a17e8. The exact sums (2^14, 2^30) are the "DC gain exactly 1" contract for both formats; today only Q15 by 3 is pinned.

4. [should-fix, before 3b starts] The fft_arith docstrings are not yet a complete kernel spec (plan Part 7: "written against this trait's docstring as its spec").
What is specified and pinned is good: formats in/out of every op, the one rounding in mul_coeff, saturation of add/sub/narrow, guard bits (2, via << 14), twiddle format (Q1.30, shared, 1.0 representable), headroom_bits definition including the empty/zero/rail cases. A 3b author still has to guess:

• (a) Complex twiddle multiply rounding count. y_r = x_r*w_r - x_i*w_i can be two mul_coeff roundings then a saturating sub, or one int64 accumulate then one >> 30 (what the header's own "SMMULR/SMMLAR is the seam" sentence implies). The trait only offers mul_coeff(sample, coeff) -> sample, which forces the two-rounding form; the Welch/Oppenheim–Weinstein noise numbers Part 9 pins differ between the forms, and switching to the Arm seam later would be a numeric change. Add a cmul/mul_coeff_sub-style op with one documented rounding per component, or state "two roundings per complex component" as the contract and pin it.
• (b) The Q31 fixed-scaling input pre-shift (Part 7: one bit, −6 dB). k_guard_bits = 0 says only "no spare width"; nothing says where the pre-shift lives, whether it is shr_round (rounded) or a plain shift, or whether BFP skips it. Add k_input_pre_shift per profile or say explicitly it is the kernel's number.
• (c) Block floating point has no left shift. Part 7's "shift only as much as growth requires" plus a right-only op set means quiet inputs never gain SNR; if intentional, say "BFP never normalizes upward" and define the clamp when headroom_bits (31 for a zero block) exceeds the stage's growth allowance; otherwise add shl_sat.
• (d) Twiddle generation source. make_coeff(double) of a libm cos/sin can differ by an LSB across hosts before the Q1.30 rounding (the audit itself flags host-identical doubles at line 413), which makes fixed-point outputs host-dependent by one twiddle LSB. State whether twiddles come from a checked-in table or a specified generator with a pinned checksum.
• (e) fft_sample_type does not require k_guard_bits, k_coeff_one, k_coeff_frac_bits, k_widen_shift, product, or noexcept; a generic kernel will use all of them. Either add them to the concept or say the concept is deliberately the minimum.
• (f) The relation between k_guard_bits, narrow's fixed 14-bit shift, and the exponent a BFP transform returns (Part 7: x * 2^-(e_fwd + e_inv)) is unstated.
  Fix: extend the header's contract list with the above (numbers, not prose) in this PR, or open the 3b-tests branch stacked on this one with those as its first red tests.

5. [should-fix] Stale "float golden" wording remains in three in-reach places.

• README.md:173 (decimate section): "sample_traits.h formats (float golden, Q15 / Q31 …)" now contradicts decimate.h:10,40-46, sample_traits.h, and CLAUDE.md:24. The PR body says this is outside the FIR-substrate ownership. Confirmed — and none of the other README-touching wave-1 branches goes near it (their hunks: capi-fft 308/318, docs-provenance 367/371, stage1-legs 329, stage1b-bench 332). So no wave-1 owner will fix it, and it is a direct consequence of this PR's contract change. Recommendation: this PR takes the one-line edit and declares it in the body as a scoped spill-over; if the wave lead refuses, it goes to the wave-4 "measured numbers into docs" item, and the PR body must say so instead of "left for the owner" (there is none).
• CLAUDE.md:15: "the sample-format traits (sample_traits.h: float/Q15/Q31)". CLAUDE.md is not a Part 12 contention file; fix it here.
• README.md:229: "Five headers carried from SampleRateTap" now heads six ### subsections, one of which (fft/fft_arith.h) is neither FIR nor from SampleRateTap. See 6.
  CLAUDE.md:24's golden-model sentence is consistent with decimate.h and sample_traits.h as changed; tools/capi/dsptap_capi.cpp:38 ("converts to the float golden model") is about the ratio converters' C ABI boundary and is out of scope, but note it for the capi owner.

6. [nit] fft_arith.h is documented under ## The FIR substrate. Part 8 puts the FFT profiles table in the README FFT section rewrite at 2b/3b. Either add one sentence ("moves to the FFT section at 3b") or leave a forward reference in the FFT section; otherwise the 2b owner will not know to move it.

7. [nit] tests/CMakeLists.txt: the new source is appended after test_yin.cpp in an alphabetical list. claude/wave1-stage5-spectrum appends test_spectrum.cpp at the same line; git merge-tree reports CONFLICT (content) there. Inserting test_fft_arith.cpp alphabetically (after test_fft_backend.cpp, eight lines away) makes the stage5 merge clean and matches the list's order. stage2a's additions are a separate block after gtest_discover_tests and merge clean either way.

8. [nit] Truthfulness and hygiene, checked. 187/187 matches the CI log (linux-ooura, "100% tests passed, 0 tests failed out of 187"); zero warnings under WERROR locally; numpy pin untouched (MatchesNumpyReference and k_reference_tolerance = 3e-5f appear only as context in the diff); the quantize.h behaviour change is reachable only for rows with a tap |c| >= 2.0, which no shipped design produces, and the new test pins it; decimate.h changes are comment-only. Ownership: touched files are exactly the Part 12 list (substrate headers, decimate.h docstring-only, new fft/ header, five test files, one CMake line, README FIR section hunks 245/273/290 all inside ## The FIR substrate); fir_kernels.h untouched. Commit: single commit, message states the contract and numbers, both trailers present; PR body follows the template's four sections and states the Arm-only TAP_DSP_Q15_SMLALD gap plainly. One inaccuracy: the body's "the same trait surface … is unchanged" should say "unchanged in signature and bit-identical; every member is now constexpr, which is additive".

9. Merge position. Part 12's left-to-right order is stage0 → stage1-legs (#17) → 1b → 2a → 3a → 5 → docs → capi. merge-tree from this branch: clean against capi-fft, docs-provenance, stage0, stage1-legs, stage1b-bench, stage2a; conflict only with stage5 (tests/CMakeLists.txt, finding 7). Both branches create include/tap/dsp/fft/ with different files — no conflict. Recommend: merge after #17 and #2a, before stage5, and rebase once after #17 lands to carry the filter edit from finding 1. Do not merge before #17: the filter fix would then land in the wrong PR.

Verdict

No blocker. Merge after #17 with findings 1, 2, 3, 5 and 7 addressed in the rebase; finding 4 must be closed before the 3b kernel author starts, in this PR or as the first commit of the stacked 3b-tests branch.

---

Generated by Claude Code

[tap]

Hostile review A (correctness / numerics)

Scope: PR #22 at bcd742f, diffed against origin/main, against plan Stage 3a / Part 7 / D1–D3 / N6, N13, N15. Rebuilt privately with -DTAP_DSP_WERROR=ON (GCC 13.3): 187/187 pass, zero warnings. Each touched header compiles standalone under clang++ -std=c++20 -Wall -Wextra -Wpedantic -Wconversion -Wsign-conversion -Wshadow -Werror. The CI clang-tidy loop (clang-tidy-18 -p build <TU>) is clean on the five touched TUs. All six GitHub checks green on both runs. Everything below was probed with throwaway programs, not read by eye.

Findings

1. should-fix — quantize.h: the rail bail-out abandons a representable row sum, and the docstring's justification is wrong for |c| in [1.99997, 2.0)

The new loop picks the largest-remainder tap and breaks if that tap is at the rail. A tap that saturated in make_coeff has the largest positive remainder in the row by construction (scaled - 32767 ≥ 0.5), so for a positive residual it is always picked first, and the correction stops even when every other tap could absorb the step.

Probe (Q15): row {1.99997, 0.3, 0.3} → scaled 32767.5, 4915.2, 4915.2, exact_sum = 42597.908, llround = 42598, quantized {32767, 4915, 4915} = 42597. Sum not preserved, though taps 1 and 2 are 27852 LSB from any rail. Same shape in Q31 with {1.9999999995, 0.3, 0.3} (2791728742 wanted, 2791728741 produced). 1.99997 < 2.0, so the docstring's "a row whose design exceeds the format (|c| >= 2.0) had no representable DC gain to preserve" is false on two counts: saturation starts at c ≥ (32767 + 0.5)/16384 = 1.99997, and the row's sum here is representable. (Strictly better than origin/main, which wrapped 32767 → -32768 on this row; but the header still says "preserving the row's DC sum exactly" and the new paragraph mis-states the exception.)

Fix: pick the best remainder among taps that can still move in the step's direction (dst[u] != rail_for(step)), and break only when no such tap exists. Termination is unchanged (|residual| strictly decreases on every non-break iteration). Then the docstring can say honestly: the sum is preserved whenever at least one tap is off the rail in the needed direction; only a row every tap of which is pinned at that rail keeps its saturated sum. Add the near-rail row above as a test next to CorrectionNeverWrapsATapAtTheRail.

2. should-fix — fft_arith.h is the wave-2 kernel's spec and leaves the two things that spec must fix ambiguous: when the fixed-scaling shift happens, and what a BFP kernel may do with headroom_bits

(a) The Q15 docstring says "the two guard bits ... are what let worst-case radix-4 growth run under fixed scaling without an input-level contract (audit Part 7)", and the file header says "the trait only supplies the operations, never a policy". Part 7 is explicit: shift-before-butterfly, 2 bits per radix-4 stage. The claim is only true under that ordering. Probe with the trait's own ops on full-scale widened Q15 (fs = widen(32767) = 536854528) at a 45° twiddle: component after rotation √2·fs = 759226954; summing four of them before shifting gives 2147483647 — saturated (add clamps); shifting each by 2 first gives 759226956, in range with 2^1.5 to spare. A kernel author who reads only this header (which is what the header says they should do) can pick shift-after and pass every test in this PR. Fix: state in the widen bullet that the guard-bit budget assumes the kernel shifts each stage's inputs by 2 bits (1 for radix-2, 1 before the real post-pass) before the butterfly, and give the bound: magnitude never exceeds √2 × full scale, i.e. 2^29.5 in Q2.29, so 2^1.5 below the int32 rail.

(b) headroom_bits is correctly Arm CLS (probe: {INT32_MIN}→0, {-1}→31, {0}→31, {1}→30; sampled sweep of all int32 at stride 97 agrees with "largest s such that x<<s fits", 0 mismatches). But a block shifted left by its full headroom can land exactly on INT32_MIN: -(2^20) has headroom 11 and -(2^20) << 11 == INT32_MIN, after which sub(0, x) saturates to INT32_MAX and mul_coeff(x, -1.0) is the one documented saturating product. The docstring should say a BFP kernel must leave at least one bit of the reported headroom unused (Part 7 already says "shifts only as much as growth requires"; the trait should say why the full amount is never legal). Also state that {-1} reports 31, indistinguishable from an all-zero block, so 31 is not a "silent block" signal.

3. should-fix — the SMMULR "same contract" claim is not achievable bit-exactly as the contract is written

mul_coeff is (x·w + 2^29) >> 30 with w in Q1.30. SMMULR computes (x·w + 2^31) >> 32: its rounding point is bit 32 of the product, not bit 30, and its result is x·w/4 in the data's format. There is no bit-exact way to get the >> 30 result from it — shifting the twiddle up 2 bits leaves 1.0 unrepresentable (the reason Q1.30 was chosen over Q0.31 in Part 7), and shr_round(mul_coeff(x, w), 2) is a double rounding: measured over 10^7 random (x, w), 1249965 results differ from the single-rounded (p + 2^31) >> 32. The header (file comment and the mul_coeff bullet) says SMMULR "is the designated seam behind the same contract, as SMLALD is for the FIR kernels" — SMLALD is bit-exact for the FIR (exact int32 products, associative int64 sum); SMMULR is not for this op. Since the sentence is aimed at whoever writes the 3b kernel and the Arm seam, fix it now: either (i) drop the bit-exactness implication and say the SMMULR path is a separately pinned profile, or (ii) if fixed scaling wants the ×1/4 anyway, define a second named op with the hardware's rounding point (mul_coeff_q2: (x·w + 2^31) >> 32) and let the kernel choose it explicitly, so the portable and the Arm builds run the same arithmetic. Either way the current sentence is a promise the arithmetic cannot keep.

4. should-fix — DoubleMatchesNumpyReferenceToItsFloatRounding pins the golden model at the reference's resolution, not the golden model's

Reproduced: worst 5.746e-8 (economy, M=2), where the peak output is 1.126 and the float ulp there is 1.192e-7 — the number is exactly the reference's half-ulp, as the comment says. Per ratio: 5.7e-8, 5.1e-8, 2.6e-8, 3.0e-8, 2.9e-8, 1.5e-8. It is not a tautology (it bounds the double path to within one float ulp of numpy), but that is the same information the existing float pin carries, and it is blind to any double-path drift below ~6e-8, three-plus orders above where a double-vs-float64 comparison would sit. The reference generator (tools/reference/make_frontend_reference.py) already emits k_mel_log as std::array<double, …>; the decimate arrays are the only float32 ones. Follow-up (the plan froze "the numpy pin does not move" for this PR, so not a blocker here): emit the decimate reference as double alongside the float arrays and pin basic_decimator<double, M> at ~1e-12; keep the float pin as is. Until then, the test name should not suggest it certifies double.

5. nit — sample_traits.h header static_asserts claim finalize(accum{}) == silence() but compare to literals

The concept comment says "the static_asserts below pin ... finalize(accum{}) is silence()"; the asserts compare against 0.0, 0.0f, 0. Compare against sample_traits<T>::silence() so the assert pins what the comment says. (0.0f == 0.0f in a constant expression is fine, as asked.)

6. nit — shr_round has an undocumented UB edge

Docstring: "the result always fits for 0 <= bits <= 31". Behaviour for bits ≥ 63 is UB (int64{1} << 62 is the last legal rounding term; >> 64 is UB). Probe: bits = 32, 33 behave (return 0). Add @pre 0 <= bits <= 62 (or <= 31, matching the sentence) so the kernel's shift-amount bookkeeping has a stated bound. Negative bits is silently the identity in the int32 trait and in the floating trait; say so or @pre bits >= 0.

7. nit — fft_sample_type does not require the constants the kernel will read

k_guard_bits, k_coeff_one, k_coeff_frac_bits exist on all four specializations but are not in the concept; k_widen_shift / k_sample_bits exist on one each. A 3b kernel written profile-generically will touch k_guard_bits and k_coeff_one. Add them to the concept now so a fifth profile cannot satisfy it without them.

8. nit — docs vs code, small

• decimate.h contract bullet: "the Q15 profile is pinned against double as a measured number" — Q31 is too now (Q31TracksDoubleWithinTheFormatFloor).
• README.md:173 (decimate section) still says "float golden"; the PR body defers this to the section owner — flagging so it does not get lost.
• README Q31 row omits "round-half-up" where the Q15 row states it; the code is identical.
• tests/CMakeLists.txt: test_fft_arith.cpp appended after test_yin.cpp in an otherwise alphabetical list.
• quantize.h docstring: "|c| >= 2.0" → "|c| ≥ 2 − 2^-15 (Q1.14) / 2 − 2^-31 (Q1.30)", per finding 1.

Verified, no finding

• Bit identity vs origin/main (old header compiled into old_tap::dsp): make_coeff over 2×10^7 uniform doubles in ±2.5 plus 22 edge values incl. 32767.5/16384, −32768.5/16384, 2^-31, ±1e300, denorm_min; Q15 mac over all 65536 × 7 coefficients with random int64 accumulators; Q31 mac and both finalizes over 5×10^7 random int64/int32 plus the 2^13 rounding boundaries and INT64_MIN/MAX. 0 mismatches in 70,458,829 cases. k_coeff_scale == 16384.0 / 1073741824.0 exactly.
• mul_coeff rounding rule. C++20 [expr.shift]/3 makes >> on a negative int64 arithmetic (P0907R4), so the op is floor((p + 2^29)/2^30) = round half toward +∞, identical to finalize's (acc + 2^13) >> 14. Probe: mul_coeff(-1, 0.5) = 0, (-3, 0.5) = -1, (3, 0.5) = 2; the tests pin the same.
• Saturation set. Exhaustive over all 2^32 x for w ∈ {±2^30, ±(2^30−1)}: exactly one saturating pair, (INT32_MIN, −2^30). Analytic: |p| ≤ 2^61, the +2^29 cannot overflow int64, and the only way (p + 2^29) >> 30 leaves int32 is p = +2^61.
• shr_round(INT32_MIN, 1) = -2^30, (INT32_MIN, 31) = -1, (INT32_MAX, 31) = 1 — the rounding add is in int64, no overflow.
• widen << 14 on negative int32 is defined in C++20; every leg compiles as C++20 via target_compile_features(cxx_std_20) on the INTERFACE target (MSVC gets /std:c++20, and MSVC has always shifted two's-complement). narrow saturates the value that rounds up to 2^15 ((32767 << 14) + 2^13 → 32767), and the all-65536 round trip is present and passing.
• Constexpr-evaluability: the header and test static_asserts compile (mul_coeff, add, shr_round, narrow(widen(·)), headroom_bits over a constexpr std::array, make_coeff(1.0) == k_coeff_one).
• round_sat(NaN): still UB via static_cast<I>(NaN) (both comparisons false). Pre-existing, unchanged, noted only.
• fir_kernels.h SMLALD path: uses tr::mac(std::int64_t, S, coeff) and tr::finalize(std::int64_t), both signature-identical; the concept only adds requirements the four specializations meet. Could not execute (no Arm libc here), agreed with the PR body.
• decimate.h over double: nothing float-specific — k_block, memmove(... sizeof(S)), silence(), dot_row<S> are generic; the !k_is_fixed_point branch of quantize is a plain make_coeff identity for double; coefficients() returns span<const double>. GroupDelayIsHalfTheTaps's EXPECT_EQ(y[peak], h[centre]) is exact for both floating profiles (single nonzero product, no rounding in finalize).
• Re-pinned numbers reproduce: Q15-vs-double 1.5745e-4 (pin 3.2e-4), Q31-vs-double 2.4506e-9 (pin 4.9e-9).
• Style: three-line banner, @pre, k_ constants, snake_case concept, braces everywhere; [[nodiscard]] is not used anywhere in include/, so no house convention to apply.

Verdict

No blockers; approve after the should-fixes (1–3 are docstring/spec-level plus a five-line loop change; 4 can be a follow-up PR). The arithmetic that ships is correct and bit-identical to main; what needs tightening is the spec text the 3b kernel will be written against.

---

Generated by Claude Code

[tap]

Fix pass for both hostile reviews — e3b9d7f

189/189 under -DTAP_DSP_WERROR=ON, zero warnings; clang-format and the CI tidy loop clean; the pre-change Q15/Q31 dump is still byte-identical (md5 a4bb7a3f…). Per finding:

Review A (correctness / numerics)

1. Rail bail-out — applied. The step now goes to the largest remainder among taps not at the rail in the step's direction; the loop breaks only when no such tap exists. Docstring threshold corrected to 2 − 2^-15 (Q1.14) / 2 − 2^-31 (Q1.30), and the cost stated (one pass per residual LSB; a tap saturated by k LSB adds k). New Quantize.NearRailRowStillPreservesItsSum: {1.99997, 0.3, 0.3} → {32767, …} summing to 42598; Q31 {1.9999999995, 0.3, 0.3} → 2791728742. CorrectionNeverWrapsATapAtTheRail now uses small-overshoot rows (both rails, sum preserved) plus an all-taps-at-rail row for the break path.
2. Spec: shift timing and BFP headroom — applied, in the fft_arith.h file header and the widen/headroom_bits bullets: shift-before-butterfly (2 bits per radix-4 stage, 1 per radix-2, 1 before the real post-pass); under it the complex magnitude never grows, so the bound is √2·FS — 2^29.5 for widened Q15 (1.5 bits below the rail), and your shift-after 45° counterexample is named as not covered; a BFP kernel never consumes the full reported headroom (−2^20 → 11, << 11 = INT32_MIN, where sub(0, x) and mul_coeff(x, −1.0) saturate); {0} and {−1} both report 31 = "no information".
3. SMMULR — applied, option (i). The header now says SMMULR is not a bit-exact seam (rounding at bit 32 vs 30; twiddle up-shift loses 1.0; the double-rounding differs on ~1/8 of products) and that an SMMULR build would be a separately pinned Arm profile with its own noise numbers. No mul_coeff_q2 for now: one portable arithmetic stays the contract every host and QEMU leg runs bit-identically; a second op is worth defining only once an on-target measurement says the ×1/4 form pays for itself, and it would then be an explicit kernel choice, not a seam.
4. Double-vs-numpy pin — recorded follow-up, not changed here. The numpy pin was frozen for this PR; the test comment and the PR body now say it bounds double to one float ulp of numpy, no tighter, and that emitting the decimate reference as double (the generator already does for k_mel_log) is the reference-script owner's item.
5. static_asserts vs silence() — applied.
6. shr_round edge — applied. @pre 0 <= bits <= 31, asserted, in both the int32 and floating traits; bits == 0 is the identity.
7. Concept constants — applied. fft_sample_type now requires k_guard_bits, k_coeff_frac_bits, k_coeff_one (as const coeff), the new k_fixed_scaling_input_pre_shift, and noexcept on every operation.
8. Docs nits — applied: decimate.h says Q31 is pinned against double too; README:173 fixed; Q31 row says round-half-up; CMake entry alphabetical; quantize threshold wording.

Review B (process / downstream)

1. Cross-PR filter — nothing to add here (Part 13: Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17's filter becomes negative, so the typed decimate suites and FftArith.* enter the on-target run by default). Noted for the Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17 owner in the PR body: fir_kernels_test/0 is now double, so that file's /0,1,2 comment is stale.
2. Contract note — applied. PR body now says SampleRateTap/RatioTap must delete or flip any static_assert(!sample_type<double>) carried from test_fixed_point.cpp on bump, and that a non-derived specialization additionally needs k_is_fixed_point; wording "unchanged in signature and bit-identical; now constexpr (additive)".
3. Committed bit-pin — applied, Decimate.FixedPointTablesAreBitPinned: taps, row sum (16384 / 1073741824) and FNV-1a-64 per format × M × profile. Your sums and tap counts reproduce exactly; your hash values do not, and none of the folds I tried (native LE bytes, int64 LE bytes, big-endian bytes, word-wise, FNV-1 order) reproduce them, so the fold you used is not stated by the numbers alone. The test pins a fold that is stated in its code — byte-wise FNV-1a-64 over the coefficient bytes in memory order — and the values were measured on this branch and are identical when the harness is compiled against origin/main's headers, which is the property the pin exists for.
4. Spec completeness — applied for (a) rounding count (two per complex component; the fused variant named as non-bit-identical; no fused op by design), (b) k_fixed_scaling_input_pre_shift = 1 for Q31 / 0 for Q15 and floats, with the bound derivation, (c) BFP never normalizes upward (no left shift; right-shift by max(0, growth − headroom)), (d) twiddle source (double cos/sin, make_coeff; libm caveat; 3b pins the table checksum per certified N), (e) concept constants and noexcept. (f) the BFP exponent's relation to narrow's fixed shift is stated only implicitly (narrow is format-preserving, the exponent is the kernel's return value); it belongs with the kernel's scaling-policy contract in 3b.
5. Stale "float golden" — applied, as a declared scoped spill-over: README.md:173, CLAUDE.md:15, README.md:229 (the intro names the sixth header with a forward note). tools/capi/dsptap_capi.cpp:38 left to the capi owner as you suggest.
6. Forward note — applied (README: documented under the FIR substrate until the 3b README rewrite).
7. CMake order — applied (after test_fft_backend.cpp).
8. Wording — applied.
9. Merge position — agreed: after Run the test battery bare-metal under QEMU on Cortex-M4, M33 and M55 #17, before stage5.

🤖 Generated with Claude Code

https://claude.ai/code/session_019ZPTzNxo5Fe4EtpXXKf7Sy

---

Generated by Claude Code