Per-source rolling holdout instead of a fixed 2025 cutoff #14

Merged
graham merged 4 commits from fix/rolling-per-source-holdout into main 2026-09-20 10:26:35 -05:00
Owner

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.)

  • Each source's fit now excludes only its own most recent ROLLING_HOLDOUT_WEEKS weeks (default 8; override with --holdout-weeks N or $PROP_HOLDOUT_WEEKS), resolved against a single now per run so every source agrees on one clock.
  • A source whose first observation is at/after the cutoff falls back to its full corpus (before=None) instead of silently fitting nothing; the JSON records holdout_start: null plus a note, and the report emits a warning bullet.
  • Cheap per-source MIN(...) probes reuse each correlation query's own row filters, so a resolved cutoff cannot be defeated by rows the fit would discard.
  • New pure resolve_holdout(first_obs, now, weeks); the invariant fit_cutoff > source_first_obs is asserted, so a holdout can never empty a fit window.
  • Fit window, first observation and the resolved now/window length are logged and recorded under data_sources in both the JSON and the Markdown report.

Verification

  • python3 -m py_compile scripts/recalibrate.py clean.
  • Throwaway probe (stubbed 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 case first_obs == cutoff falls back; --holdout-weeks 4 and $PROP_HOLDOUT_WEEKS move 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.
  • Grep confirms the old constant is gone and every before= call site is per-source.

Known follow-up (not in this PR)

validate_before_write / run_validation still shell out to scripts/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 by scripts/validate_algo.py; nothing in recalibrate.py needs to change when it does.

Run against prod in the review follow-up below (9a98ae85): the resolved
window 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_datetimetimestamp without time zone, so psycopg hands back naive datetimes while cutoff is always aware. Against prod, before the fix:

pskr:                datetime(2026, 5, 4, 22, 0)      tzinfo=None
contacts:            datetime(1991, 5, 4, 0, 50)      tzinfo=None
beacon_measurements: datetime(2026, 8, 10, 11, 23, 47) tzinfo=None
plan_holdouts raised: TypeError can't compare offset-naive and offset-aware datetimes

_first_observation/2 now 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 for timestamptz.

Verification against prod (this revision)

  • plan_holdouts resolves: 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-run completes 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.py clean.

validate_before_write still shells out to validate_algo.py with 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_observation described a population the fit never reads. Measured on prod before the fix:

source probe (raw table) probe (fit population)
contacts 1991-05-04 2016-09-17
beacon_measurements 2026-08-10 2026-08-25
pskr 2026-05-04 2026-05-04 (no join in its query)

The 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_measurements rows 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 use EXISTS instead of the full join, because a MIN needs 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 BandCorr per band under a HAVING 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)

  • Probes against prod: contacts and pskr resolve the 8-week cutoff (2026-07-25), beacon declines with the note, and each first_observation recorded in the JSON is now the joinable one.
  • 4-week window: beacon declines the holdout; the before=2026-08-22 fit query was run directly and returns 0 rows, which is what the old probe was setting up.
  • --dry-run reproduces 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.py clean.
