Repository navigation
feat(config)!: default to FFT with Konno-Ohmachi smoothing - #51
Merged
Merged
Conversation
…e goldens
REFACTOR_PLAN §6.6 records a claim whose mechanism was absent:
"Regression tests pin an explicit config file, never the defaults, so
changing a default cannot silently move a golden test."
Nothing pinned anything. `load_config()` takes a `project_file`, but the
pipeline does not call it that way: `spectrum_set_from_streams` and most of
what it calls read settings from functions that accept no configuration
argument, so there was nowhere to thread one through. The golden references
were therefore captured under whatever the shipped defaults were, and moving a
default moved the committed numbers — measured, bumping `smoothing.n_bins` from
151 to 158 broke 9 of 25, and changing the estimator and smoother defaults
breaks 11. A deliberate change and an accidental regression were the same
failure, wearing the same message.
`config.using()` is the mechanism. A `ContextVar` rather than a module global,
so a pin does not leak between threads or concurrent tasks and the reset token
restores exactly the previous one however the block exits. Only an
argument-free `load_config()` consults it: an explicit `project_file=` or
override still wins, because a study config read inside a pinned block must
still be the file it asked for. The local file and the environment are excluded
inside a pin, since a pin a developer's shell could move would not be one.
`load_config`'s `start` becomes positional-only, so a section named `start` in
`**overrides` cannot collide with it. No caller passed it by keyword.
`tests/golden/reference.toml` is the pinned configuration: every setting, not
an overlay. An overlay would still inherit the shipped defaults for whatever it
omitted, so a later default change to an unlisted key would move the numbers
again — the same failure by a narrower path. It is generated from the stamp the
references already carried, and round-trips to the same config hash,
7f602da31774.
`tools/make_golden.py` captures inside the pin and the golden suite compares
inside it. Verified by regenerating: every number reproduces to machine epsilon
— 272 of 273 changed leaves at <= 1.4e-15, all BLAS-level — the exception being
the known-unstable cwt band edge at `UR.AQ07.00.HHN`, which predates this
change. The committed files are unchanged.
`to_toml` no longer emits trailing whitespace, on a header's blank lines or
after a null placeholder's `=`. It writes files that get committed, and the
repository strips trailing whitespace on commit, which left a frozen file
differing from what the function would write for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REFACTOR_PLAN §8.1's open decision, resolved on the measurement it asked for.
Against the shipped multitaper + `log_bins`, on the 28 PNR windows with each
station fitted freely:
fft + log_bins median dMw -0.039 per-station -0.85 to +1.90
fft + konno_ohmachi median dMw +0.027 per-station -0.52 to +0.51
The smoother earns the change, not the transform. `log_bins` reduces the
frequency axis without reducing the variance — 151 bins over a few hundred
Fourier samples is about one sample per bin — so an FFT read through it swings
a station by up to 1.9 magnitude units. Pairing it with a real smoother halves
the worst excursion and pulls the `fc` range in by a third. The median station
barely moves either way, which is why the per-station spread is the argument.
What this event cannot settle, and what the decision does not rest on: its
event corner is unconstrained under all three (stage-one spread 895% to 1042%
of the event value). On the ensemble, PNR's Mw moves -0.1615 under both
constant sets — a pure Omega shift, the constants being an offset applied
after. Against the catalogue's 2.9 the two sets move in opposite directions,
the default constants from 0.194 to 0.032 away and §4.7's from 0.160 to 0.321,
so they no longer both sit within 0.25. Recorded, not offered as evidence.
`!` because every result moves for any configuration that did not pin both
settings, while nothing is renamed and no signature changes: the break is in
the numbers, not in the call. `docs/upgrading.md` says what to pin to keep the
old ones.
A coupling the measurement did not predict: `[fitting] fit_bins` fits the
binned spectrum, and Konno-Ohmachi preserves the frequency axis, so under the
new default the binned axis *is* the Fourier axis and the setting selects the
same 98 points either way. It bites only under an axis-reducing smoother.
The tests split the way the change does. Those asserting the pipeline's
behaviour or a recorded measurement — the golden comparisons, the displacement
reference, §4.7's magnitudes, the selection patterns, the `[fitting]` settings
— now run under `tests/golden/reference.toml`, so they keep testing their
subject rather than whichever pair ships. Those asserting what a user gets with
no configuration moved, and `test_the_shipped_default_moves_omega_by_a_constant`
is new: without it the default's own answer would be asserted nowhere.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… choice The smoothed spectra set the signal-to-noise ratio and the band. With `[fitting] fit_bins = false` — the default in 0.1.1 and since — the fit and the initial guess both read the *unsmoothed* amplitudes inside that band, so the smoother reaches `Omega` only through the band. 1081364's text said otherwise: that Konno-Ohmachi removes the FFT's variance from the fit, and that `fit_bins` is inert under it. Neither holds. Under Konno-Ohmachi `fit_bins` selects the same frequencies and switches the amplitudes, which differ by a median 0.10 dex on the first PNR channel. The comparison behind the default is re-measured on the path the pipeline and the tutorial actually use — velocity, from which `llpsp` is the displacement plateau — against multitaper + `log_bins`, stage one, 28 channels: fft + log_bins median dMw -0.079 per-station -0.71 to +0.23 band 1.55 dec fft + konno_ohmachi median dMw -0.068 per-station -0.25 to +0.19 band 1.68 dec ... with fit_bins=true median dMw -0.073 per-station -0.25 to +0.15 The two default-path FFT rows fit identical amplitudes, so their difference is the band alone. The earlier table was measured on displacement (-0.85/+0.70 and -0.56/+0.52 there): same ordering, wider spread. The tutorial gains "What the fit reads", which states the default and fits the event both ways, computed on every build: on PNR 93% of the unsmoothed points sit in the band's top decade, which is what `weight_method = "log"` exists to shift. It also stops saying the shipped default is multitaper, and says the comparison spectra are smoothed rather than binned. Edited in its builder, as `test_docs_are_current` requires. `fit_bins` now has a comment on the field itself, the corrected upgrading, roadmap and §8.1 text carries the velocity numbers, and a test pins what the setting does under Konno-Ohmachi: same frequencies, different amplitudes, the default reading the unsmoothed ones. Co-Authored-By: Claude Opus 5.5 <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
2 participants
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.
Important
Merge with a merge commit, not a squash. Three commits that want separate review and separate revert: the pin is infrastructure, the default change moves published numbers, and the docs commit says what the fit reads. A squash fuses them and the
!bump rides on one subject.REFACTOR_PLAN§8.1's open decision, resolved. Every result moves for any config that did not pin both settings — nothing is renamed, no signature changes.What each setting reaches
The estimator sets the spectrum that is fitted. The smoother sets the band: the signal-to-noise ratio and the gate are computed on smoothed spectra. With
[fitting] fit_bins = false— the default in 0.1.1 and since — the fit and the initial guess read the unsmoothed amplitudes inside that band, so the smoother reachesOmegaonly through the band.That is a modelling choice rather than a detail, so the tutorial now says so and fits the event both ways (What the fit reads).
The measurement
On the 28 PNR channels, each fitted independently on velocity — the motion the pipeline fits by default, from which
llpspis the displacement plateau — against the shippedmultitaper+log_bins:fcratio, median (range)multitaper+log_binsfft+log_binsfft+konno_ohmachifft+konno_ohmachi,fit_bins = trueThe two default-path FFT rows fit identical amplitudes, so the whole difference between them is the band: Konno–Ohmachi selects a wider one.
What I am not claiming. This event's corner is unconstrained under every pair (stage-one spread 895–1042% of the event value), so the event-level number is not evidence. For the record, since the suite asserts it: PNR's event Mw moves −0.1615 under both constant sets, and against the catalogue's 2.9 the default constants move from 0.194 to 0.032 away while §4.7's move from 0.160 to 0.321 — so the pair no longer both sit within 0.25.
Commit 1 — the prerequisite (
71a4e9c)Flipping the defaults broke 21 tests, 11 of them golden. §6.6 already recorded why, as a claim whose mechanism was absent:
Nothing pinned anything — the pipeline reads
load_config()from functions that take no config argument.config.using()closes it: aContextVar, explicitproject_file=still wins, local file and environment excluded inside a pin.tests/golden/reference.tomlholds every setting, not an overlay.Verified by regenerating the references under the pin: 272 of 273 changed leaves at ≤1.4e-15, the exception being the known-unstable cwt band edge at
UR.AQ07.00.HHN. Committed files unchanged. With the pin, the same default flip breaks 11 → 0 golden tests.Commit 2 — the default (
1081364)Tests asserting pipeline behaviour or a recorded measurement now run under the reference config; only those whose subject is the default moved.
test_the_shipped_default_moves_omega_by_a_constantis new, so the default's own answer is asserted somewhere.Commit 3 — what the fit reads (
33975ed)[fitting] weight_method = "log"exists to shift. Also stops saying the shipped default is multitaper. Edited in its builder.fit_binsgets a comment on the field itself.fit_binsdoes under Konno–Ohmachi: same frequencies, different amplitudes, the default reading the unsmoothed ones.upgrading.md, the roadmap and §8.1 carry the velocity numbers above and the mechanism as stated here. 1081364's commit message describes both incorrectly; this commit's message records the correction.Verification
🤖 Generated with Claude Code