fix(pipeline): reject estimator keywords instead of discarding them - #23
Merged
Conversation
`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.
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.
estimate_spectrumfiltered the caller's**kwargsto what the selectedestimator 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_noisebelongs tocompare, so it was dropped here and the run silently kept the configuredvalue. 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 spellingthat works:
Four tests: an unknown keyword raises, a
comparekeyword raises with theredirect, 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.