## 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.) - Each source's fit now excludes only its own most recent `ROLLING_HOLDOUT_WEEKS` weeks (default 8; override with `--holdout-weeks N` or `$PROP_HOLDOUT_WEEKS`), resolved against a single `now` per run so every source agrees on one clock. - A source whose first observation is at/after the cutoff falls back to its full corpus (`before=None`) instead of silently fitting nothing; the JSON records `holdout_start: null` plus a `note`, and the report emits a warning bullet. - Cheap per-source `MIN(...)` probes reuse each correlation query's own row filters, so a resolved cutoff cannot be defeated by rows the fit would discard. - New pure `resolve_holdout(first_obs, now, weeks)`; the invariant `fit_cutoff > source_first_obs` is asserted, so a holdout can never empty a fit window. - Fit window, first observation and the resolved `now`/window length are logged and recorded under `data_sources` in both the JSON and the Markdown report. ## Verification - `python3 -m py_compile scripts/recalibrate.py` clean. - Throwaway probe (stubbed `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 case `first_obs == cutoff` falls back; `--holdout-weeks 4` and `$PROP_HOLDOUT_WEEKS` move 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. - Grep confirms the old constant is gone and every `before=` call site is per-source. ## Known follow-up (not in this PR) `validate_before_write` / `run_validation` still shell out to `scripts/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 by `scripts/validate_algo.py`; nothing in `recalibrate.py` needs to change when it does. Run against prod in the review follow-up below (9a98ae85): the resolved window 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 while `cutoff` is always aware. Against prod, before the fix: ``` pskr: datetime(2026, 5, 4, 22, 0) tzinfo=None contacts: datetime(1991, 5, 4, 0, 50) tzinfo=None beacon_measurements: datetime(2026, 8, 10, 11, 23, 47) tzinfo=None plan_holdouts raised: TypeError can't compare offset-naive and offset-aware datetimes ``` `_first_observation/2` now 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 for `timestamptz`. ## Verification against prod (this revision) - `plan_holdouts` resolves: 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-run` completes 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.py` clean. `validate_before_write` still shells out to `validate_algo.py` with 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_observation` described a population the fit never reads. Measured on prod before the fix: | source | probe (raw table) | probe (fit population) | | --- | --- | --- | | `contacts` | 1991-05-04 | **2016-09-17** | | `beacon_measurements` | 2026-08-10 | **2026-08-25** | | `pskr` | 2026-05-04 | 2026-05-04 (no join in its query) | The 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_measurements` rows 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 use `EXISTS` instead of the full join, because a `MIN` needs 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 `BandCorr` per band under a `HAVING 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) - Probes against prod: contacts and pskr resolve the 8-week cutoff (2026-07-25), beacon declines with the note, and each `first_observation` recorded in the JSON is now the joinable one. - 4-week window: beacon declines the holdout; the `before=2026-08-22` fit query was run directly and returns 0 rows, which is what the old probe was setting up. - `--dry-run` reproduces 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.py` clean.
fix(algo): per-source rolling holdout instead of a fixed 2025 cutoff
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
308f2dc6ce
skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity Location Issue
🟠 High scripts/recalibrate.py:258 Probe returns a naive datetime; aware/naive compare kills the run

Reviewed 308f2dc6ced0. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `scripts/recalibrate.py:258` | Probe returns a naive datetime; aware/naive compare kills the run | <sub>Reviewed `308f2dc6ced0`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -194,0 +255,4 @@
"no rows match the source filters; fitting the full (empty) "
"corpus with no holdout"
)
if cutoff <= source_first_obs:
First-time contributor

🟠 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 to timestamp (the partitioned hrrr_profiles DDL spells the same type timestamp(0) WITHOUT TIME ZONE), and psycopg only returns aware datetimes for timestamptz. cutoff here is always aware, so cutoff <= source_first_obs raises TypeError: can't compare offset-naive and offset-aware datetimes on 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: now gets normalized two lines up, the probe result does not. The stubbed-psycopg probe 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):

if source_first_obs is not None and source_first_obs.tzinfo is None:
    source_first_obs = source_first_obs.replace(tzinfo=dt.timezone.utc)

or probe with min(hour_utc) AT TIME ZONE 'UTC'.

**🟠 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 to `timestamp` (the partitioned `hrrr_profiles` DDL spells the same type `timestamp(0) WITHOUT TIME ZONE`), and psycopg only returns aware datetimes for `timestamptz`. `cutoff` here is always aware, so `cutoff <= source_first_obs` raises `TypeError: can't compare offset-naive and offset-aware datetimes` on 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: `now` gets normalized two lines up, the probe result does not. The stubbed-`psycopg` probe 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): ```python if source_first_obs is not None and source_first_obs.tzinfo is None: source_first_obs = source_first_obs.replace(tzinfo=dt.timezone.utc) ``` or probe with `min(hour_utc) AT TIME ZONE 'UTC'`.
skippy-bot marked this conversation as resolved
graham force-pushed fix/rolling-per-source-holdout from 308f2dc6ce
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
to 9a98ae8537
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
2026-09-19 18:10:59 -05:00
Compare
Author
Owner

