Make the algorithm validator score what production scores #13

Merged
graham merged 5 commits from fix/validator-production-parity into main 2026-09-20 10:55:32 -05:00
Owner

What

scripts/validate_algo.py scored a different algorithm than production, so every rho in docs/algo-reports/validation-*.md — and the accept/revert gate in scripts/recalibrate.py that shells out to it — measured a scorer that does not exist. This rebuilds the port from the source of truth and fixes the degenerate baseline table. (tmp/bugs.md §A, three P0 items.)

Parity fixes

  • score_pressure was inverted: 88 at <980 mb falling to 30 at >=1020 mb, against production's ascending 30 → 40 → 55 → 70 → 82 → 88 (scorer.ex:543-548). The tendency curve is ported too, with a comment that hrrr_profiles has no previous-pressure column so it is unreachable from this report.
  • score_season omitted the Region multiplier (Region.for_point/2 + Region.seasonal_adjustment/2). The join now selects pos1 latitude, and a missing coordinate falls back to the CONUS centre exactly where production's path helpers do.
  • score_time_of_day used a month-only sunrise table and the fixed local >= 20 evening cutoff. It now ports BandConfig.sunrise_hour/2 + sunset_hour/2 and the sunset-relative clause order.
  • Band tables re-derived from band_config.ex / region.ex (24/47 GHz rain_k/rain_alpha were still the pre-2026-04 values; humidity, refractivity, seasonal and region boxes re-checked), and weights now resolve through the same JSON-first order as BandWeights.lookup/1.
  • Retired inputs (duct band, bulk Richardson, BL depth) are still passed to the ported refractivity factor, proving they are inert rather than assuming it.
  • flagged_invalid predicate aligned with the column definition (boolean NOT NULL DEFAULT false).

Baseline table

pers_deltas was literally 0.0, so rho(persistence) was identically 0 and "skill gain" was just rho(algorithm); climatology and no-skill were both a constant minus the monthly median, i.e. one rank vector carrying one number. Now every predictor is scored against the raw distance target (algorithm, monthly persistence, climatology, no-skill), a constant predictor reports an undefined rho (null in JSON, in the table, never 0.0), and "does the score beat month-of-year alone?" is a direct rho(alg) − rho(pers) comparison. The seasonality-removed view is kept, clearly labelled, for the algorithm only.

Three of the ten factors (sky, wind, rain) have no column in hrrr_profiles and stay pinned to their neutral constants. The report and JSON now state that share of composite weight per band, instead of hiding it behind a single number.

Verification

  • Value-for-value parity against a dump of the real Elixir scorer: every band's rain/humidity/seasonal/refractivity/region tables, the JSON-first weight resolution, sunrise and sunset over 8 latitudes × 12 months, time-of-day over 5 latitudes × 12 months × 24 hours, pressure including the tendency curve, humidity/T−Td/refractivity/PWAT/rain/sky/wind grids, season over 5 lat/lon × 12 months, and 5 synthetic condition sets × 23 bands → 0 mismatches. The only intentional gaps are the three pinned factors, and recomposing the composite from production's own factor values reproduces every expected score exactly.
  • Baseline behaviour proven on synthetic rows: persistence rho +0.886 vs algorithm rho −0.462 (skill gain −1.349 — the table can now show month-of-year beating the score), constants undefined, uncovered months falling back to the band median.
  • python3 -m py_compile clean.

Merge order (one dependency)

The time-of-day port mirrors the scorer after fix/time-of-day-sunset (the sunset-relative evening window from BandConfig.sunset_hour/2 + classify_time_period/3). Merge that PR first — against it this port is exact (0 mismatches over the full band/factor grid). If that scorer change is rejected, restore the fixed-hour evening clause here in one hunk (local >= 20 or local <= 1, no sunset argument), otherwise the validator scores an evening window production does not have — the exact failure mode this PR removes.

The script itself was not run (it needs PROP_PROD_DB_URL and prod data).

Review follow-up (01069008)

Rebased onto main#12 merged at 17:56, so the ported evening window now matches the scorer production runs and the merge-order dependency is gone.

Null rho broke both readers it feeds — fixed, and it is not hypothetical. A regenerated report (run below) carries six bands with skill_gain: null (222 MHz n=752, 432 MHz n=898, 2304/3400/5760/122000 MHz). On that payload the pre-fix sum(1 for info in per_band.values() if info.get("skill_gain", 0.0) > 0) raises TypeError: '>' not supported between instances of 'NoneType' and 'int' — i.e. recalibrate.py dies after the gate has spent its two validation runs — and validation_regressions raises the same way on float(alg_rho). Both now skip an undefined measurement instead of defaulting it to zero (no two numbers, no regression verdict), counted by a new skill_gain_counts/1.

ValidationReport had the same defect at compile time: nils sorted in with the gains (numbers sort before atoms), so an odd eligible count put nil into a float() field — Float.round/2 on the /algo facts band — and an even one averaged the wrong pairs. Against the regenerated report the old pipeline reports 0.2379 over 6 bands; the guarded one reports 0.086 over 4 (gains −0.0053, 0.0073, 0.1647, 0.3110). Undefined gains are now rejected before the median and bands counts the bands the median is over.

Rows production refuses to score are dropped by a new partition_scorable/1 and counted in the payload (summary.unscorable_rows) and the Markdown header. Worth knowing: this one is latent rather than active — hrrr_profiles currently holds 0 NULLs in surface_temp_c/surface_dewpoint_c across 89,098,625 rows, so nothing was being fabricated today. It stays because the schema allows it, PathCompute.build_scoring/6 bails on an empty temperature list, and the fabrication (humidity 75 plus a T−Td score) is silent when it happens.

Humidity comment rewritten as suggested — you are right that nil falls through to 75 on beneficial bands (every comparison against an atom is false) and only raises on the harmful nil * penalty. The T−Td clause gets the same treatment: production raises there, too, so both are documented as guards, not curves.

Verification (this revision)

  • python3 scripts/validate_algo.py against prod: 74,087 joined rows scored, unscorable_rows: 0, report written, py_compile clean on both scripts.
  • The regenerated report is not committed here — it moves the /algo facts band, which is a separate deliberate step.
  • make precommit: green (4930 passed, 6 skipped, credo/xref/format clean).
