Per-source rolling holdout instead of a fixed 2025 cutoff #14
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/rolling-per-source-holdout"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
HOLDOUT_CUTOFF = 2025-01-01(scripts/recalibrate.py:157) was passed as the fit cutoff to all three correlation sources. PSKR data starts 2026-05-04 and beacon measurements are newer still, so both sources selected zero rows and could never be refit (the 2026-08-17 report records "PSKR bands [], 0 samples" and "Beacon 0 bands"). (tmp/bugs.md§A, P1.)ROLLING_HOLDOUT_WEEKSweeks (default 8; override with--holdout-weeks Nor$PROP_HOLDOUT_WEEKS), resolved against a singlenowper run so every source agrees on one clock.before=None) instead of silently fitting nothing; the JSON recordsholdout_start: nullplus anote, and the report emits a warning bullet.MIN(...)probes reuse each correlation query's own row filters, so a resolved cutoff cannot be defeated by rows the fit would discard.resolve_holdout(first_obs, now, weeks); the invariantfit_cutoff > source_first_obsis asserted, so a holdout can never empty a fit window.now/window length are logged and recorded underdata_sourcesin both the JSON and the Markdown report.Verification
python3 -m py_compile scripts/recalibrate.pyclean.psycopg, since it is not installed here) against synthetic inputs: 8-week cutoff for a source starting 2026-05-04 resolves to 2026-07-25 with a non-empty fit window; a source starting 2026-09-01 falls back to full corpus with a warning; the boundary casefirst_obs == cutofffalls back;--holdout-weeks 4and$PROP_HOLDOUT_WEEKSmove the cutoff; negative weeks rejected; every applied cutoff is strictly after the source's first observation; the loaders receive per-source values; the report and JSON carry the resolved window.before=call site is per-source.Known follow-up (not in this PR)
validate_before_write/run_validationstill shell out toscripts/validate_algo.py, which keeps its own fixed 2025-01-01 split, so the gate's test set can overlap this rolling fit window until that file moves too. Owned byscripts/validate_algo.py; nothing inrecalibrate.pyneeds to change when it does.Run against prod in the review follow-up below (
9a98ae85): the resolvedwindow plus a full
--dry-run.Review follow-up (
9a98ae85)Rebased onto
main(#12 is in).The naive-timestamp probe was fatal, and it reproduced. All three columns are Ecto
:utc_datetime→timestamp without time zone, so psycopg hands back naive datetimes whilecutoffis always aware. Against prod, before the fix:_first_observation/2now re-tags a naive result as UTC — exact, since the application writes UTC into those columns — and the docstring no longer claims psycopg returns aware datetimes fortimestamptz.Verification against prod (this revision)
plan_holdoutsresolves: pskr → fit before 2026-07-25, contacts → fit before 2026-07-25 (8 weeks /--holdout-weeks 8), beacon_measurements → full corpus + the "first observation 2026-08-10 is at/after the cutoff" note, exactly the documented fallback.--dry-runcompletes end to end in 202 s: 14 bands merged (3 PSKR, 11 contacts, 0 beacon) — where the fixed 2025-01-01 cutoff recorded "PSKR bands [], 0 samples". The PR's premise now shows up in the output rather than in the report text.python3 -m py_compile scripts/recalibrate.pyclean.validate_before_writestill shells out tovalidate_algo.pywith its own 2025-01-01 split; that is #13's file and is noted there.Second review round (
beff8b3a)Both new findings (619/6451 and 620/6456) are the same defect, and both were right: the probes reused the correlation queries' scalar filters but not their joins, so
first_observationdescribed a population the fit never reads. Measured on prod before the fix:contactsbeacon_measurementspskrThe silent state is reachable today, not just in theory: at a 4-week window (cutoff 2026-08-22) the old probe applies the beacon holdout, and the fit returns 0 bands / 0 rows — while 8,133 raw
beacon_measurementsrows sit inside that window.resolve_holdout's invariant held against the raw table, so no note fired.Fix: the population predicates are now written once —
CONTACTS_HRRR_MATCH,BEACON_HRRR_MATCH,BEACON_BAND_BIN_MATCH,BAND_MAP_VALUES— and interpolated into both each correlation query and its probe, so the two cannot drift. The probes useEXISTSinstead of the full join, because aMINneeds the population and not the matched rows (probe cost measured: ~2 s for all three against prod). PSKR needs no join and now says so.I did not take the "cheap post-fit check" alternative: the loaders return
BandCorrper band under aHAVING count(*) >= 50, so "no bands came back" cannot distinguish an empty fit from a band that merely missed the fit threshold — beacon returns zero bands today with a non-empty window. The shared-predicate version removes the drift rather than trying to detect it.Verification (
beff8b3a)first_observationrecorded in the JSON is now the joinable one.before=2026-08-22fit query was run directly and returns 0 rows, which is what the old probe was setting up.--dry-runreproduces the pre-refactor summary exactly — 14 merged bands (3 PSKR, 11 contacts, 0 beacon), 6 overrides — confirming the shared predicates are the ones the fit already applied.python3 -m py_compile scripts/recalibrate.pyclean.🤖 Skippy PR review
1 finding — 1 blocking before merge.
scripts/recalibrate.py:258Reviewed
308f2dc6ced0. Commentskippy reviewto re-run.@ -194,0 +255,4 @@"no rows match the source filters; fitting the full (empty) ""corpus with no holdout")if cutoff <= source_first_obs:🟠 High — Probe returns a naive datetime; aware/naive compare kills the run
min(hour_utc)/min(qso_timestamp)/min(bm.measured_at)come back naive: all three columns are Ecto:utc_datetime, which ecto_sql maps totimestamp(the partitionedhrrr_profilesDDL spells the same typetimestamp(0) WITHOUT TIME ZONE), and psycopg only returns aware datetimes fortimestamptz.cutoffhere is always aware, socutoff <= source_first_obsraisesTypeError: can't compare offset-naive and offset-aware datetimeson the first probe and the whole run dies before any fit is computed. The docstring's "psycopg returns aware datetimes for timestamptz" is the trap:nowgets normalized two lines up, the probe result does not. The stubbed-psycopgprobe in the PR verification cannot catch this, since a stub hands back whatever tz it is told to.Fix (these columns store UTC, so re-tagging is correct):
or probe with
min(hour_utc) AT TIME ZONE 'UTC'.308f2dc6ce9a98ae8537Fixed in
9a98ae85(rebased ontomain).Confirmed and reproduced against prod rather than by inspection: all three probes return naive datetimes (
min(hour_utc)→datetime(2026, 5, 4, 22, 0),min(qso_timestamp)→datetime(1991, 5, 4, 0, 50),min(measured_at)→datetime(2026, 8, 10, 11, 23, 47)) andplan_holdoutsraisedTypeError: can't compare offset-naive and offset-aware datetimesbefore computing a single fit. You were also right that the stubbed-psycopg check could not catch it — that verification was worthless for this class of bug._first_observation/2now re-tags a naive value as UTC, which is exact for columns the application writes in UTC, and the docstring'stimestamptzclaim is gone.With the fix, against prod: pskr and contacts resolve a 2026-07-25 cutoff (8 weeks), beacon_measurements falls back to its full corpus with the note, and a full
--dry-runfinishes in 202 s with 14 bands merged (3 PSKR, 11 contacts, 0 beacon) — the PSKR source that the fixed cutoff left with zero rows now fits three bands.One merge-order note for whoever lands these: #13, #14 and #15 all branch off
5db4ad54and all three touch files that overlap — #13 and #14 both editscripts/recalibrate.py(different regions: the holdout probe vs. the validation-gate helpers), and #13 and #15 both add a###entry under## 2026-09-19inCHANGELOG.md, which will textually conflict at the same insertion point. Resolution there is "keep both entries". Happy to rebase whichever of them lands last.skippy review
9a98ae8537437fd95e04🤖 Skippy PR review
2 findings — 1 blocking before merge.
deps:1depsdirscripts/recalibrate.py:428Findings outside the commentable diff
deps:1— Machine-local absolute symlink committed as the repo'sdepsdirThis commit adds
depsas a symlink to the absolute macOS path/Users/graham/dev/ntms/prop/deps(a different checkout, outside the repo). It slipped past the ignore rules because both.gitignore:14and.dockerignore:36use the trailing-slash form/deps/, which matches directories only, never a symlink. Every clone, CI checkout and Docker build context now gets a danglingdepslink: locallymix deps.getresolves prop's dependencies through another project's deps tree, and where the path does not exist the link is simply broken. Fix:git rm --cached deps(keep the local link) and widen both ignore patterns to/deps.Resolved 1 earlier finding (naive probe timestamps vs aware cutoff) fixed in
9a98ae85. Branch was rebased, so the previous head was no longer in history and this pass re-read the full PR diff.Reviewed
9a98ae85375a. Commentskippy reviewto re-run.@ -337,2 +425,4 @@# First-observation probes: one `MIN(...)` per source, filtered like the# corresponding correlation query so each rolling cutoff is resolved against🔵 Suggestion — Probes drop the HRRR/band_map joins, so "a cutoff can never empty the fit" is only approximate
The three
MIN(...)probes reuse the correlation queries' scalar filters but not their joins:CONTACTS_FIRST_OBS_SQLhas nohrrr_profilesjoin andBEACON_MEASUREMENTS_FIRST_OBS_SQLhas neither theband_mapnor thehrrr_profilesjoin. A source whose raw rows predate the cutoff but whose joinable rows only start after it gets an applied holdout and an empty fit (PSKR bands[], beacon0 bands) with nonoteand no warning bullet, which is the silent state this PR exists to remove. Adding the sameEXISTS (SELECT 1 FROM hrrr_profiles ...)/band_mappredicates to the probes closes it; otherwise a cheap post-fit check (if not source_rows: note/fallback) would do.Housekeeping: the follow-up commit is now
437fd95e(the SHA above predates an amend that dropped the worktreedepssymlink from the index). Same content otherwise —scripts/recalibrate.pyis the only file in this PR.🤖 Skippy PR review
2 findings — 2 blocking before merge.
deps:1depssymlink points at your Mac and breaks every other checkoutscripts/recalibrate.py:438Findings outside the commentable diff
deps:1— Committeddepssymlink points at your Mac and breaks every other checkoutThis adds
depsas a symlink (mode 120000) to the absolute path/Users/graham/dev/ntms/prop/deps. On any other checkout it is a dangling link: in the Linux clone and in the Docker/CI builds (Dockerfile.cicopies the tree, thenmix deps.get) the path exists but resolves nowhere, so mix cannot create or populate the deps directory and the build fails before compiling a single file..gitignorehas/deps/, which matches a real directory and not a symlink, so nothing prevents this being committed again.git rm deps(orgit rm --cached depsplus adepsline, no trailing slash, in.gitignore).Resolved 1 earlier finding (naive probe timestamps, fixed in 5ea5286..9a98ae8).
Reviewed
9a98ae85375a. Commentskippy reviewto re-run.@ -339,0 +435,4 @@"""CONTACTS_FIRST_OBS_SQL = """SELECT min(qso_timestamp)🟡 Warning — First-observation probe sees rows the fit query would discard
CONTACTS_FIRST_OBS_SQLfilterscontactson its own, but the fit query (CONTACTS_CORR_SQL) additionally inner-joinshrrr_profileson the +/-0.07 deg, +/-1h window and only appliesband >= 50after that join. Sofirst_observationcan predate every row the fit can actually use: the prod probe reported contacts1991-05-04, and no 1991 contact has an HRRR profile.resolve_holdoutdecides the fallback by comparing the cutoff against that stale value, so when a source's HRRR-matched rows all sit inside the rolling window (an HRRR archive gap, or a beacon whose old rows have no profiles)cutoff > source_first_obsholds, no warning fires, and the fit silently returns zero rows, which is the failure this PR exists to remove. Probe the same population the fit reads:SELECT min(qso_timestamp) FROM contacts c JOIN hrrr_profiles h ON ... WHERE c.band >= 50 AND ...with thebeforeclause stripped.BEACON_MEASUREMENTS_FIRST_OBS_SQLhas the same gap (noband_map/hrrr_profilesjoin).scripts/recalibrate.pyat437fd95is byte-identical to the file I reviewed at9a98ae85, so this push adds no new code to review. What did change:depssymlink is gone from the tree at437fd95(verified against the repo contents API and a fresh clone), so the "machine-local absolute symlink committed asdeps" warning is cleared.MIN(...)probes reuse the correlation queries' scalar filters but not their joins (hrrr_profiles,band_map), so a source whose raw rows start before the cutoff while its joinable rows start after it still gets an applied holdout and an empty fit with nonote.The branch was rewritten, so the previous head is not in history and this pass re-read the full PR diff; no earlier findings were re-posted.
Both right, and they are the same defect — fixed in
beff8b3a.The probes now read the population the fit reads. Measured on prod, before → after:
contacts:1991-05-04→2016-09-17(the earliest HRRR-matched contact)beacon_measurements:2026-08-10→2026-08-25(the earliest band-binned, HRRR-matched measurement)pskr: unchanged — its correlation query readspskr_calibration_samplesdirectly, so the probe has no join to mirrorYour escalation scenario is live today: at a 4-week window (cutoff 2026-08-22) the old probe applies the beacon holdout, and running that fit directly returns 0 bands / 0 rows — with 8,133 raw
beacon_measurementsrows inside the same window.resolve_holdout'sfit_cutoff > source_first_obsinvariant held against the wrong table, so no note fired and nothing was logged.CONTACTS_HRRR_MATCH,BEACON_HRRR_MATCH,BEACON_BAND_BIN_MATCHandBAND_MAP_VALUESare now written once and interpolated into both the correlation query and its probe, so the populations cannot drift apart again; the probes useEXISTSrather than the full join (aMINneeds the population, not the matched rows — all three probes together cost ~2 s against prod).I skipped the post-fit alternative you offered: the loaders return bands only under
HAVING count(*) >= 50, so an empty return cannot distinguish an empty fit from a band that missed the fit threshold — beacon returns zero bands today with a non-empty window, which is exactly the false positive that check would raise.Verification: 8-week probe resolution unchanged for pskr/contacts and beacon still declines with its note; 4-week beacon declines instead of fitting nothing; and a full
--dry-runreproduces the pre-refactor summary exactly — 14 bands (3 PSKR, 11 contacts, 0 beacon), 6 overrides — so the shared predicates are the ones the fit already used.skippy review
Resolved 2 of 2 earlier findings, both fixed in
beff8b3a:MIN(...)probes now reuse the fit queries' own join predicates (hrrr_profilesfor contacts;band_map+hrrr_profilesfor beacon) via shared constants (CONTACTS_HRRR_MATCH,BEACON_BAND_BIN_MATCH,BEACON_HRRR_MATCH), so a cutoff can no longer be resolved against a row the fit would discard.Nothing new in
437fd95..beff8b3a: the extracted predicates are character-identical to the ones removed from the two correlation queries, so the fit's own row population is unchanged.depsis still absent at this head.🤖 Skippy PR review
2 findings — 1 blocking before merge.
scripts/recalibrate.py:266scripts/recalibrate.py:252--holdout-weeks 0records a holdout window that contains no rowsResolved 2 earlier findings fixed in
beff8b3a(probes now reuse the fit queries' HRRR/band_map predicates). The validation gate in scripts/validate_algo.py still splits at 2025-01-01, so the rows it scores overlap this rolling fit window (the PR's own known follow-up). Branch was rebased, so this pass re-read the full PR diff.Reviewed
beff8b3a4788. Commentskippy reviewto re-run.@ -194,0 +249,4 @@raise ValueError(f"holdout weeks must be >= 0, got {weeks}")if now.tzinfo is None:now = now.replace(tzinfo=dt.timezone.utc)cutoff = now - dt.timedelta(weeks=weeks)🔵 Suggestion —
--holdout-weeks 0records a holdout window that contains no rows0passes the>= 0check and resolvescutoff = now, which is after every first observation, so the holdout counts as applied: the JSON and report record a realholdout_startequal to the run timestamp,note: null, no warning bullet, while the window the validation gate is meant to score (>= holdout_start) holds zero rows. Since--holdout-weeks 0is the natural way to ask for "fit everything, no holdout", either return(None, "no holdout requested")forweeks == 0or reject it with the same error path as a negative value.@ -194,0 +263,4 @@"with no holdout")assert cutoff > source_first_obs, "holdout cutoff would empty the fit window"🟡 Warning — The invariant is row-level, the fit is band-level: a source can still end up silently unfittable
resolve_holdoutguarantees only that one row exists before the cutoff, but a band is actually refit only when its window rows clear the load query'sHAVING count(*) >= 50andMIN_N_FOR_FIT = 1_000/MIN_CLUSTERS_FOR_FIT = 20(derive_weights, line 720). So a cutoff can survive the guard, be recorded asholdout_startwithnote: null, and still yield zero fittable bands, which is the silent-empty state this PR exists to remove, one layer down, with nothing in the JSON or report saying so.It is already scheduled:
beacon_measurements' first HRRR-matched observation is2026-08-25against a cutoff ofnow - 8w(2026-07-25today), so the fallback note fires now and stops firing in about four weeks. From then on the beacon fit window is days wide, every band sits under the thresholds, no beacon override is produced and no warning bullet appears. Any--holdout-weeksvalue that shrinks the window without crossingfirst_obsdoes the same today.Resolve the fallback from what the load returned instead of a lone
MIN(...): after theload_*calls, if a source contributed no band survivingMIN_N_FOR_FIT/MIN_CLUSTERS_FOR_FIT(or an empty band list) whileholdout_startis set, write thenotethere and re-emit the report warning. Cheaper variant: return the window's row and cluster counts from the probes soresolve_holdoutcan compare them against the thresholds.Resolved 2 of 2 earlier findings, both fixed in
8d71ef8d:--holdout-weeks 0now returns(None, "holdout disabled")before resolving a cutoff, so no phantomholdout_startat the run timestamp with an empty holdout window._source_block/2now recomputes the per-sourcenotefrom the loadedBandCorrs: an applied holdout whose bands all fall underMIN_N_FOR_FIT/MIN_CLUSTERS_FOR_FIT(including the empty-band-list case,0 of 0) writes the note and re-emits the report warning, so the band-level silent-empty state is visible instead ofnote: null.The threshold pair in that check is identical to the gate in
derive_weights/2(n < MIN_N_FOR_FIT or n_clusters < MIN_CLUSTERS_FOR_FIT→ no override), and the notes are built from each source's own loader output, so the note claims a non-contribution only when the fit genuinely produced nothing for that source.Nothing new in
beff8b3a..8d71ef8d: the report-line prefix change (the old "holdout not applied" wording is gone) has no other consumer in the repo (no test or doc parses that string), and the_source_blocksignature change is internal tobuild_json.Fixed in
8d71ef8d.Row-level invariant vs band-level fit threshold (6516, blocking).
resolve_holdoutonly guarantees a cutoff leaves one row before it; a band only actually refits once it clearsMIN_N_FOR_FIT/MIN_CLUSTERS_FOR_FITinsidederive_weights, one layer below where this PR's guard lives._source_blocknow takes each source's ownBandCorrdict instead of a precomputed(bands, total)pair, checks which of its bands actually clear both thresholds, and — when the holdout resolver's ownnoteisNone(holdout genuinely applied, not falling back) but zero bands survive — synthesizes a note naming the fit-window cutoff and the 0-of-N count. The three call sites now pass the dicts directly. The report's warning line no longer says "holdout not applied" unconditionally (wrong for this new case) — it's now note-agnostic:- warning: **{key}**: {note}.--holdout-weeks 0(6517, suggestion).resolve_holdoutnow short-circuits before computing a cutoff whenweeks == 0, returning(None, "holdout disabled (--holdout-weeks 0); fitting the full corpus")instead of silently resolving an empty holdout window that looked applied.Verification (psycopg isn't installed in this sandbox, so the script can't run end-to-end here): stubbed
sys.modules['psycopg']/psycopg.rowsso the real, unmodified module logic imports and runs, then exercised the actual functions —resolve_holdout(weeks=0)returns the disabled tuple;resolve_holdout(weeks=8, ...)unchanged for the normal case; a below-threshold-onlyBandCorrdict withnote=None, fit_before=<date>produces the synthesized "0 of 1 band(s) survived..." note; the same dict with one above-threshold band added producesnote: None.ast.parseconfirms valid syntax.skippy review