Fixed in 9a98ae85 (rebased onto main).

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)) and plan_holdouts raised TypeError: can't compare offset-naive and offset-aware datetimes before 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/2 now re-tags a naive value as UTC, which is exact for columns the application writes in UTC, and the docstring's timestamptz claim 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-run finishes 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.

Fixed in `9a98ae85` (rebased onto `main`). 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)`) and `plan_holdouts` raised `TypeError: can't compare offset-naive and offset-aware datetimes` before 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/2` now re-tags a naive value as UTC, which is exact for columns the application writes in UTC, and the docstring's `timestamptz` claim 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-run` finishes 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.
Author
Owner

One merge-order note for whoever lands these: #13, #14 and #15 all branch off 5db4ad54 and all three touch files that overlap — #13 and #14 both edit scripts/recalibrate.py (different regions: the holdout probe vs. the validation-gate helpers), and #13 and #15 both add a ### entry under ## 2026-09-19 in CHANGELOG.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

One merge-order note for whoever lands these: #13, #14 and #15 all branch off `5db4ad54` and all three touch files that overlap — #13 and #14 both edit `scripts/recalibrate.py` (different regions: the holdout probe vs. the validation-gate helpers), and #13 and #15 both add a `###` entry under `## 2026-09-19` in `CHANGELOG.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
graham force-pushed fix/rolling-per-source-holdout from 9a98ae8537
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
to 437fd95e04
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
2026-09-19 18:12:33 -05:00
Compare
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity Location Issue
🟡 Warning deps:1 Machine-local absolute symlink committed as the repo's deps dir
🔵 Suggestion scripts/recalibrate.py:428 Probes drop the HRRR/band_map joins, so "a cutoff can never empty the fit" is only approximate

Findings outside the commentable diff

  • 🟡 Warning — deps:1 — Machine-local absolute symlink committed as the repo's deps dir
    This commit adds deps as 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:14 and .dockerignore:36 use the trailing-slash form /deps/, which matches directories only, never a symlink. Every clone, CI checkout and Docker build context now gets a dangling deps link: locally mix deps.get resolves 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. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `deps:1` | Machine-local absolute symlink committed as the repo's `deps` dir | | 🔵 Suggestion | `scripts/recalibrate.py:428` | Probes drop the HRRR/band_map joins, so "a cutoff can never empty the fit" is only approximate | #### Findings outside the commentable diff - **🟡 Warning — `deps:1` — Machine-local absolute symlink committed as the repo's `deps` dir** This commit adds `deps` as 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:14` and `.dockerignore:36` use the trailing-slash form `/deps/`, which matches directories only, never a symlink. Every clone, CI checkout and Docker build context now gets a dangling `deps` link: locally `mix deps.get` resolves 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. <sub>Reviewed `9a98ae85375a`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -337,2 +425,4 @@
# First-observation probes: one `MIN(...)` per source, filtered like the
# corresponding correlation query so each rolling cutoff is resolved against
First-time contributor

🔵 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_SQL has no hrrr_profiles join and BEACON_MEASUREMENTS_FIRST_OBS_SQL has neither the band_map nor the hrrr_profiles join. 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 [], beacon 0 bands) with no note and no warning bullet, which is the silent state this PR exists to remove. Adding the same EXISTS (SELECT 1 FROM hrrr_profiles ...) / band_map predicates to the probes closes it; otherwise a cheap post-fit check (if not source_rows: note/fallback) would do.

**🔵 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_SQL` has no `hrrr_profiles` join and `BEACON_MEASUREMENTS_FIRST_OBS_SQL` has neither the `band_map` nor the `hrrr_profiles` join. 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 `[]`, beacon `0 bands`) with no `note` and no warning bullet, which is the silent state this PR exists to remove. Adding the same `EXISTS (SELECT 1 FROM hrrr_profiles ...)` / `band_map` predicates to the probes closes it; otherwise a cheap post-fit check (`if not source_rows: note/fallback`) would do.
skippy-bot marked this conversation as resolved
Author
Owner

