Skip to content

fix(pipeline): reject estimator keywords instead of discarding them - #23

Merged
sgjholt merged 3 commits into
mainfrom
claude/pipeline-kwargs-and-binned-noise
Aug 7, 2026
Merged

fix(pipeline): reject estimator keywords instead of discarding them#23
sgjholt merged 3 commits into
mainfrom
claude/pipeline-kwargs-and-binned-noise

Conversation

@sgjholt

@sgjholt sgjholt commented Aug 7, 2026

Copy link
Copy Markdown
Owner

estimate_spectrum filtered the caller's **kwargs to what the selected
estimator accepts and dropped the rest, silently. A typo did nothing. An
argument meant for a different stage did nothing.

Filtering the configuration that way is correct and stays: [transform]
holds every estimator's settings at once, so a CWT parameter is not an error
when the FFT is running. The caller's own keywords are a different thing —
they were written deliberately, and one the estimator does not recognise is a
mistake rather than a setting for someone else.

This is not hypothetical. spectrum_set_from_streams(rotate_noise=False)
reads exactly as though it disables the noise lift; rotate_noise belongs to
compare, so it was dropped here and the run silently kept the configured
value. I took three measurements against what I believed was a modified
pipeline and was not, and concluded from them that a documented explanation
of a known inconsistency was wrong. It was not — the recorded settings on the
pair are what gave it away.

The error names the offending keywords, lists what the estimator does take,
and — when the keyword is one of compare's — says so and shows the spelling
that works:

fft does not accept rotate_noise. It takes drop_dc, n_fft, name, taper,
taper_alpha, taper_correction. rotate_noise configures the signal-to-noise
comparison, not the transform — pass it as compare={'rotate_noise': ...}.

Four tests: an unknown keyword raises, a compare keyword raises with the
redirect, the message names the valid parameters, and a keyword the estimator
does take still changes the answer — the passthrough is the point of the
parameter and rejecting unknown keys must not break it.

claude added 3 commits August 7, 2026 21:07
`estimate_spectrum` filtered the caller's `**kwargs` to what the selected
estimator accepts and dropped the rest, silently. A typo did nothing. An
argument meant for a different stage did nothing.

Filtering the *configuration* that way is correct and stays: `[transform]`
holds every estimator's settings at once, so a CWT parameter is not an error
when the FFT is running. The caller's own keywords are a different thing —
they were written deliberately, and one the estimator does not recognise is a
mistake rather than a setting for someone else.

This is not hypothetical. `spectrum_set_from_streams(rotate_noise=False)`
reads exactly as though it disables the noise lift; `rotate_noise` belongs to
`compare`, so it was dropped here and the run silently kept the configured
value. I took three measurements against what I believed was a modified
pipeline and was not, and concluded from them that a documented explanation
of a known inconsistency was wrong. It was not — the recorded settings on the
pair are what gave it away.

The error names the offending keywords, lists what the estimator does take,
and — when the keyword is one of `compare`'s — says so and shows the spelling
that works:

    fft does not accept rotate_noise. It takes drop_dc, n_fft, name, taper,
    taper_alpha, taper_correction. rotate_noise configures the signal-to-noise
    comparison, not the transform — pass it as compare={'rotate_noise': ...}.

Four tests: an unknown keyword raises, a `compare` keyword raises with the
redirect, the message names the valid parameters, and a keyword the estimator
*does* take still changes the answer — the passthrough is the point of the
parameter and rejecting unknown keys must not break it.
`compare` applied the boost lift twice, by two routes that do not agree: it
multiplied the *bins* by the factor, and separately multiplied the unbinned
array by that factor interpolated onto the fine axis. A bin holds the
geometric mean of `log10(amp)`, so binning the lifted noise gives
`mean(log a) + mean(log f)` while lifting the bin gives
`mean(log a) + log f(centre)` — equal only where the factor is flat across
the bin.

So a stored pair's `binned_noise` was not the binning of its own `noise`, by
up to 18.8% on the PNR windows. **Every pair was born inconsistent.** The
lift is now applied to the unbinned noise and the binned noise derived from
it: one source of truth, and the domain round trip becomes exact.

