Make the algorithm validator score what production scores #13
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/validator-production-parity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
scripts/validate_algo.pyscored a different algorithm than production, so every rho indocs/algo-reports/validation-*.md— and the accept/revert gate inscripts/recalibrate.pythat 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_pressurewas inverted: 88 at<980 mbfalling 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 thathrrr_profileshas no previous-pressure column so it is unreachable from this report.score_seasonomitted the Region multiplier (Region.for_point/2+Region.seasonal_adjustment/2). The join now selectspos1latitude, and a missing coordinate falls back to the CONUS centre exactly where production's path helpers do.score_time_of_dayused a month-only sunrise table and the fixedlocal >= 20evening cutoff. It now portsBandConfig.sunrise_hour/2+sunset_hour/2and the sunset-relative clause order.band_config.ex/region.ex(24/47 GHzrain_k/rain_alphawere still the pre-2026-04 values; humidity, refractivity, seasonal and region boxes re-checked), and weights now resolve through the same JSON-first order asBandWeights.lookup/1.flagged_invalidpredicate aligned with the column definition (boolean NOT NULL DEFAULT false).Baseline table
pers_deltaswas literally0.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 (nullin JSON,—in the table, never0.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_profilesand 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
python3 -m py_compileclean.Merge order (one dependency)
The time-of-day port mirrors the scorer after
fix/time-of-day-sunset(the sunset-relative evening window fromBandConfig.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, nosunsetargument), 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_URLand 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-fixsum(1 for info in per_band.values() if info.get("skill_gain", 0.0) > 0)raisesTypeError: '>' not supported between instances of 'NoneType' and 'int'— i.e.recalibrate.pydies after the gate has spent its two validation runs — andvalidation_regressionsraises the same way onfloat(alg_rho). Both now skip an undefined measurement instead of defaulting it to zero (no two numbers, no regression verdict), counted by a newskill_gain_counts/1.ValidationReporthad the same defect at compile time: nils sorted in with the gains (numbers sort before atoms), so an odd eligible count putnilinto afloat()field —Float.round/2on the/algofacts 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 andbandscounts the bands the median is over.Rows production refuses to score are dropped by a new
partition_scorable/1and counted in the payload (summary.unscorable_rows) and the Markdown header. Worth knowing: this one is latent rather than active —hrrr_profilescurrently holds 0 NULLs insurface_temp_c/surface_dewpoint_cacross 89,098,625 rows, so nothing was being fabricated today. It stays because the schema allows it,PathCompute.build_scoring/6bails on an empty temperature list, and the fabrication (humidity 75plus a T−Td score) is silent when it happens.Humidity comment rewritten as suggested — you are right that
nilfalls through to 75 on beneficial bands (every comparison against an atom is false) and only raises on the harmfulnil * 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.pyagainst prod: 74,087 joined rows scored,unscorable_rows: 0, report written,py_compileclean on both scripts./algofacts band, which is a separate deliberate step.make precommit: green (4930 passed, 6 skipped, credo/xref/format clean).bb9ddc15313ba391b90a🤖 Skippy PR review
2 findings — 1 blocking before merge.
scripts/validate_algo.py:821scripts/validate_algo.py:278Reviewed
bb9ddc1531af. Commentskippy reviewto re-run.@ -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🔵 Suggestion — Humidity nil comment states the reverse of production's actual behaviour
nilis an atom, and Erlang term order puts numbers before atoms, sonil < 4is false: production'sEnum.findfalls through all six thresholds and returnshumidity_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, wherenil * penaltyraises. 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 againstnilis false) and raises only on harmful bands.@ -441,0 +818,4 @@return {"n": n,"alg_rho": _rho4(alg_rho),"pers_rho": _rho4(pers_rho),🟠 High — Undefined rho is now emitted as JSON null, which the recalibration gate cannot read
_rho4now preservesNonefor an undefined rho, soskill_gain(andalg_rho) isnullin 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:939doesinfo.get("skill_gain", 0.0) > 0across every band inper_band-- the.getdefault does not cover an explicitnull-- soNone > 0raisesTypeErrorafter the validation gate has already run, and the run dies before the weights JSON is written.scripts/recalibrate.py:669-670has the same shape (float(info.get("alg_rho", 0.0)), guarded only byn >= 50), andlib/microwaveprop/propagation/validation_report.exdoes compile-time arithmetic onstats["skill_gain"]forn >= 50bands, so anullthere failsmix compileand 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 foralg_rho; in ValidationReportEnum.reject(&is_nil/1)before the median. Emitting the key only when defined would also dodge it, butnullis the honest value -- guard the readers.🤖 Skippy PR review
3 findings — 2 blocking before merge.
scripts/validate_algo.py:825scripts/validate_algo.py:277scripts/validate_algo.py:353Full-diff fallback: previous head
bb9ddc15is 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. Commentskippy reviewto re-run.@ -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🟡 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_candsurface_dewpoint_care nullable onhrrr_profiles, so a joined row with either NULL reachescomposite_scorewithabs_humidity = Noneand fabricates humidity 75 (the beneficial default, also applied on harmful bands where production'sabs_hum * penaltyraises) plusscore_td_depression(None, None, ...) = 50on top of the other eight factors. Production never scores those contacts at all:PathCompute.build_scoring/6returns{nil, nil}whentempsordewpointsis empty (path_compute.ex:369) andScorer.build_path_conditions/2returns 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 inCONTACTS_HRRR_SQL, or count them in the payload the wayunavailable_factorsnow counts pinned weight.@ -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:🔵 Suggestion — Evening window matches the unmerged sunset branch, not main's scorer
Checked against the source: this clause,
sunrise_hour/2,sunset_hour/2and thelocal < sunsetguards are value-for-value identical tofix/time-of-day-sunset(PR #12), whilelib/microwaveprop/propagation/scorer.exonmainstill has the fixed-hourlocal >= 20 or local <= 1clause andband_config.exonmainhas nosunset_hour/2at 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-*.jsonand the recalibrate gate all read as parity with production. Worth a guard (grepsunset_hourout ofband_config.exat 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.@ -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),🟠 High — null skill_gain/alg_rho breaks the two callers this report feeds
_rho4now publishesnullfor an undefined rho, and both consumers do raw arithmetic on that key.scripts/recalibrate.py:939runsinfo.get("skill_gain", 0.0) > 0over every band inper_band(nonfilter): a band whose test rows fall in a single month makespers_rhoNone, soskill_gainis null andNone > 0raises TypeError outside the gate'sexcept (FileNotFoundError, subprocess.TimeoutExpired, RuntimeError)at line 934, abortingrecalibrate.pyafter it has already spent one or two full validation runs.scripts/recalibrate.py:669has the same shape withfloat(info.get("alg_rho", 0.0)), andlib/microwaveprop/propagation/validation_report.ex:30mapsstats["skill_gain"]out of the newestvalidation-*.jsonat 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 typedfloat(). The corpus already has bands small enough for this (validation-2026-08-17.jsonhas 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.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
3ba391b9review 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 tracksfix/time-of-day-sunset(#12), notmain's scorer, so the merge order matters.3ba391b90a0106900882Fixed in
01069008(rebased ontomain, 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)raisesTypeErroron it, abortingrecalibrate.pyafter its two validation runs.validation_regressionsfloat(alg_rho)and theValidationReportcompile-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/1plus a count in the payload. Measured first:hrrr_profilescurrently 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 precommitgreen (4930 passed). The regenerated report is deliberately not committed — it moves the/algofigure.skippy review
🤖 Skippy PR review
2 findings — 2 blocking before merge.
deps:1depssymlink got committed, pointing at a macOS pathtest/microwaveprop/propagation/validation_report_test.exs:31Resolved all 4 earlier findings (6301, 6302, 6319, 6320) — each is fixed at this head. Previous head
3ba391b9is not in this branch's history, so this is a full-diff review.Reviewed
01069008824a. Commentskippy reviewto re-run.@ -0,0 +1 @@/Users/graham/dev/ntms/prop/deps🟠 High — The local
depssymlink got committed, pointing at a macOS pathThis adds
depsas a symlink to/Users/graham/dev/ntms/prop/deps..gitignoreguards/deps/, which only matches a directory, so a symlink of that name is not ignored and is now tracked (a plaingit add -Apicks it up). On every machine that is not that Mac the link dangles, and the name being taken breaks anything that needs a realdepsdirectory:mkdir -p depsexits 1 withmkdir: cannot create directory 'deps': File exists(verified against a dangling symlink), which is the first command of theVerify compilationstep at.forgejo/workflows/build.yaml:135— that job runs on every push tomainunderbash -e, so the step aborts there, andmix deps.getruns into the same taken name. Drop it (git rm --cached deps) and add a baredepsnext to/deps/in.gitignoreso the symlink cannot come back.@ -28,6 +28,7 @@ defmodule Microwaveprop.Propagation.ValidationReportTest doreport["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)🟡 Warning — The sibling count assertion still expects every n>=50 band, not the defined ones
ValidationReportnow setsbands: length(@eligible_gains)after theEnum.reject(&is_nil/1)added on this line, so a band withn >= 50and anullskill_gainno longer counts (validation_report.ex:34,:54). The median test below was given the matching filter, but the count test at line 23 still assertssummary().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).0106900882ee1fbcb6a3Two housekeeping notes on the follow-up commit, now
bdbf0f5d(the SHA in the comment above predates the amend):depssymlink..gitignorecarried/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.deps.🤖 Skippy PR review
1 finding — 1 blocking before merge.
scripts/recalibrate.py:685Full-diff fallback: previous head
01069008is 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 committeddepssymlink is gone from the tree and/depsnow ignores it (ee1fbcb6). 1 still open: the sibling count assertion invalidation_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. Commentskippy reviewto re-run.@ -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:🟡 Warning — An undefined rho skips the gate on either side, so a candidate that flattens a band is never reverted
Treating
nullas "no evidence" is right for thecurrentside, but thiscontinuealso swallows the candidate side: when the candidate's ownalg_rhoisnull, the band exitsvalidation_regressionsas "nothing to compare" instead of "this weights payload cannot be measured". Withn >= 50already enforced above, the only way a candidate band lands onnullis zero rank variance in its composite (spearman_rhoreturns None whenden_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_weightsonly 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 returned0.0for zero variance and0.0 < old_rho - REGRESSION_EPSreverted the band. Now the payload ships with that band's weights unvalidated. Split the sides: keepcontinuewhenold_rho is None(the measurement never existed), but whennew_rho is None and old_rho is not Noneraise the regression instead of skipping it, so a candidate that destroys its own rank signal fails the gate loudly. The second pass (final_regressionsagainstcurrent) inherits the same hole.Resolved 1 of 2 earlier findings: the gate hole in
scripts/recalibrate.pyis closed at80ee08e1— 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 intest/microwaveprop/propagation/validation_report_test.exs:20.Nothing new in
ee1fbcb6..80ee08e1. Verified the reversion can only fire on a candidate-caused undefined ρ:spearman_rhoreturns None for n<3 (blocked by then >= 50guard here), or zero variance on either side, and the distance target is the same rows in both runs, soold_rhodefined +new_rhonull implies the candidate composite had zero rank variance. The-math.infsort key, theundefinedprint and the second-passfinal_regressionsall handle the None row.Reviewed
80ee08e1d05b. Commentskippy reviewto re-run.Fixed in
80ee08e1.Undefined candidate rho skips the gate on either side (6482, blocking). Split the two
Nonesides invalidation_regressions:old_rho is Nonestill means "no baseline, keep the candidate" (unchanged), butnew_rho is Noneno longer takes the samecontinue. Sincen >= VALIDATION_MIN_Nis 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_regressionsagainstcurrent) shares this function, so it's fixed too.Downstream fixes for the now-possible
Nonein tuple position 3:rows.sorttreats an undefined new_rho as the worst possible regression (-math.inf, sorts first) instead of raising onNone - float; both f-string formatters (validate_before_write's kept-weights print and thefinal_regressionsfailure message) render"undefined"instead of applying:+.4ftoNone. Return/local type annotations updated tofloat | None.Verification (psycopg isn't installed in this sandbox): stubbed
sys.modules['psycopg']so the real module imports, then ranvalidation_regressionsagainst 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.sortdidn't raise with theNonepresent and put the undefined row first.ast.parseconfirms valid syntax.skippy review
Fixed in
30670b83. Second of the two open findings closed (6482, therecalibrate.pyrho gate, was already fixed and confirmed at80ee08e1per skippy-bot's 7593).Sibling count assertion didn't share the median's filter (line 20, blocking).
ValidationReport's@eligible_gainsfilters onn >= 50and rejectsnilskill_gain before the median and thebandscount are both derived from itslength/1. The test's median assertion already mirrored that (filter, map,Enum.reject(&is_nil/1)), but the count assertion only checkedn >= 50— it agreed with the real module purely because the embeddedvalidation-2026-08-17.jsonfixture has zero null-skill_gain bands among itsn >= 50bands 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 whileValidationReport.summary().bandscorrectly excludes it, and it would fail without anyone having touchedvalidation_report.ex.Pulled both assertions onto one
eligible_bands/1test helper that matches@eligible_gains's filter exactly, so the median and count assertions can't drift again. Added a case that builds a syntheticper_bandmap with onen: 200, skill_gain: 0.5band and onen: 60, skill_gain: nilband and assertseligible_bands/1returns only the first — confirmed this fails against the pre-fix filter (length2, not 1) and passes against the fix.Verification:
mix compile --no-deps-check --warnings-as-errorsclean,mix test test/microwaveprop/propagation/validation_report_test.exs— 7 passed,mix format --check-formattedclean on the touched file. Only the test file changed;validation_report.exwas already correct.skippy review
Resolved 1 of 1 earlier findings: the sibling count assertion in
test/microwaveprop/propagation/validation_report_test.exsnow filters througheligible_bands/1(n >= 50and a non-nilskill_gain), which is the setValidationReport's@eligible_gainsactually 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. Commentskippy reviewto re-run.Resolved 1 of 1 earlier finding: the eligible-band count in
validation_report_test.exsnow derives from the sameeligible_bands/1helper as the median, so it matchesValidationReport'sn >= 50+reject(is_nil)filter.Nothing new in
80ee08e1..30670b83(test-only commit;scripts/validate_algo.pyandscripts/recalibrate.pyare unchanged from the heads already reviewed). No blocking findings remain.Reviewed
30670b83d429.