Housekeeping: the follow-up commit is now 437fd95e (the SHA above predates an amend that dropped the worktree deps symlink from the index). Same content otherwise — scripts/recalibrate.py is the only file in this PR.

Housekeeping: the follow-up commit is now `437fd95e` (the SHA above predates an amend that dropped the worktree `deps` symlink from the index). Same content otherwise — `scripts/recalibrate.py` is the only file in this PR.
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity Location Issue
🟠 High deps:1 Committed deps symlink points at your Mac and breaks every other checkout
🟡 Warning scripts/recalibrate.py:438 First-observation probe sees rows the fit query would discard

Findings outside the commentable diff

  • 🟠 High — deps:1 — Committed deps symlink points at your Mac and breaks every other checkout
    This adds deps as 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.ci copies the tree, then mix 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. .gitignore has /deps/, which matches a real directory and not a symlink, so nothing prevents this being committed again. git rm deps (or git rm --cached deps plus a deps line, no trailing slash, in .gitignore).

Resolved 1 earlier finding (naive probe timestamps, fixed in 5ea5286..9a98ae8).

Reviewed 9a98ae85375a. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `deps:1` | Committed `deps` symlink points at your Mac and breaks every other checkout | | 🟡 Warning | `scripts/recalibrate.py:438` | First-observation probe sees rows the fit query would discard | #### Findings outside the commentable diff - **🟠 High — `deps:1` — Committed `deps` symlink points at your Mac and breaks every other checkout** This adds `deps` as 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.ci` copies the tree, then `mix 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. `.gitignore` has `/deps/`, which matches a real directory and not a symlink, so nothing prevents this being committed again. `git rm deps` (or `git rm --cached deps` plus a `deps` line, no trailing slash, in `.gitignore`). Resolved 1 earlier finding (naive probe timestamps, fixed in 5ea5286..9a98ae8). <sub>Reviewed `9a98ae85375a`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -339,0 +435,4 @@
"""
CONTACTS_FIRST_OBS_SQL = """
SELECT min(qso_timestamp)
First-time contributor

🟡 Warning — First-observation probe sees rows the fit query would discard

CONTACTS_FIRST_OBS_SQL filters contacts on its own, but the fit query (CONTACTS_CORR_SQL) additionally inner-joins hrrr_profiles on the +/-0.07 deg, +/-1h window and only applies band >= 50 after that join. So first_observation can predate every row the fit can actually use: the prod probe reported contacts 1991-05-04, and no 1991 contact has an HRRR profile. resolve_holdout decides 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_obs holds, 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 the before clause stripped. BEACON_MEASUREMENTS_FIRST_OBS_SQL has the same gap (no band_map/hrrr_profiles join).