This was filed as a `to_motion` defect, and that was wrong in an instructive
way. A domain change re-bins, so `to_motion` silently *repaired* the pair —
it was the only operation that made `binned_noise` agree with `noise`, and it
therefore looked like the thing that broke it. The old test asserted that
appearance and is replaced by two: the round trip is exact, and a pair is
consistent with itself at construction. The second is the property that was
actually missing, and asserting it directly is what stops the diagnosis
landing on the wrong function again.

What it costs, measured rather than argued: no bin gains or loses a sample.
Bin centres, bin counts and the binned *signal* are bit-identical; only the
value representing each bin moves, because the lift is now averaged across
the bin along with the amplitude instead of being applied once at the centre.
Close to symmetric — 987 bins down, 1064 up, median ratio 1.000000 — with a
slight net rise in noise (geometric mean 1.00068) and so a slight net fall in
signal-to-noise (0.99932). Median |change| is 0.08-0.37% per decade. The
large excursions are rare and at the extremes: 11.7% around 1-5 Hz, where a
bin holds one or two samples and the factor at the sample is not the factor
at the bin centre, and 18.8% above 60 Hz, where a bin spans enough absolute
frequency for the factor to vary across it. Both sit outside every selected
band: **0 of 28 bands move and no fitted parameter moves at all** (0.0000%).

`bsnr` in the golden reference is regenerated — the first deliberate
numerical divergence from the legacy record in this refactor. The superseded
values are kept beside it as `bsnr_legacy`, and a new test pins the size of
the divergence at under 5% and asserts the bin counts are unchanged, so "one
deliberate correction" cannot quietly become two. `noise_amp`, `amp`, `band`,
`freq`, `resolution_floor` and `_environment` are untouched legacy values,
verified field by field before committing.
`compare` computed the floor as `max(signal.freq.min(), noise.freq.min())`.
That works exactly once. The noise is interpolated onto the signal's axis
before binning, so from then on `noise.freq.min()` *is* the signal's lowest
frequency and the noise's own is gone. A converted pair therefore inherited
the longer signal window's floor in place of the shorter noise window's.

The consequence is not cosmetic. The floor is what keeps the band out of the
region below the noise's resolution, where `interpolate_onto` repeats an edge
value rather than reporting a measurement — as its own docstring warns, a
ratio computed there has an invented denominator. A converted pair could open
its band into fabricated noise.

`spectrum_from_trace` has recorded `meta["resolution_floor"]` on every
spectrum all along, `aligned_noise` carries that meta, and `Spectrum.to_motion`
preserves it through a conversion. The value was tracked from the start;
nothing read it. Reading it is the whole fix — a `_resolution_floor` helper
that prefers `meta` and falls back to the axis only for a spectrum built by
hand, which is the one case where the axis is still the truth.

Measured across all five estimators:

- the floor differed between a pair and its own conversion on **28 of 28**
  stations; it now differs on none
- bands of *unconverted* pairs move on **0 of 28** for every estimator, so
  nothing computed without a domain change is affected
- round-trip band stability goes from 3/28 (fft), 8/28 (multitaper), 27/28
  (cwt), 7/28 (welch), 8/28 (quadratic) to **0/28 everywhere**

`band` in the recorded displacement set is regenerated, with the superseded
edges kept as `band_legacy`. It touches 3 of 28 stations, only the lower edge,
and only upward. That direction is asserted rather than observed: a floor that
was too low permitted an edge that was too low, so correcting it can raise an
edge and can never lower one, and the upper edge cannot move at all. A test
pins all three properties, so a future change that moves a band the other way
fails rather than being absorbed.

`amp`, `noise_amp`, `bsnr`, `motion` and `_environment` in that file remain
untouched legacy values, verified field by field before committing.

Also adds the invariant on its own: a domain change multiplies both spectra by
a power of `2*pi*f` and lengthens neither window, so it cannot change what
either can resolve.
@sgjholt
sgjholt merged commit 55cfbb1 into main Aug 7, 2026
10 checks passed
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.

2 participants