Pad the clutter convolution to a measured length - #64
Merged
Merged
Conversation
WienerHopf planned its three convolution transforms at nBins + nSamples + 1. At
the shipped geometry (fs 2e6, cpi 0.5, clutter delay -10..400) that is
1000411 = 269 x 3719, both prime, so FFTW falls back to Rader instead of a fast
codelet and the transform costs about three times what it should.
The length only has to reach the alias-free linear-convolution length
(nSamples + nBins - 1), and only the first nSamples outputs are read, so
rounding up is free. It now rounds up to 1016064 = 2^8 * 3^4 * 7^2.
That number is measured, not derived, and the distinction matters. A formula
was tried first and was worse than doing nothing: taking the smallest
admissible length gives 1002375 = 3^6 * 5^3 * 11, which measured 1.40x FASTER
on x86 and 1.54x SLOWER on ARM at four threads. Transform cost is a sawtooth in
N and depends on the thread count and the codelets the target's FFTW actually
ships, so the only honest way to choose is to measure the target.
Every admissible length within 2% of the minimum was timed, 2 forward plus 1
backward as the filter runs, against the image's own NEON FFTW. 1016064 came
first of fifteen on four Pi 5 boards, across both memory variants, at every
thread count from one to four, idle and under load:
threads unpadded 1016064 ratio
1 463.25ms 137.44ms 3.37x
2 304.66ms 87.31ms 3.49x
3 259.44ms 84.13ms 3.08x
4 254.73ms 92.39ms 2.76x
Best and median agreed throughout, and a second board under live radar load
reproduced rank 1 at 2.25x to 3.82x. Measured end to end on a node, the clutter
filter went 762.2 ms to 504.9 ms a CPI (Welch t = -54.3, p10-p90 bands
separated).
clutterFftLength() applies the constant only where it covers the geometry
without being wastefully larger, inside the same 2% band the candidates came
from. Any other geometry falls back to the unpadded length, which is always
alias-free and is exactly what this code did before. So changing cpi or the
delay span quietly stops using the constant rather than aliasing.
Output differs only by floating-point reassociation, about 6e-14 relative or
264 dB down, far beneath thermal noise. testClutterFft validates the filtered
output against an independent direct time-domain convolution to 1e-9 rather
than against another FFT, across nine geometries.
Also fixes a latent bug the test caught: `i - delayMin` promoted to unsigned,
so a positive delayMin wrapped at 2^32 and read the wrong sample. Negative and
zero cancelled exactly, so the shipped -10 was unaffected and it stayed hidden.
Ported from mickeyslaven/blah2-VectorWarp de45d9b (MIT), though only the idea
of padding survives: their length rule is the one measured as a regression here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
WienerHopfplanned its three convolution transforms atnBins + nSamples + 1. At the shipped geometry that is 1,000,411 = 269 x 3719, both prime, so FFTW falls back to Rader and the transform costs about three times what it should.The length only has to reach the alias-free convolution length, and only the first
nSamplesoutputs are read, so rounding up is free. It now rounds up to 1,016,064 = 2^8 x 3^4 x 7^2.Live A-B-A on three nodes
Stock v0.4.3 -> branch -> stock, 240 s measurement window per phase, throughput from
nCpiadvance rather than polling (a tight poll loop costs ~200 ms/CPI on a saturated node):Two independent nodes agreeing to within 0.3% on the ratio, with baseline drift under 1% on both. Both returned to v0.4.3 at restarts=0.
The number is measured, not derived, and that matters
A formula was tried first and was worse than doing nothing. Taking the smallest admissible length gives 1,002,375 = 3^6 x 5^3 x 11, which measured 1.40x faster on x86 and 1.54x slower on ARM at four threads. Transform cost is a sawtooth in N and depends on the thread count and on which codelets the target's FFTW actually ships, so the only honest way to choose is to measure the target.
Every admissible length within 2% of the minimum was timed, 2 forward plus 1 backward as the filter runs, against the image's own NEON FFTW. 1,016,064 came first of fifteen on four Pi 5 boards, both memory variants, at every thread count, idle and under load:
Best and median agreed throughout. A second board under live radar load reproduced rank 1 at 2.25x to 3.82x.
The guard
clutterFftLength()uses the constant only where it covers the geometry without being wastefully larger, inside the same 2% band the candidates came from. Anything else falls back to the unpadded length, which is always alias-free and is exactly what this code did before.At
cpi0.5 that covers clutterdelayMaxfrom 200 up to 16,053, and the cost is identical across that whole range because transform cost is a property of the length, not of the minimum it covers. Doppler span does not enter this calculation at all:WienerHopfnever sees it.Outside that, for example a different
cpi, it quietly stops padding rather than aliasing.Verification
Output differs only by floating-point reassociation, about 6e-14 relative or 264 dB down, far beneath thermal noise.
testClutterFftvalidates the filtered output against an independent direct time-domain convolution to 1e-9, not against another FFT, across nine geometries, plus the guard boundaries and rejection of unrepresentable geometry. It runs in CI as of #63.Also fixes a latent bug
i - delayMinpromoted to unsigned, so a positivedelayMinwrapped at 2^32 and read the wrong sample. Negative and zero cancelled exactly, so the shipped -10 was unaffected and it stayed hidden. Two lines, and the test covers it. Say the word if you would rather it were split out.Provenance
The idea of padding comes from
mickeyslaven/blah2-VectorWarpde45d9b (MIT). Only the idea survives: their length rule is the one measured as a regression here.🤖 Generated with Claude Code