**🟡 Warning — First-observation probe sees rows the fit query would discard** `CONTACTS_FIRST_OBS_SQL` filters `contacts` on its own, but the fit query (`CONTACTS_CORR_SQL`) additionally inner-joins `hrrr_profiles` on the +/-0.07 deg, +/-1h window and only applies `band >= 50` after that join. So `first_observation` can predate every row the fit can actually use: the prod probe reported contacts `1991-05-04`, and no 1991 contact has an HRRR profile. `resolve_holdout` decides 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_obs` holds, 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 the `before` clause stripped. `BEACON_MEASUREMENTS_FIRST_OBS_SQL` has the same gap (no `band_map`/`hrrr_profiles` join).
skippy-bot marked this conversation as resolved
First-time contributor

scripts/recalibrate.py at 437fd95 is byte-identical to the file I reviewed at 9a98ae85, so this push adds no new code to review. What did change:

  • Resolved: the deps symlink is gone from the tree at 437fd95 (verified against the repo contents API and a fresh clone), so the "machine-local absolute symlink committed as deps" warning is cleared.
  • Still open, non-blocking: the three 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 no note.

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.

`scripts/recalibrate.py` at `437fd95` is byte-identical to the file I reviewed at `9a98ae85`, so this push adds no new code to review. What did change: - **Resolved:** the `deps` symlink is gone from the tree at `437fd95` (verified against the repo contents API and a fresh clone), so the "machine-local absolute symlink committed as `deps`" warning is cleared. - **Still open, non-blocking:** the three `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 no `note`. 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. <!-- skippy-pr-review -->
fix(algo): probe the population the fit reads, not the raw table
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
beff8b3a47
Review follow-up on #14 (second round). The first-observation probes reused the
correlation queries' scalar filters but not their joins, so `first_observation`
could predate every row the fit can use — measured on prod:

    source              probe before   probe after (HRRR/band_map matched)
    contacts            1991-05-04     2016-09-17
    beacon_measurements 2026-08-10     2026-08-25

`resolve_holdout` decides the fallback by comparing the cutoff against that
value, so a source whose matched rows all sit inside the rolling window got the
holdout applied, the `fit_cutoff > source_first_obs` invariant held against a
population the fit never had, no note fired, and the fit came back empty — the
silent state this PR exists to remove. At a 4-week window today the old probe
applies the beacon holdout and its fit returns 0 bands / 0 rows, while 8,133
raw beacon measurements sit inside that window.

The join predicates are now written once (`CONTACTS_HRRR_MATCH`,
`BEACON_HRRR_MATCH`, `BEACON_BAND_BIN_MATCH`, `BAND_MAP_VALUES`) and
interpolated into both the correlation query and its probe, so the two cannot
drift; the probes use `EXISTS` rather than the full join, since a `MIN` needs
the population and not the matched rows. PSKR needs no join — its query reads
`pskr_calibration_samples` directly — and says so.

Verified against prod: contacts and pskr still resolve the 8-week cutoff
(2026-07-25) and beacon still falls back with its note, now off the joinable
first observation; at 4 weeks the beacon holdout is declined instead of applied
to an empty fit; a full `--dry-run` reproduces the same 14 merged bands
(3 PSKR, 11 contacts, 0 beacon) as before the SQL refactor, confirming the
shared predicates are the ones the fit already used.
Author
Owner

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-042016-09-17 (the earliest HRRR-matched contact)
  • beacon_measurements: 2026-08-102026-08-25 (the earliest band-binned, HRRR-matched measurement)
  • pskr: unchanged — its correlation query reads pskr_calibration_samples directly, so the probe has no join to mirror

Your 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_measurements rows inside the same window. resolve_holdout's fit_cutoff > source_first_obs invariant held against the wrong table, so no note fired and nothing was logged.

CONTACTS_HRRR_MATCH, BEACON_HRRR_MATCH, BEACON_BAND_BIN_MATCH and BAND_MAP_VALUES are now written once and interpolated into both the correlation query and its probe, so the populations cannot drift apart again; the probes use EXISTS rather than the full join (a MIN needs 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-run reproduces 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

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 reads `pskr_calibration_samples` directly, so the probe has no join to mirror Your 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_measurements` rows inside the same window. `resolve_holdout`'s `fit_cutoff > source_first_obs` invariant held against the wrong table, so no note fired and nothing was logged. `CONTACTS_HRRR_MATCH`, `BEACON_HRRR_MATCH`, `BEACON_BAND_BIN_MATCH` and `BAND_MAP_VALUES` are now written once and interpolated into both the correlation query and its probe, so the populations cannot drift apart again; the probes use `EXISTS` rather than the full join (a `MIN` needs 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-run` reproduces 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
First-time contributor

Resolved 2 of 2 earlier findings, both fixed in beff8b3a:

  • The MIN(...) probes now reuse the fit queries' own join predicates (hrrr_profiles for contacts; band_map + hrrr_profiles for 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.
  • The beacon probe picks up the band-bin predicate it was missing.

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. deps is still absent at this head.

Resolved 2 of 2 earlier findings, both fixed in `beff8b3a`: - The `MIN(...)` probes now reuse the fit queries' own join predicates (`hrrr_profiles` for contacts; `band_map` + `hrrr_profiles` for 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. - The beacon probe picks up the band-bin predicate it was missing. 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. `deps` is still absent at this head. <!-- skippy-pr-review -->
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity Location Issue
🟡 Warning scripts/recalibrate.py:266 The invariant is row-level, the fit is band-level: a source can still end up silently unfittable
🔵 Suggestion scripts/recalibrate.py:252 --holdout-weeks 0 records a holdout window that contains no rows

Resolved 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. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `scripts/recalibrate.py:266` | The invariant is row-level, the fit is band-level: a source can still end up silently unfittable | | 🔵 Suggestion | `scripts/recalibrate.py:252` | `--holdout-weeks 0` records a holdout window that contains no rows | Resolved 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. <sub>Reviewed `beff8b3a4788`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -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)
First-time contributor