## What `scripts/validate_algo.py` scored a *different algorithm* than production, so every rho in `docs/algo-reports/validation-*.md` — and the accept/revert gate in `scripts/recalibrate.py` that shells out to it — measured a scorer that does not exist. This rebuilds the port from the source of truth and fixes the degenerate baseline table. (`tmp/bugs.md` §A, three P0 items.) ### Parity fixes - `score_pressure` was **inverted**: 88 at `<980 mb` falling to 30 at `>=1020 mb`, against production's ascending 30 → 40 → 55 → 70 → 82 → 88 (`scorer.ex:543-548`). The tendency curve is ported too, with a comment that `hrrr_profiles` has no previous-pressure column so it is unreachable from this report. - `score_season` omitted the **Region multiplier** (`Region.for_point/2` + `Region.seasonal_adjustment/2`). The join now selects `pos1` latitude, and a missing coordinate falls back to the CONUS centre exactly where production's path helpers do. - `score_time_of_day` used a month-only sunrise table and the fixed `local >= 20` evening cutoff. It now ports `BandConfig.sunrise_hour/2` + `sunset_hour/2` and the sunset-relative clause order. - Band tables re-derived from `band_config.ex` / `region.ex` (24/47 GHz `rain_k`/`rain_alpha` were still the pre-2026-04 values; humidity, refractivity, seasonal and region boxes re-checked), and weights now resolve through the same JSON-first order as `BandWeights.lookup/1`. - Retired inputs (duct band, bulk Richardson, BL depth) are still passed to the ported refractivity factor, proving they are inert rather than assuming it. - `flagged_invalid` predicate aligned with the column definition (`boolean NOT NULL DEFAULT false`). ### Baseline table `pers_deltas` was literally `0.0`, so rho(persistence) was identically 0 and "skill gain" was just rho(algorithm); climatology and no-skill were both a constant minus the monthly median, i.e. one rank vector carrying one number. Now every predictor is scored against the **raw** distance target (algorithm, monthly persistence, climatology, no-skill), a constant predictor reports an **undefined** rho (`null` in JSON, `—` in the table, never `0.0`), and "does the score beat month-of-year alone?" is a direct rho(alg) − rho(pers) comparison. The seasonality-removed view is kept, clearly labelled, for the algorithm only. Three of the ten factors (sky, wind, rain) have no column in `hrrr_profiles` and stay pinned to their neutral constants. The report and JSON now state that share of composite weight per band, instead of hiding it behind a single number. ## Verification - Value-for-value parity against a dump of the **real Elixir scorer**: every band's rain/humidity/seasonal/refractivity/region tables, the JSON-first weight resolution, sunrise and sunset over 8 latitudes × 12 months, time-of-day over 5 latitudes × 12 months × 24 hours, pressure including the tendency curve, humidity/T−Td/refractivity/PWAT/rain/sky/wind grids, season over 5 lat/lon × 12 months, and 5 synthetic condition sets × 23 bands → **0 mismatches**. The only intentional gaps are the three pinned factors, and recomposing the composite from production's own factor values reproduces every expected score exactly. - Baseline behaviour proven on synthetic rows: persistence rho +0.886 vs algorithm rho −0.462 (skill gain −1.349 — the table can now show month-of-year beating the score), constants undefined, uncovered months falling back to the band median. - `python3 -m py_compile` clean. ## Merge order (one dependency) The time-of-day port mirrors the scorer **after** `fix/time-of-day-sunset` (the sunset-relative evening window from `BandConfig.sunset_hour/2` + `classify_time_period/3`). Merge that PR first — against it this port is exact (0 mismatches over the full band/factor grid). If that scorer change is rejected, restore the fixed-hour evening clause here in one hunk (`local >= 20 or local <= 1`, no `sunset` argument), otherwise the validator scores an evening window production does not have — the exact failure mode this PR removes. The script itself was not run (it needs `PROP_PROD_DB_URL` and prod data). ## Review follow-up (01069008) Rebased onto `main` — #12 merged at 17:56, so the ported evening window now matches the scorer production runs and the merge-order dependency is gone. **Null rho broke both readers it feeds — fixed, and it is not hypothetical.** A regenerated report (run below) carries six bands with `skill_gain: null` (222 MHz n=752, 432 MHz n=898, 2304/3400/5760/122000 MHz). On that payload the pre-fix `sum(1 for info in per_band.values() if info.get("skill_gain", 0.0) > 0)` raises `TypeError: '>' not supported between instances of 'NoneType' and 'int'` — i.e. `recalibrate.py` dies *after* the gate has spent its two validation runs — and `validation_regressions` raises the same way on `float(alg_rho)`. Both now skip an undefined measurement instead of defaulting it to zero (no two numbers, no regression verdict), counted by a new `skill_gain_counts/1`. `ValidationReport` had the same defect at compile time: nils sorted in with the gains (numbers sort before atoms), so an odd eligible count put `nil` into a `float()` field — `Float.round/2` on the `/algo` facts band — and an even one averaged the wrong pairs. Against the regenerated report the old pipeline reports **0.2379 over 6 bands**; the guarded one reports **0.086 over 4** (gains −0.0053, 0.0073, 0.1647, 0.3110). Undefined gains are now rejected before the median and `bands` counts the bands the median is over. **Rows production refuses to score** are dropped by a new `partition_scorable/1` and counted in the payload (`summary.unscorable_rows`) and the Markdown header. Worth knowing: this one is latent rather than active — `hrrr_profiles` currently holds **0 NULLs** in `surface_temp_c`/`surface_dewpoint_c` across 89,098,625 rows, so nothing was being fabricated today. It stays because the schema allows it, `PathCompute.build_scoring/6` bails on an empty temperature list, and the fabrication (`humidity 75` plus a T−Td score) is silent when it happens. **Humidity comment** rewritten as suggested — you are right that `nil` falls through to 75 on beneficial bands (every comparison against an atom is false) and only raises on the harmful `nil * penalty`. The T−Td clause gets the same treatment: production raises there, too, so both are documented as guards, not curves. ## Verification (this revision) - `python3 scripts/validate_algo.py` against prod: 74,087 joined rows scored, `unscorable_rows: 0`, report written, `py_compile` clean on both scripts. - The regenerated report is *not* committed here — it moves the `/algo` facts band, which is a separate deliberate step. - `make precommit`: green (4930 passed, 6 skipped, credo/xref/format clean).
fix(algo): make validate_algo.py score what production scores
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
bb9ddc1531
The validator was a hand re-implementation that had drifted from
scorer.ex/band_config.ex/region.ex, so every rho in
docs/algo-reports/validation-*.md measured an algorithm that does not
exist:

- pressure was inverted (88 at low MSLP falling to 30 at high, vs
  production's ascending 30 -> 88), including the delta curve
- score_season omitted the Region multiplier
- time-of-day used a month-only sunrise table and the old fixed 8 pm
  evening cutoff, not BandConfig's latitude/month sunrise + sunset
- rain/humidity/seasonal/refractivity constants were pre-2026-04 values
- weights were not resolved through BandWeights' JSON-first lookup

Rebuilt from the source of truth and re-derived every band table; the
whole port is now value-for-value identical to production (verified
against a dump of the real scorer over every band, factor grid and the
Region boxes).

The baseline table was also degenerate: pers_deltas was literally 0.0,
so rho(persistence) == 0 and 'skill gain' == rho(algorithm), while
climatology and no-skill shared a rank vector. Predictors are now each
scored against the raw distance target, so 'does the score beat
month-of-year alone?' is a direct rho comparison; a constant predictor
reports an undefined rho (null / em dash), never a fabricated 0.0, and
the seasonality-removed view is kept for the algorithm only.

Three factors (sky/wind/rain) have no HRRR column in the corpus, so they
stay pinned to their neutral constants; the report and JSON now state
that share of composite weight explicitly instead of hiding it.

Also aligns the flagged_invalid predicate with the column definition
(boolean NOT NULL DEFAULT false), ports the retired duct/Richardson
inputs so their inertness is proven rather than assumed, and refreshes
the module docstring.
graham force-pushed fix/validator-production-parity from bb9ddc1531
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
to 3ba391b90a
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
2026-09-19 15:07:44 -05:00
Compare
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity Location Issue
🟠 High scripts/validate_algo.py:821 Undefined rho is now emitted as JSON null, which the recalibration gate cannot read
🔵 Suggestion scripts/validate_algo.py:278 Humidity nil comment states the reverse of production's actual behaviour

Reviewed bb9ddc1531af. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `scripts/validate_algo.py:821` | Undefined rho is now emitted as JSON null, which the recalibration gate cannot read | | 🔵 Suggestion | `scripts/validate_algo.py:278` | Humidity nil comment states the reverse of production's actual behaviour | <sub>Reviewed `bb9ddc1531af`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -152,2 +275,4 @@
def score_humidity(abs_hum: float | None, band_cfg: dict) -> int:
# Divergence, deliberate: production has no nil clause here. A nil
# abs_humidity falls through Erlang term ordering (`nil < 4` is true) to 55
First-time contributor

🔵 Suggestion — Humidity nil comment states the reverse of production's actual behaviour

nil is an atom, and Erlang term order puts numbers before atoms, so nil < 4 is false: production's Enum.find falls through all six thresholds and returns humidity_beneficial_default() (75) -- the same value this port returns, so there is no divergence on beneficial bands. The divergence is real only on harmful bands, where nil * penalty raises. As written, the comment promises a 55-vs-75 gap that does not exist and cites a term-ordering rule that runs the other way, which is a trap for whoever re-derives this port next. Rewrite it as: production lands on 75 for the same reason this does (every comparison against nil is false) and raises only on harmful bands.

**🔵 Suggestion — Humidity nil comment states the reverse of production's actual behaviour** `nil` is an atom, and Erlang term order puts numbers *before* atoms, so `nil < 4` is **false**: production's `Enum.find` falls through all six thresholds and returns `humidity_beneficial_default()` (75) -- the same value this port returns, so there is no divergence on beneficial bands. The divergence is real only on harmful bands, where `nil * penalty` raises. As written, the comment promises a 55-vs-75 gap that does not exist and cites a term-ordering rule that runs the other way, which is a trap for whoever re-derives this port next. Rewrite it as: production lands on 75 for the same reason this does (every comparison against `nil` is false) and raises only on harmful bands.
skippy-bot marked this conversation as resolved
@ -441,0 +818,4 @@
return {
"n": n,
"alg_rho": _rho4(alg_rho),
"pers_rho": _rho4(pers_rho),
First-time contributor

🟠 High — Undefined rho is now emitted as JSON null, which the recalibration gate cannot read

_rho4 now preserves None for an undefined rho, so skill_gain (and alg_rho) is null in the JSON whenever the rank vector does not exist: any band with <3 test contacts, or whose test rows all land in one month so the persistence predictor is constant. Two existing readers assume a number and were not updated. scripts/recalibrate.py:939 does info.get("skill_gain", 0.0) > 0 across every band in per_band -- the .get default does not cover an explicit null -- so None > 0 raises TypeError after the validation gate has already run, and the run dies before the weights JSON is written. scripts/recalibrate.py:669-670 has the same shape (float(info.get("alg_rho", 0.0)), guarded only by n >= 50), and lib/microwaveprop/propagation/validation_report.ex does compile-time arithmetic on stats["skill_gain"] for n >= 50 bands, so a null there fails mix compile and drops /algo's facts band; its test does the same median.

Fix: null-guard the readers in this PR. In recalibrate.py skip (if info.get("skill_gain") is not None) rather than default, same for alg_rho; in ValidationReport Enum.reject(&is_nil/1) before the median. Emitting the key only when defined would also dodge it, but null is the honest value -- guard the readers.

**🟠 High — Undefined rho is now emitted as JSON null, which the recalibration gate cannot read** `_rho4` now preserves `None` for an undefined rho, so `skill_gain` (and `alg_rho`) is `null` in the JSON whenever the rank vector does not exist: any band with <3 test contacts, or whose test rows all land in one month so the persistence predictor is constant. Two existing readers assume a number and were not updated. `scripts/recalibrate.py:939` does `info.get("skill_gain", 0.0) > 0` across **every** band in `per_band` -- the `.get` default does not cover an explicit `null` -- so `None > 0` raises `TypeError` after the validation gate has already run, and the run dies before the weights JSON is written. `scripts/recalibrate.py:669-670` has the same shape (`float(info.get("alg_rho", 0.0))`, guarded only by `n >= 50`), and `lib/microwaveprop/propagation/validation_report.ex` does compile-time arithmetic on `stats["skill_gain"]` for `n >= 50` bands, so a `null` there fails `mix compile` and drops `/algo`'s facts band; its test does the same median. Fix: null-guard the readers in this PR. In recalibrate.py skip (`if info.get("skill_gain") is not None`) rather than default, same for `alg_rho`; in ValidationReport `Enum.reject(&is_nil/1)` before the median. Emitting the key only when defined would also dodge it, but `null` is the honest value -- guard the readers.
skippy-bot marked this conversation as resolved
skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity Location Issue
🟠 High scripts/validate_algo.py:825 null skill_gain/alg_rho breaks the two callers this report feeds
🟡 Warning scripts/validate_algo.py:277 Rows production refuses to score are still scored here
🔵 Suggestion scripts/validate_algo.py:353 Evening window matches the unmerged sunset branch, not main's scorer

Full-diff fallback: previous head bb9ddc15 is not in this branch's history (force-push/rebase), and there is no prior review from skippy-bot, so this is a first review of the whole diff.

Reviewed 3ba391b90a9a. Comment skippy review to re-run.

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `scripts/validate_algo.py:825` | null skill_gain/alg_rho breaks the two callers this report feeds | | 🟡 Warning | `scripts/validate_algo.py:277` | Rows production refuses to score are still scored here | | 🔵 Suggestion | `scripts/validate_algo.py:353` | Evening window matches the unmerged sunset branch, not main's scorer | Full-diff fallback: previous head bb9ddc15 is not in this branch's history (force-push/rebase), and there is no prior review from skippy-bot, so this is a first review of the whole diff. <sub>Reviewed `3ba391b90a9a`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -151,6 +274,11 @@ def c_to_f(c: float) -> float:
def score_humidity(abs_hum: float | None, band_cfg: dict) -> int:
# Divergence, deliberate: production has no nil clause here. A nil
First-time contributor

🟡 Warning — Rows production refuses to score are still scored here

The deliberate-divergence note is about humidity, but the blast radius is wider: surface_temp_c and surface_dewpoint_c are nullable on hrrr_profiles, so a joined row with either NULL reaches composite_score with abs_humidity = None and fabricates humidity 75 (the beneficial default, also applied on harmful bands where production's abs_hum * penalty raises) plus score_td_depression(None, None, ...) = 50 on top of the other eight factors. Production never scores those contacts at all: PathCompute.build_scoring/6 returns {nil, nil} when temps or dewpoints is empty (path_compute.ex:369) and Scorer.build_path_conditions/2 returns nil for the same input (scorer.ex:686). So every rho here is computed over rows whose composite production cannot produce, in a direction nobody has measured. Either exclude NULL temp/dewpoint rows in CONTACTS_HRRR_SQL, or count them in the payload the way unavailable_factors now counts pinned weight.

**🟡 Warning — Rows production refuses to score are still scored here** The deliberate-divergence note is about humidity, but the blast radius is wider: `surface_temp_c` and `surface_dewpoint_c` are nullable on `hrrr_profiles`, so a joined row with either NULL reaches `composite_score` with `abs_humidity = None` and fabricates humidity 75 (the beneficial default, also applied on harmful bands where production's `abs_hum * penalty` raises) plus `score_td_depression(None, None, ...) = 50` on top of the other eight factors. Production never scores those contacts at all: `PathCompute.build_scoring/6` returns `{nil, nil}` when `temps` or `dewpoints` is empty (`path_compute.ex:369`) and `Scorer.build_path_conditions/2` returns nil for the same input (`scorer.ex:686`). So every rho here is computed over rows whose composite production cannot produce, in a direction nobody has measured. Either exclude NULL temp/dewpoint rows in `CONTACTS_HRRR_SQL`, or count them in the payload the way `unavailable_factors` now counts pinned weight.
skippy-bot marked this conversation as resolved
@ -177,0 +350,4 @@
return 78 # "Good — inversion eroding"
if -3.0 <= delta < -1.5:
return 82 # "Pre-dawn — inversion building"
if local >= sunset and local - sunset <= 3.0:
First-time contributor

🔵 Suggestion — Evening window matches the unmerged sunset branch, not main's scorer

Checked against the source: this clause, sunrise_hour/2, sunset_hour/2 and the local < sunset guards are value-for-value identical to fix/time-of-day-sunset (PR #12), while lib/microwaveprop/propagation/scorer.ex on main still has the fixed-hour local >= 20 or local <= 1 clause and band_config.ex on main has no sunset_hour/2 at all. If #13 lands before #12 (or #12 is dropped), this file scores an evening window production does not have: in December the sunset-keyed window opens around local 16.5 instead of 20.0, so several evening hours take different factor scores while the report, validation-*.json and the recalibrate gate all read as parity with production. Worth a guard (grep sunset_hour out of band_config.ex at startup and fail loudly) or at minimum the dependency stated in the module docstring at the top, next to the parity claim, rather than only in the PR body and this docstring.

**🔵 Suggestion — Evening window matches the unmerged sunset branch, not main's scorer** Checked against the source: this clause, `sunrise_hour/2`, `sunset_hour/2` and the `local < sunset` guards are value-for-value identical to `fix/time-of-day-sunset` (PR #12), while `lib/microwaveprop/propagation/scorer.ex` on `main` still has the fixed-hour `local >= 20 or local <= 1` clause and `band_config.ex` on `main` has no `sunset_hour/2` at all. If #13 lands before #12 (or #12 is dropped), this file scores an evening window production does not have: in December the sunset-keyed window opens around local 16.5 instead of 20.0, so several evening hours take different factor scores while the report, `validation-*.json` and the recalibrate gate all read as parity with production. Worth a guard (grep `sunset_hour` out of `band_config.ex` at startup and fail loudly) or at minimum the dependency stated in the module docstring at the top, next to the parity claim, rather than only in the PR body and this docstring.
skippy-bot marked this conversation as resolved
@ -441,0 +822,4 @@
"clim_rho": _rho4(clim_rho),
"no_skill_rho": _rho4(ns_rho),
"alg_rho_anomaly": _rho4(alg_rho_anomaly),
"skill_gain": _rho4(skill_gain),
First-time contributor

🟠 High — null skill_gain/alg_rho breaks the two callers this report feeds

_rho4 now publishes null for an undefined rho, and both consumers do raw arithmetic on that key. scripts/recalibrate.py:939 runs info.get("skill_gain", 0.0) > 0 over every band in per_band (no n filter): a band whose test rows fall in a single month makes pers_rho None, so skill_gain is null and None > 0 raises TypeError outside the gate's except (FileNotFoundError, subprocess.TimeoutExpired, RuntimeError) at line 934, aborting recalibrate.py after it has already spent one or two full validation runs. scripts/recalibrate.py:669 has the same shape with float(info.get("alg_rho", 0.0)), and lib/microwaveprop/propagation/validation_report.ex:30 maps stats["skill_gain"] out of the newest validation-*.json at compile time and takes a median: an even count reaches (Enum.at(...) + Enum.at(...)) / 2 (line 47) which raises ArithmeticError on nil, an odd count can return nil into a field typed float(). The corpus already has bands small enough for this (validation-2026-08-17.json has n=4, 6, 21, 25), so it is real data rather than a hypothetical. Fix on either side: filter nil in the consumers, or keep the JSON numeric and publish the undefined state as a separate boolean/flag.

**🟠 High — null skill_gain/alg_rho breaks the two callers this report feeds** `_rho4` now publishes `null` for an undefined rho, and both consumers do raw arithmetic on that key. `scripts/recalibrate.py:939` runs `info.get("skill_gain", 0.0) > 0` over every band in `per_band` (no `n` filter): a band whose test rows fall in a single month makes `pers_rho` None, so `skill_gain` is null and `None > 0` raises TypeError *outside* the gate's `except (FileNotFoundError, subprocess.TimeoutExpired, RuntimeError)` at line 934, aborting `recalibrate.py` after it has already spent one or two full validation runs. `scripts/recalibrate.py:669` has the same shape with `float(info.get("alg_rho", 0.0))`, and `lib/microwaveprop/propagation/validation_report.ex:30` maps `stats["skill_gain"]` out of the newest `validation-*.json` at compile time and takes a median: an even count reaches `(Enum.at(...) + Enum.at(...)) / 2` (line 47) which raises ArithmeticError on nil, an odd count can return nil into a field typed `float()`. The corpus already has bands small enough for this (`validation-2026-08-17.json` has n=4, 6, 21, 25), so it is real data rather than a hypothetical. Fix on either side: filter nil in the consumers, or keep the JSON numeric and publish the undefined state as a separate boolean/flag.
skippy-bot marked this conversation as resolved
First-time contributor

Bookkeeping: this head's review landed concurrently with the one on bb9ddc15, so its JSON-null finding duplicated the earlier high (already open at line 821). I resolved the duplicate inline comment (6318) rather than leave the same defect twice.

New in the 3ba391b9 review and still open: the warning that rows with a NULL HRRR temp/dewpoint are scored here while production bails on them, and the note that the evening-window port tracks fix/time-of-day-sunset (#12), not main's scorer, so the merge order matters.

Bookkeeping: this head's review landed concurrently with the one on `bb9ddc15`, so its JSON-null finding duplicated the earlier high (already open at line 821). I resolved the duplicate inline comment (6318) rather than leave the same defect twice. New in the `3ba391b9` review and still open: the warning that rows with a NULL HRRR temp/dewpoint are scored here while production bails on them, and the note that the evening-window port tracks `fix/time-of-day-sunset` (#12), not `main`'s scorer, so the merge order matters. <!-- skippy-pr-review -->
graham force-pushed fix/validator-production-parity from 3ba391b90a
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
to 0106900882
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
2026-09-19 18:04:47 -05:00
Compare
Author
Owner

Fixed in 01069008 (rebased onto main, so the #12 merge-order note is resolved).

All three findings were real; the null-rho one is worse than "cannot read" — a regenerated report carries six nulls (222 MHz n=752, 432 MHz n=898, 2304/3400/5760/122000 MHz), and sum(... if info.get("skill_gain", 0.0) > 0) raises TypeError on it, aborting recalibrate.py after its two validation runs. validation_regressions float(alg_rho) and the ValidationReport compile-time median had the same defect; the latter was silently wrong rather than loud — 0.2379 over 6 bands where the defined-gain median is 0.086 over 4. All three now skip undefined measurements instead of defaulting them to zero.

The NULL surface temp/dewpoint case is handled by partition_scorable/1 plus a count in the payload. Measured first: hrrr_profiles currently has 0 NULLs in either column across 89,098,625 rows, so it was latent, not active — kept because the schema permits it and the failure is silent.

Humidity comment rewritten as you described, and the T−Td clause too (production raises there as well).

Verification: validator run end-to-end against prod (74,087 rows, unscorable_rows: 0), guarded readers checked against the resulting report, make precommit green (4930 passed). The regenerated report is deliberately not committed — it moves the /algo figure.

skippy review

Fixed in `01069008` (rebased onto `main`, so the #12 merge-order note is resolved). All three findings were real; the null-rho one is worse than "cannot read" — a regenerated report carries six nulls (222 MHz n=752, 432 MHz n=898, 2304/3400/5760/122000 MHz), and `sum(... if info.get("skill_gain", 0.0) > 0)` raises `TypeError` on it, aborting `recalibrate.py` after its two validation runs. `validation_regressions` `float(alg_rho)` and the `ValidationReport` compile-time median had the same defect; the latter was silently wrong rather than loud — 0.2379 over 6 bands where the defined-gain median is 0.086 over 4. All three now skip undefined measurements instead of defaulting them to zero. The NULL surface temp/dewpoint case is handled by `partition_scorable/1` plus a count in the payload. Measured first: `hrrr_profiles` currently has 0 NULLs in either column across 89,098,625 rows, so it was latent, not active — kept because the schema permits it and the failure is silent. Humidity comment rewritten as you described, and the T−Td clause too (production raises there as well). Verification: validator run end-to-end against prod (74,087 rows, `unscorable_rows: 0`), guarded readers checked against the resulting report, `make precommit` green (4930 passed). The regenerated report is deliberately not committed — it moves the `/algo` figure. skippy review
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity Location Issue
🟠 High deps:1 The local deps symlink got committed, pointing at a macOS path
🟡 Warning test/microwaveprop/propagation/validation_report_test.exs:31 The sibling count assertion still expects every n>=50 band, not the defined ones

Resolved all 4 earlier findings (6301, 6302, 6319, 6320) — each is fixed at this head. Previous head 3ba391b9 is not in this branch's history, so this is a full-diff review.

Reviewed 01069008824a. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `deps:1` | The local `deps` symlink got committed, pointing at a macOS path | | 🟡 Warning | `test/microwaveprop/propagation/validation_report_test.exs:31` | The sibling count assertion still expects every n>=50 band, not the defined ones | Resolved all 4 earlier findings (6301, 6302, 6319, 6320) — each is fixed at this head. Previous head 3ba391b9 is not in this branch's history, so this is a full-diff review. <sub>Reviewed `01069008824a`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
deps Outdated
@ -0,0 +1 @@
/Users/graham/dev/ntms/prop/deps
First-time contributor

🟠 High — The local deps symlink got committed, pointing at a macOS path

This adds deps as a symlink to /Users/graham/dev/ntms/prop/deps. .gitignore guards /deps/, which only matches a directory, so a symlink of that name is not ignored and is now tracked (a plain git add -A picks it up). On every machine that is not that Mac the link dangles, and the name being taken breaks anything that needs a real deps directory: mkdir -p deps exits 1 with mkdir: cannot create directory 'deps': File exists (verified against a dangling symlink), which is the first command of the Verify compilation step at .forgejo/workflows/build.yaml:135 — that job runs on every push to main under bash -e, so the step aborts there, and mix deps.get runs into the same taken name. Drop it (git rm --cached deps) and add a bare deps next to /deps/ in .gitignore so the symlink cannot come back.

**🟠 High — The local `deps` symlink got committed, pointing at a macOS path** This adds `deps` as a **symlink to `/Users/graham/dev/ntms/prop/deps`**. `.gitignore` guards `/deps/`, which only matches a *directory*, so a symlink of that name is not ignored and is now tracked (a plain `git add -A` picks it up). On every machine that is not that Mac the link dangles, and the name being taken breaks anything that needs a real `deps` directory: `mkdir -p deps` exits 1 with `mkdir: cannot create directory 'deps': File exists` (verified against a dangling symlink), which is the first command of the `Verify compilation` step at `.forgejo/workflows/build.yaml:135` — that job runs on every push to `main` under `bash -e`, so the step aborts there, and `mix deps.get` runs into the same taken name. Drop it (`git rm --cached deps`) and add a bare `deps` next to `/deps/` in `.gitignore` so the symlink cannot come back.
skippy-bot marked this conversation as resolved
@ -28,6 +28,7 @@ defmodule Microwaveprop.Propagation.ValidationReportTest do
report["per_band"]
|> Enum.filter(fn {_band, v} -> v["n"] >= 50 end)
|> Enum.map(fn {_band, v} -> v["skill_gain"] end)
|> Enum.reject(&is_nil/1)
First-time contributor

🟡 Warning — The sibling count assertion still expects every n>=50 band, not the defined ones

ValidationReport now sets bands: length(@eligible_gains) after the Enum.reject(&is_nil/1) added on this line, so a band with n >= 50 and a null skill_gain no longer counts (validation_report.ex:34, :54). The median test below was given the matching filter, but the count test at line 23 still asserts summary().bands == Enum.count(report["per_band"], fn {_b, v} -> v["n"] >= 50 end) — the pre-PR contract. It passes today only because the embedded report (validation-2026-08-17.json) has no null gain among its six eligible bands; the first regeneration that yields one, which the module docstring calls expected ("a persistence predictor that is constant within the band"), fails the test and takes CI with it. Filter the same way in the count: Enum.count(report["per_band"], fn {_b, v} -> v["n"] >= 50 and is_number(v["skill_gain"]) end).

**🟡 Warning — The sibling count assertion still expects every n>=50 band, not the defined ones** `ValidationReport` now sets `bands: length(@eligible_gains)` *after* the `Enum.reject(&is_nil/1)` added on this line, so a band with `n >= 50` and a `null` `skill_gain` no longer counts (`validation_report.ex:34`, `:54`). The median test below was given the matching filter, but the count test at line 23 still asserts `summary().bands == Enum.count(report["per_band"], fn {_b, v} -> v["n"] >= 50 end)` — the pre-PR contract. It passes today only because the embedded report (`validation-2026-08-17.json`) has no null gain among its six eligible bands; the first regeneration that yields one, which the module docstring calls expected ("a persistence predictor that is constant within the band"), fails the test and takes CI with it. Filter the same way in the count: `Enum.count(report["per_band"], fn {_b, v} -> v["n"] >= 50 and is_number(v["skill_gain"]) end)`.
skippy-bot marked this conversation as resolved
graham force-pushed fix/validator-production-parity from 0106900882
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
to ee1fbcb6a3
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
2026-09-19 18:12:32 -05:00
Compare
Author
Owner

Two housekeeping notes on the follow-up commit, now bdbf0f5d (the SHA in the comment above predates the amend):

  • It had picked up the worktree's deps symlink. .gitignore carried /deps/, and a trailing-slash pattern matches directories only, so the symlink a worktree points at the main checkout's dependency tree was tracked and would have checked out dangling on CI. The pattern is now /deps, so a symlink is ignored too. Nothing else changed.
  • The diff is otherwise what was reviewed: five files, no deps.
Two housekeeping notes on the follow-up commit, now `bdbf0f5d` (the SHA in the comment above predates the amend): - It had picked up the worktree's `deps` symlink. `.gitignore` carried `/deps/`, and a trailing-slash pattern matches directories only, so the symlink a worktree points at the main checkout's dependency tree was tracked and would have checked out dangling on CI. The pattern is now `/deps`, so a symlink is ignored too. Nothing else changed. - The diff is otherwise what was reviewed: five files, no `deps`.
skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity Location Issue
🟡 Warning scripts/recalibrate.py:685 An undefined rho skips the gate on either side, so a candidate that flattens a band is never reverted

Full-diff fallback: previous head 01069008 is not in this branch's history (rebased onto main after #12 merged, so the sunset-relative evening window is now main's, not a dependency). Resolved 1 earlier finding: the committed deps symlink is gone from the tree and /deps now ignores it (ee1fbcb6). 1 still open: the sibling count assertion in validation_report_test.exs. New in ee1fbcb6/bdbf0f5d: the guard on this page. Verified the port value-for-value against band_config.ex/region.ex/scorer.ex on the branch head (23 bands' rain/humidity/seasonal tables, the 8 in-source weight maps, @weights, thresholds, days_of_year, BandWeights' 0.001 tolerance and JSON-first order, the pressure curves, and the CONUS-centre fallback in path_latitude/longitude): no divergence found.

Reviewed ee1fbcb6a30c. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `scripts/recalibrate.py:685` | An undefined rho skips the gate on either side, so a candidate that flattens a band is never reverted | Full-diff fallback: previous head 01069008 is not in this branch's history (rebased onto main after #12 merged, so the sunset-relative evening window is now main's, not a dependency). Resolved 1 earlier finding: the committed `deps` symlink is gone from the tree and `/deps` now ignores it (ee1fbcb6). 1 still open: the sibling count assertion in `validation_report_test.exs`. New in ee1fbcb6/bdbf0f5d: the guard on this page. Verified the port value-for-value against band_config.ex/region.ex/scorer.ex on the branch head (23 bands' rain/humidity/seasonal tables, the 8 in-source weight maps, @weights, thresholds, days_of_year, BandWeights' 0.001 tolerance and JSON-first order, the pressure curves, and the CONUS-centre fallback in path_latitude/longitude): no divergence found. <sub>Reviewed `ee1fbcb6a30c`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -671,0 +682,4 @@
# An undefined ρ on either side is not evidence of a regression — there
# are no two numbers to compare, so the band keeps the candidate's
# weights rather than being reverted on a measurement that does not exist.
if new_rho is None or old_rho is None:
First-time contributor

🟡 Warning — An undefined rho skips the gate on either side, so a candidate that flattens a band is never reverted

Treating null as "no evidence" is right for the current side, but this continue also swallows the candidate side: when the candidate's own alg_rho is null, the band exits validation_regressions as "nothing to compare" instead of "this weights payload cannot be measured". With n >= 50 already enforced above, the only way a candidate band lands on null is zero rank variance in its composite (spearman_rho returns None when den_x == 0), i.e. the candidate weights made the score a constant across the band's test rows. Scoring mass on a pinned factor does exactly that (sky/wind/rain are fixed constants in this corpus, so {"rain": 1.0, rest 0.0} gives every row 100 and still parses: parse_json_weights only checks the 10 keys are present and sum to ~1.0), and the old code caught that case by accident, because the pre-PR validator returned 0.0 for zero variance and 0.0 < old_rho - REGRESSION_EPS reverted the band. Now the payload ships with that band's weights unvalidated. Split the sides: keep continue when old_rho is None (the measurement never existed), but when new_rho is None and old_rho is not None raise the regression instead of skipping it, so a candidate that destroys its own rank signal fails the gate loudly. The second pass (final_regressions against current) inherits the same hole.

**🟡 Warning — An undefined rho skips the gate on either side, so a candidate that flattens a band is never reverted** Treating `null` as "no evidence" is right for the `current` side, but this `continue` also swallows the **candidate** side: when the candidate's own `alg_rho` is `null`, the band exits `validation_regressions` as "nothing to compare" instead of "this weights payload cannot be measured". With `n >= 50` already enforced above, the only way a candidate band lands on `null` is zero rank variance in its composite (`spearman_rho` returns None when `den_x == 0`), i.e. the candidate weights made the score a constant across the band's test rows. Scoring mass on a pinned factor does exactly that (sky/wind/rain are fixed constants in this corpus, so `{"rain": 1.0, rest 0.0}` gives every row 100 and still parses: `parse_json_weights` only checks the 10 keys are present and sum to ~1.0), and the old code caught that case by accident, because the pre-PR validator returned `0.0` for zero variance and `0.0 < old_rho - REGRESSION_EPS` reverted the band. Now the payload ships with that band's weights unvalidated. Split the sides: keep `continue` when `old_rho is None` (the measurement never existed), but when `new_rho is None and old_rho is not None` raise the regression instead of skipping it, so a candidate that destroys its own rank signal fails the gate loudly. The second pass (`final_regressions` against `current`) inherits the same hole.
skippy-bot marked this conversation as resolved
fix(algo): revert bands where candidate holdout rho collapses to undefined
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
80ee08e1d0
validation_regressions() previously skipped a band whenever either side's
alg_rho was None, treating an undefined candidate rho the same as an
undefined baseline. But once n >= VALIDATION_MIN_N is enforced, a None
candidate rho only happens when the candidate's weights make the composite
score constant across the band's test rows (zero rank variance) — a real
regression, not "no evidence". Gate on it against any defined baseline,
same as a numeric drop.
First-time contributor

Resolved 1 of 2 earlier findings: the gate hole in scripts/recalibrate.py is closed at 80ee08e1 — an undefined candidate ρ is now treated as a regression against a defined baseline, and only an undefined baseline still skips. 1 still open: the sibling count assertion in test/microwaveprop/propagation/validation_report_test.exs:20.

Nothing new in ee1fbcb6..80ee08e1. Verified the reversion can only fire on a candidate-caused undefined ρ: spearman_rho returns None for n<3 (blocked by the n >= 50 guard here), or zero variance on either side, and the distance target is the same rows in both runs, so old_rho defined + new_rho null implies the candidate composite had zero rank variance. The -math.inf sort key, the undefined print and the second-pass final_regressions all handle the None row.

Reviewed 80ee08e1d05b. Comment skippy review to re-run.

Resolved 1 of 2 earlier findings: the gate hole in `scripts/recalibrate.py` is closed at `80ee08e1` — an undefined *candidate* ρ is now treated as a regression against a defined baseline, and only an undefined *baseline* still skips. 1 still open: the sibling count assertion in `test/microwaveprop/propagation/validation_report_test.exs:20`. Nothing new in `ee1fbcb6..80ee08e1`. Verified the reversion can only fire on a candidate-caused undefined ρ: `spearman_rho` returns None for n<3 (blocked by the `n >= 50` guard here), or zero variance on either side, and the distance target is the same rows in both runs, so `old_rho` defined + `new_rho` null implies the candidate composite had zero rank variance. The `-math.inf` sort key, the `undefined` print and the second-pass `final_regressions` all handle the None row. <sub>Reviewed `80ee08e1d05b`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
Author
Owner

Fixed in 80ee08e1.

Undefined candidate rho skips the gate on either side (6482, blocking). Split the two None sides in validation_regressions: old_rho is None still means "no baseline, keep the candidate" (unchanged), but new_rho is None no longer takes the same continue. Since n >= VALIDATION_MIN_N is already enforced above, the only way the candidate's own rho comes back undefined is zero rank variance in its composite score — the candidate weights collapsed the band's rank signal — and that's a real regression against any defined baseline, not "no evidence." It's now gated (new_rho is None or new_rho < old_rho - REGRESSION_EPS) even though it has no numeric magnitude. The second pass (final_regressions against current) shares this function, so it's fixed too.

Downstream fixes for the now-possible None in tuple position 3: rows.sort treats an undefined new_rho as the worst possible regression (-math.inf, sorts first) instead of raising on None - float; both f-string formatters (validate_before_write's kept-weights print and the final_regressions failure message) render "undefined" instead of applying :+.4f to None. Return/local type annotations updated to float | None.

Verification (psycopg isn't installed in this sandbox): stubbed sys.modules['psycopg'] so the real module imports, then ran validation_regressions against three synthetic bands — one with candidate rho undefined and a defined baseline (now correctly included, ('A', 50, 0.55, None) — this is the bug), one with an undefined baseline (correctly still excluded), one with a normal numeric regression (unchanged, ('C', 50, 0.3, 0.1)). rows.sort didn't raise with the None present and put the undefined row first. ast.parse confirms valid syntax.

skippy review

Fixed in `80ee08e1`. **Undefined candidate rho skips the gate on either side (6482, blocking).** Split the two `None` sides in `validation_regressions`: `old_rho is None` still means "no baseline, keep the candidate" (unchanged), but `new_rho is None` no longer takes the same `continue`. Since `n >= VALIDATION_MIN_N` is already enforced above, the only way the candidate's own rho comes back undefined is zero rank variance in its composite score — the candidate weights collapsed the band's rank signal — and that's a real regression against any defined baseline, not "no evidence." It's now gated (`new_rho is None or new_rho < old_rho - REGRESSION_EPS`) even though it has no numeric magnitude. The second pass (`final_regressions` against `current`) shares this function, so it's fixed too. Downstream fixes for the now-possible `None` in tuple position 3: `rows.sort` treats an undefined new_rho as the worst possible regression (`-math.inf`, sorts first) instead of raising on `None - float`; both f-string formatters (`validate_before_write`'s kept-weights print and the `final_regressions` failure message) render `"undefined"` instead of applying `:+.4f` to `None`. Return/local type annotations updated to `float | None`. Verification (psycopg isn't installed in this sandbox): stubbed `sys.modules['psycopg']` so the real module imports, then ran `validation_regressions` against three synthetic bands — one with candidate rho undefined and a defined baseline (now correctly included, `('A', 50, 0.55, None)` — this is the bug), one with an undefined baseline (correctly still excluded), one with a normal numeric regression (unchanged, `('C', 50, 0.3, 0.1)`). `rows.sort` didn't raise with the `None` present and put the undefined row first. `ast.parse` confirms valid syntax. skippy review
test(algo): match eligible-band count to the skill-gain filter
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
30670b83d4
The bands count and median were computed from different filters:
median rejected null skill_gain, count only checked n >= 50. It only
passed because the embedded fixture has no n>=50 band with a null
skill_gain yet. Share one eligible_bands/1 filter between both
assertions and add a regression case with a synthetic per_band map
proving a high-n, null-skill_gain band is excluded from the count.
Author
Owner

Fixed in 30670b83. Second of the two open findings closed (6482, the recalibrate.py rho gate, was already fixed and confirmed at 80ee08e1 per skippy-bot's 7593).

Sibling count assertion didn't share the median's filter (line 20, blocking). ValidationReport's @eligible_gains filters on n >= 50 and rejects nil skill_gain before the median and the bands count are both derived from its length/1. The test's median assertion already mirrored that (filter, map, Enum.reject(&is_nil/1)), but the count assertion only checked n >= 50 — it agreed with the real module purely because the embedded validation-2026-08-17.json fixture has zero null-skill_gain bands among its n >= 50 bands right now. The first regenerated report with one (the module's own docstring calls that expected — a persistence predictor constant within the band) would have made this test assert the pre-PR count while ValidationReport.summary().bands correctly excludes it, and it would fail without anyone having touched validation_report.ex.

Pulled both assertions onto one eligible_bands/1 test helper that matches @eligible_gains's filter exactly, so the median and count assertions can't drift again. Added a case that builds a synthetic per_band map with one n: 200, skill_gain: 0.5 band and one n: 60, skill_gain: nil band and asserts eligible_bands/1 returns only the first — confirmed this fails against the pre-fix filter (length 2, not 1) and passes against the fix.

Verification: mix compile --no-deps-check --warnings-as-errors clean, mix test test/microwaveprop/propagation/validation_report_test.exs — 7 passed, mix format --check-formatted clean on the touched file. Only the test file changed; validation_report.ex was already correct.

skippy review

Fixed in `30670b83`. Second of the two open findings closed (6482, the `recalibrate.py` rho gate, was already fixed and confirmed at `80ee08e1` per skippy-bot's 7593). **Sibling count assertion didn't share the median's filter (line 20, blocking).** `ValidationReport`'s `@eligible_gains` filters on `n >= 50` *and* rejects `nil` skill_gain before the median and the `bands` count are both derived from its `length/1`. The test's median assertion already mirrored that (filter, map, `Enum.reject(&is_nil/1)`), but the count assertion only checked `n >= 50` — it agreed with the real module purely because the embedded `validation-2026-08-17.json` fixture has zero null-skill_gain bands among its `n >= 50` bands right now. The first regenerated report with one (the module's own docstring calls that expected — a persistence predictor constant within the band) would have made this test assert the pre-PR count while `ValidationReport.summary().bands` correctly excludes it, and it would fail without anyone having touched `validation_report.ex`. Pulled both assertions onto one `eligible_bands/1` test helper that matches `@eligible_gains`'s filter exactly, so the median and count assertions can't drift again. Added a case that builds a synthetic `per_band` map with one `n: 200, skill_gain: 0.5` band and one `n: 60, skill_gain: nil` band and asserts `eligible_bands/1` returns only the first — confirmed this fails against the pre-fix filter (`length` 2, not 1) and passes against the fix. Verification: `mix compile --no-deps-check --warnings-as-errors` clean, `mix test test/microwaveprop/propagation/validation_report_test.exs` — 7 passed, `mix format --check-formatted` clean on the touched file. Only the test file changed; `validation_report.ex` was already correct. skippy review
First-time contributor

Resolved 1 of 1 earlier findings: the sibling count assertion in test/microwaveprop/propagation/validation_report_test.exs now filters through eligible_bands/1 (n >= 50 and a non-nil skill_gain), which is the set ValidationReport's @eligible_gains actually counts (validation_report.ex:30-35) - the mirror is exact.

Nothing new in 80ee08e1..30670b83 (test-only commit; the added helper self-test is redundant but harmless). 0 findings open.

Reviewed 30670b83d429. Comment skippy review to re-run.

Resolved 1 of 1 earlier findings: the sibling count assertion in `test/microwaveprop/propagation/validation_report_test.exs` now filters through `eligible_bands/1` (`n >= 50` **and** a non-nil `skill_gain`), which is the set `ValidationReport`'s `@eligible_gains` actually counts (`validation_report.ex:30-35`) - the mirror is exact. Nothing new in `80ee08e1..30670b83` (test-only commit; the added helper self-test is redundant but harmless). 0 findings open. <sub>Reviewed `30670b83d429`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
First-time contributor

Resolved 1 of 1 earlier finding: the eligible-band count in validation_report_test.exs now derives from the same eligible_bands/1 helper as the median, so it matches ValidationReport's n >= 50 + reject(is_nil) filter.

Nothing new in 80ee08e1..30670b83 (test-only commit; scripts/validate_algo.py and scripts/recalibrate.py are unchanged from the heads already reviewed). No blocking findings remain.

Reviewed 30670b83d429.

Resolved 1 of 1 earlier finding: the eligible-band count in `validation_report_test.exs` now derives from the same `eligible_bands/1` helper as the median, so it matches `ValidationReport`'s `n >= 50` + `reject(is_nil)` filter. Nothing new in `80ee08e1..30670b83` (test-only commit; `scripts/validate_algo.py` and `scripts/recalibrate.py` are unchanged from the heads already reviewed). No blocking findings remain. <sub>Reviewed `30670b83d429`.</sub> <!-- skippy-pr-review -->
graham merged commit abf6933250 into main 2026-09-20 10:55:32 -05:00
graham deleted branch fix/validator-production-parity 2026-09-20 10:55:32 -05:00
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!13
No description provided.