🔵 Suggestion — --holdout-weeks 0 records a holdout window that contains no rows

0 passes the >= 0 check and resolves cutoff = now, which is after every first observation, so the holdout counts as applied: the JSON and report record a real holdout_start equal 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 0 is the natural way to ask for "fit everything, no holdout", either return (None, "no holdout requested") for weeks == 0 or reject it with the same error path as a negative value.

**🔵 Suggestion — `--holdout-weeks 0` records a holdout window that contains no rows** `0` passes the `>= 0` check and resolves `cutoff = now`, which is after every first observation, so the holdout counts as applied: the JSON and report record a real `holdout_start` equal 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 0` is the natural way to ask for "fit everything, no holdout", either return `(None, "no holdout requested")` for `weeks == 0` or reject it with the same error path as a negative value.
skippy-bot marked this conversation as resolved
@ -194,0 +263,4 @@
"with no holdout"
)
assert cutoff > source_first_obs, "holdout cutoff would empty the fit window"
First-time contributor

🟡 Warning — The invariant is row-level, the fit is band-level: a source can still end up silently unfittable

resolve_holdout guarantees only that one row exists before the cutoff, but a band is actually refit only when its window rows clear the load query's HAVING count(*) >= 50 and MIN_N_FOR_FIT = 1_000 / MIN_CLUSTERS_FOR_FIT = 20 (derive_weights, line 720). So a cutoff can survive the guard, be recorded as holdout_start with note: 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 is 2026-08-25 against a cutoff of now - 8w (2026-07-25 today), 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-weeks value that shrinks the window without crossing first_obs does the same today.

Resolve the fallback from what the load returned instead of a lone MIN(...): after the load_* calls, if a source contributed no band surviving MIN_N_FOR_FIT / MIN_CLUSTERS_FOR_FIT (or an empty band list) while holdout_start is set, write the note there and re-emit the report warning. Cheaper variant: return the window's row and cluster counts from the probes so resolve_holdout can compare them against the thresholds.

**🟡 Warning — The invariant is row-level, the fit is band-level: a source can still end up silently unfittable** `resolve_holdout` guarantees only that one row exists before the cutoff, but a band is actually refit only when its window rows clear the load query's `HAVING count(*) >= 50` *and* `MIN_N_FOR_FIT = 1_000` / `MIN_CLUSTERS_FOR_FIT = 20` (`derive_weights`, line 720). So a cutoff can survive the guard, be recorded as `holdout_start` with `note: 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 is `2026-08-25` against a cutoff of `now - 8w` (`2026-07-25` today), 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-weeks` value that shrinks the window without crossing `first_obs` does the same today. Resolve the fallback from what the load returned instead of a lone `MIN(...)`: after the `load_*` calls, if a source contributed no band surviving `MIN_N_FOR_FIT` / `MIN_CLUSTERS_FOR_FIT` (or an empty band list) while `holdout_start` is set, write the `note` there and re-emit the report warning. Cheaper variant: return the window's row and cluster counts from the probes so `resolve_holdout` can compare them against the thresholds.
skippy-bot marked this conversation as resolved
fix(algo): treat 0-week holdout as disabled and flag sources with no fittable bands
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
8d71ef8d9c
resolve_holdout(weeks=0) previously resolved cutoff=now, silently applying
an empty holdout window instead of disabling holdout. Now returns (None,
note) before computing a cutoff, matching the negative-weeks guard already
in place.

Separately, resolve_holdout only guarantees a cutoff leaves >=1 row before
it; a band is only actually refit once it clears MIN_N_FOR_FIT /
MIN_CLUSTERS_FOR_FIT in derive_weights. A cutoff could pass that row-level
guard and still leave zero of a source's bands fittable, while build_json
echoed note: null as if nothing were wrong. _source_block now takes the
source's own BandCorr dict, and synthesizes a note when the holdout was
applied but no band survived the fit thresholds. The markdown report's
warning line is reworded to be note-agnostic since it now covers both
'holdout not applied' and 'holdout applied but nothing survived'.
First-time contributor

Resolved 2 of 2 earlier findings, both fixed in 8d71ef8d:

  • --holdout-weeks 0 now returns (None, "holdout disabled") before resolving a cutoff, so no phantom holdout_start at the run timestamp with an empty holdout window.
  • _source_block/2 now recomputes the per-source note from the loaded BandCorrs: an applied holdout whose bands all fall under MIN_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 of note: 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_block signature change is internal to build_json.

Resolved 2 of 2 earlier findings, both fixed in `8d71ef8d`: - `--holdout-weeks 0` now returns `(None, "holdout disabled")` before resolving a cutoff, so no phantom `holdout_start` at the run timestamp with an empty holdout window. - `_source_block/2` now recomputes the per-source `note` from the loaded `BandCorr`s: an applied holdout whose bands all fall under `MIN_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 of `note: 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_block` signature change is internal to `build_json`. <!-- skippy-pr-review -->
graham merged commit aa77f3ba65 into main 2026-09-20 10:26:35 -05:00
graham deleted branch fix/rolling-per-source-holdout 2026-09-20 10:26:35 -05:00
Author
Owner

Fixed in 8d71ef8d.

Row-level invariant vs band-level fit threshold (6516, blocking). resolve_holdout only guarantees a cutoff leaves one row before it; a band only actually refits once it clears MIN_N_FOR_FIT/MIN_CLUSTERS_FOR_FIT inside derive_weights, one layer below where this PR's guard lives. _source_block now takes each source's own BandCorr dict instead of a precomputed (bands, total) pair, checks which of its bands actually clear both thresholds, and — when the holdout resolver's own note is None (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_holdout now short-circuits before computing a cutoff when weeks == 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.rows so 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-only BandCorr dict with note=None, fit_before=<date> produces the synthesized "0 of 1 band(s) survived..." note; the same dict with one above-threshold band added produces note: None. ast.parse confirms valid syntax.

skippy review

Fixed in `8d71ef8d`. **Row-level invariant vs band-level fit threshold (6516, blocking).** `resolve_holdout` only guarantees a cutoff leaves one row before it; a band only actually refits once it clears `MIN_N_FOR_FIT`/`MIN_CLUSTERS_FOR_FIT` inside `derive_weights`, one layer below where this PR's guard lives. `_source_block` now takes each source's own `BandCorr` dict instead of a precomputed `(bands, total)` pair, checks which of its bands actually clear both thresholds, and — when the holdout resolver's own `note` is `None` (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_holdout` now short-circuits before computing a cutoff when `weeks == 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.rows` so 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-only `BandCorr` dict with `note=None, fit_before=<date>` produces the synthesized "0 of 1 band(s) survived..." note; the same dict with one above-threshold band added produces `note: None`. `ast.parse` confirms valid syntax. skippy review
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
graham/prop!14
No description provided.