Stop fabricating a surface dewpoint when the observation is missing #15

graham · 2026-09-19T15:04:31-05:00

graham commented

2026-09-19 15:04:31 -05:00

What

SoundingParams.derive/1 reported a surface dewpoint that was never measured (surface_dewpoint_c: sfc["dwpc"] || sfc["tmpc"] - 10). The entry filter dropped every level without a dewpoint, so the real surface was discarded and the first dewpoint-bearing level (925 hPa) was silently reported as "the surface" — a profile whose surface has no dewpoint reported surface_dewpoint_c == 12.0, reproduced before the fix. (tmp/bugs.md §B, P2; the root cause is the level filter, not the dead fallback.)

derive/1 keeps every level with pres/tmpc/hght (nil-dewpoint levels included), still requires at least three levels carrying a dewpoint, and reports the true surface's dewpoint as nil.
Scorer.score_td_depression/3 and Scorer.absolute_humidity/2 gained explicit nil clauses (neutral 50 / nil) rather than raising ArithmeticError on the now-possible missing value.
compute_k_index no longer substitutes -30.0 for missing 850/700 hPa dewpoints; the index is nil when it is genuinely undefined, and every consumer already renders nil as "no value".
PathCompute.build_scoring/2 logs when it returns {nil, nil} (profile/temp/dewpoint counts plus the reason) instead of dropping the failure silently. No scoring behaviour changed; the loss-budget 7.5 g/m³ humidity fallback is untouched.
Rust needed no change: rust/prop_grid_rs/src/sounding_params.rs already models the dewpoint as Option, and the quantities it derives are unchanged, so the Elixir/Rust mirror stays exact.

Verification

Reproducer: with the pre-fix lib/ restored and the new assertions in place, 5 assertions fail (surface_dewpoint_c 12.0 instead of nil, k_index 37.0 instead of nil, ArithmeticError: 80.0 - nil); with the fix, 77 passed — green on three seeds.
Consumer sweep (weather, grid, workers, PSKR sampler, propagation, PathCompute, Skew-T/contact/weather LiveViews, API controllers, features): 572 additional tests green.
Every consumer of surface_dewpoint_c / dewpoint_f / k_index was audited for an explicit nil path; the list is in the commit context.
Golden fixture untouched, Elixir↔Rust parity test passes. mix credo --strict: no issues; mix format --check-formatted on touched files: clean.
CHANGELOG entry added.

Flagged, not fixed

Microwaveprop.Propagation.Recalibrator (dev/test-only) still invents values — absolute_humidity(...) else 10.0 and score_td_depression(temp_f || 70, dewpoint_f || 60, ...).

build_path_conditions/2 averages each field over the profiles that observed it, one bucket per field, so a cell with no surface dewpoint still moves the T−Td pair through its temperature (the path_integrated_conditions/2 test "averages only the profiles that observed a dewpoint" pins exactly that). Long-standing bucket behaviour rather than something this PR introduced — pairing per profile would move scores on mixed paths and change the golden fixture, so it is now documented at the decision point instead of changed here.

Review follow-up (`98637821`)

Rebased onto main (#12 is in), which also clears the merge conflict.

Precipitable water (High) — fixed. The pair guard is unreachable while the entry filter drops dewpoint-less levels, and once they are kept it deletes the whole layer between the two dewpoint-bearing neighbours. compute_precipitable_water/1 now pairs only the dewpoint-bearing levels, so the trapezoid spans the gap and only the missing levels' own spans are excluded. New tests pin the no_upper_dewpoints column at 29.2 mm (was 11.8 mm) and assert it equals the same sounding with the dewpoint-less levels deleted. Against main, the value is unchanged — that filter produced the same arithmetic before this PR, so this restores the pre-PR number rather than inventing a third one.

Surface refractivity datum (Warning) — fixed on both sides. derive/1 reported N from the lowest level that has a dewpoint; with the surface now allowed to have none, one row carried 1013 hPa surface fields beside a 925 hPa N. It is now nil unless the observed surface reported a dewpoint, matching surface_dewpoint_c, and rust/prop_grid_rs/src/sounding_params.rs::surface_refractivity returns None for the same input instead of the lowest usable level. Both Elixir call sites share one refractivity_n/3 so the profile and the surface datum cannot drift.

Blast radius was measured before touching the Rust side:

hrrr_profiles: 0 of 132,613 rows in a day (2026-09-18) have a surface level without a dewpoint in the stored profile, so the Rust change is inert on current HRRR data rather than blanking the map layer.
soundings: 18,709 of 29,474 rows are already nil in both surface_dewpoint_c and surface_refractivity — zero rows carry one without the other, so no stored pairing changes.

Bucket averaging (Suggestion) — documented, not changed. You called it pre-existing and defensible; I agree, and changing it would move scores for mixed paths and regenerate the golden fixture, which is a scoring decision rather than a review fix. build_path_conditions/2 now says so at the point of decision, and the PR body lists it under "Flagged, not fixed".

Verification (this revision)

mix test test/microwaveprop/weather test/microwaveprop/propagation → 1316 passed.
make precommit → green (4940 passed, 6 skipped; credo, format, xref clean).
cargo clippy --all-targets -- -D warnings clean; cargo test --release green — 261 lib tests plus scorer_golden parity, including the two new Rust datum tests.

## What `SoundingParams.derive/1` reported a surface dewpoint that was never measured (`surface_dewpoint_c: sfc["dwpc"] || sfc["tmpc"] - 10`). The entry filter dropped every level without a dewpoint, so the real surface was discarded and the first dewpoint-bearing level (925 hPa) was silently reported as "the surface" — a profile whose surface has no dewpoint reported `surface_dewpoint_c == 12.0`, reproduced before the fix. (`tmp/bugs.md` §B, P2; the root cause is the level filter, not the dead fallback.) - `derive/1` keeps every level with `pres`/`tmpc`/`hght` (nil-dewpoint levels included), still requires at least three levels carrying a dewpoint, and reports the true surface's dewpoint as `nil`. - `Scorer.score_td_depression/3` and `Scorer.absolute_humidity/2` gained explicit nil clauses (neutral 50 / nil) rather than raising `ArithmeticError` on the now-possible missing value. - `compute_k_index` no longer substitutes `-30.0` for missing 850/700 hPa dewpoints; the index is nil when it is genuinely undefined, and every consumer already renders nil as "no value". - `PathCompute.build_scoring/2` logs when it returns `{nil, nil}` (profile/temp/dewpoint counts plus the reason) instead of dropping the failure silently. No scoring behaviour changed; the loss-budget 7.5 g/m³ humidity fallback is untouched. - Rust needed no change: `rust/prop_grid_rs/src/sounding_params.rs` already models the dewpoint as `Option`, and the quantities it derives are unchanged, so the Elixir/Rust mirror stays exact. ## Verification - Reproducer: with the pre-fix `lib/` restored and the new assertions in place, 5 assertions fail (`surface_dewpoint_c` 12.0 instead of nil, `k_index` 37.0 instead of nil, `ArithmeticError: 80.0 - nil`); with the fix, 77 passed — green on three seeds. - Consumer sweep (weather, grid, workers, PSKR sampler, propagation, PathCompute, Skew-T/contact/weather LiveViews, API controllers, features): 572 additional tests green. - Every consumer of `surface_dewpoint_c` / `dewpoint_f` / `k_index` was audited for an explicit nil path; the list is in the commit context. - Golden fixture untouched, Elixir↔Rust parity test passes. `mix credo --strict`: no issues; `mix format --check-formatted` on touched files: clean. - CHANGELOG entry added. ## Flagged, not fixed `Microwaveprop.Propagation.Recalibrator` (dev/test-only) still invents values — `absolute_humidity(...) else 10.0` and `score_td_depression(temp_f || 70, dewpoint_f || 60, ...)`. `build_path_conditions/2` averages each field over the profiles that observed it, one bucket per field, so a cell with no surface dewpoint still moves the T−Td pair through its temperature (the `path_integrated_conditions/2` test "averages only the profiles that observed a dewpoint" pins exactly that). Long-standing bucket behaviour rather than something this PR introduced — pairing per profile would move scores on mixed paths and change the golden fixture, so it is now documented at the decision point instead of changed here. ## Review follow-up (98637821) Rebased onto `main` (#12 is in), which also clears the merge conflict. **Precipitable water (High) — fixed.** The pair guard is unreachable while the entry filter drops dewpoint-less levels, and once they are kept it deletes the whole layer between the two dewpoint-bearing neighbours. `compute_precipitable_water/1` now pairs only the dewpoint-bearing levels, so the trapezoid spans the gap and only the missing levels' own spans are excluded. New tests pin the `no_upper_dewpoints` column at **29.2 mm** (was 11.8 mm) and assert it equals the same sounding with the dewpoint-less levels deleted. Against `main`, the value is unchanged — that filter produced the same arithmetic before this PR, so this restores the pre-PR number rather than inventing a third one. **Surface refractivity datum (Warning) — fixed on both sides.** `derive/1` reported N from the lowest level that has a dewpoint; with the surface now allowed to have none, one row carried 1013 hPa surface fields beside a 925 hPa N. It is now nil unless the *observed* surface reported a dewpoint, matching `surface_dewpoint_c`, and `rust/prop_grid_rs/src/sounding_params.rs::surface_refractivity` returns `None` for the same input instead of the lowest usable level. Both Elixir call sites share one `refractivity_n/3` so the profile and the surface datum cannot drift. Blast radius was measured before touching the Rust side: - `hrrr_profiles`: 0 of **132,613** rows in a day (2026-09-18) have a surface level without a dewpoint in the stored profile, so the Rust change is inert on current HRRR data rather than blanking the map layer. - `soundings`: **18,709 of 29,474** rows are already nil in both `surface_dewpoint_c` and `surface_refractivity` — zero rows carry one without the other, so no stored pairing changes. **Bucket averaging (Suggestion) — documented, not changed.** You called it pre-existing and defensible; I agree, and changing it would move scores for mixed paths and regenerate the golden fixture, which is a scoring decision rather than a review fix. `build_path_conditions/2` now says so at the point of decision, and the PR body lists it under "Flagged, not fixed". ## Verification (this revision) - `mix test test/microwaveprop/weather test/microwaveprop/propagation` → 1316 passed. - `make precommit` → green (4940 passed, 6 skipped; credo, format, xref clean). - `cargo clippy --all-targets -- -D warnings` clean; `cargo test --release` green — 261 lib tests plus `scorer_golden` parity, including the two new Rust datum tests.

graham added 1 commit

2026-09-19 15:04:31 -05:00

fix(sounding): stop fabricating a surface dewpoint when the observation is missing

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

81dacb1997

SoundingParams.derive/1 returned `sfc["dwpc"] || sfc["tmpc"] - 10` while its
entry filter discarded every level without a dewpoint, so a sounding whose
surface dewpoint was never observed either invented a 10 °C depression or
silently reported the 925 mb level as "the surface".

Keep levels that carry no dewpoint, report `surface_dewpoint_c: nil`, and make
the nil path explicit: the T−Td factor scores 50 (unknown) for a missing
temperature or dewpoint, absolute_humidity/2 returns nil, and the K-index is
nil rather than a -30 °C placeholder when the 850/700 dewpoints are missing.
PathCompute now logs why a path with no surface dewpoint has no score.

skippy-bot reviewed

2026-09-19 15:19:50 -05:00

skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/microwaveprop/weather/sounding_params.ex:43`	Keeping nil-dewpoint levels silently understates precipitable water
🟡 Warning	`lib/microwaveprop/weather/sounding_params.ex:127`	surface_refractivity now describes a different level than the rest of the surface fields
🔵 Suggestion	`test/microwaveprop/propagation/scorer_test.exs:203`	The path aggregation this test pins still mixes temperature and dewpoint populations

_{Reviewed 81dacb1997a4. Comment skippy review to re-run.}

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/microwaveprop/weather/sounding_params.ex:43` | Keeping nil-dewpoint levels silently understates precipitable water | | 🟡 Warning | `lib/microwaveprop/weather/sounding_params.ex:127` | surface_refractivity now describes a different level than the rest of the surface fields | | 🔵 Suggestion | `test/microwaveprop/propagation/scorer_test.exs:203` | The path aggregation this test pins still mixes temperature and dewpoint populations | Reviewed `81dacb1997a4`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/sounding_params.ex Outdated

					
				@ -36,3 +43,1 @@

				      |> Enum.filter(fn p ->

				        p["pres"] != nil and p["tmpc"] != nil and p["hght"] != nil and p["dwpc"] != nil

				      end)

				      |> Enum.filter(fn p -> p["pres"] != nil and p["tmpc"] != nil and p["hght"] != nil end)

🟠 High — Keeping nil-dewpoint levels silently understates precipitable water

compute_precipitable_water/1 still reduces over adjacent pairs of sorted and skips any pair whose endpoint has no dewpoint, but that guard was unreachable before this diff: the old entry filter guaranteed every retained level had a dewpoint, so dewpoint-bearing levels formed pairs across the dropped ones (a trapezoid over the whole column). Now both pairs around a missing-dewpoint level are skipped and the entire layer span between its dewpoint-bearing neighbours contributes nothing.

On this PR's own no_upper_dewpoints fixture (dwpc at 1000/900/600/500, absent at 850/700) PW falls from 29.2 mm to 11.8 mm, a 60% dry bias that lands in soundings.precipitable_water_mm and in the PWAT rail on /skewt. Fix: reduce over the dewpoint-bearing levels (Enum.filter(sorted, fn p -> p["dwpc"] != nil end)), or keep the trapezoid across the gap, so only the missing level's own span is excluded. The do_derive/1 comment added here says PW filters the dewpoint-less levels out; it does not, it drops whole pairs.

**🟠 High — Keeping nil-dewpoint levels silently understates precipitable water** `compute_precipitable_water/1` still reduces over adjacent pairs of `sorted` and skips any pair whose endpoint has no dewpoint, but that guard was unreachable before this diff: the old entry filter guaranteed every retained level had a dewpoint, so dewpoint-bearing levels formed pairs across the dropped ones (a trapezoid over the whole column). Now both pairs around a missing-dewpoint level are skipped and the entire layer span between its dewpoint-bearing neighbours contributes nothing. On this PR's own `no_upper_dewpoints` fixture (dwpc at 1000/900/600/500, absent at 850/700) PW falls from 29.2 mm to 11.8 mm, a 60% dry bias that lands in `soundings.precipitable_water_mm` and in the PWAT rail on /skewt. Fix: reduce over the dewpoint-bearing levels (`Enum.filter(sorted, fn p -> p["dwpc"] != nil end)`), or keep the trapezoid across the gap, so only the missing level's own span is excluded. The `do_derive/1` comment added here says PW filters the dewpoint-less levels out; it does not, it drops whole pairs.

skippy-bot marked this conversation as resolved

lib/microwaveprop/weather/sounding_params.ex Outdated

					
				@ -115,6 +124,9 @@ defmodule Microwaveprop.Weather.SoundingParams do

				  end

				  defp compute_refractivity_profile(sorted, sfc_hght) do

				    # Only levels with a dewpoint contribute N — refractivity is a

🟡 Warning — surface_refractivity now describes a different level than the rest of the surface fields

sfc_n is the first entry of the dewpoint-filtered refractivity profile, so with the surface now allowed to have no dewpoint it is taken from whatever level does. In this PR's fixture that is 925 hPa (about 630 m AGL), while surface_pressure_mb, surface_temp_c and surface_dewpoint_c in the same result describe the true 1013 hPa surface: one row, two datums. That value is stored in hrrr_profiles.surface_refractivity and published as the surface_refractivity weather-map layer.

Either return nil when hd(sorted)["dwpc"] is nil, or rename the field so the datum is explicit. rust/prop_grid_rs/src/sounding_params.rs::surface_refractivity/1 picks the same level today, so the Elixir/Rust mirror has to change together.

**🟡 Warning — surface_refractivity now describes a different level than the rest of the surface fields** `sfc_n` is the first entry of the dewpoint-filtered refractivity profile, so with the surface now allowed to have no dewpoint it is taken from whatever level does. In this PR's fixture that is 925 hPa (about 630 m AGL), while `surface_pressure_mb`, `surface_temp_c` and `surface_dewpoint_c` in the same result describe the true 1013 hPa surface: one row, two datums. That value is stored in `hrrr_profiles.surface_refractivity` and published as the `surface_refractivity` weather-map layer. Either return nil when `hd(sorted)["dwpc"]` is nil, or rename the field so the datum is explicit. `rust/prop_grid_rs/src/sounding_params.rs::surface_refractivity/1` picks the same level today, so the Elixir/Rust mirror has to change together.

skippy-bot marked this conversation as resolved

test/microwaveprop/propagation/scorer_test.exs Outdated

					
				@ -146,0 +200,4 @@

				      cond_map = Scorer.path_integrated_conditions(profiles, @contact)

				      # Temperature still averages over both cells (25 and 15 → 20 °C).

				      assert_in_delta cond_map.temp_f, 68.0, 0.1

🔵 Suggestion — The path aggregation this test pins still mixes temperature and dewpoint populations

This asserts temperature averaged over both cells (25 and 15) against a dewpoint averaged over one (5), so the pair the band factors score is 20 C / 5 C: a 15 C depression that neither cell reported. A dewpoint-less cell therefore still bends the T-Td and humidity factors through its temperature, which is the half of the fabrication the new CHANGELOG entry claims is gone.

Pre-existing behaviour, so accepting it is defensible, but if the intent is that a missing dewpoint contributes nothing to humidity, drop those profiles from the temperature average too (or average per-profile pairs) and update this assertion.

**🔵 Suggestion — The path aggregation this test pins still mixes temperature and dewpoint populations** This asserts temperature averaged over both cells (25 and 15) against a dewpoint averaged over one (5), so the pair the band factors score is 20 C / 5 C: a 15 C depression that neither cell reported. A dewpoint-less cell therefore still bends the T-Td and humidity factors through its temperature, which is the half of the fabrication the new CHANGELOG entry claims is gone. Pre-existing behaviour, so accepting it is defensible, but if the intent is that a missing dewpoint contributes nothing to humidity, drop those profiles from the temperature average too (or average per-profile pairs) and update this assertion.

skippy-bot marked this conversation as resolved

graham referenced this pull request from a commit

2026-09-19 18:11:39 -05:00

fix(sounding): span dewpoint gaps in PW, and put surface N on the surface datum

graham force-pushed fix/surface-dewpoint-no-fabrication from 81dacb1997

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to 9863782152

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

2026-09-19 18:11:39 -05:00

Compare

graham commented

2026-09-19 18:11:47 -05:00

Fixed in 98637821 (rebased onto main; the conflict is gone).

PW: you were right, and the fixture is the one you named. With the dewpoint-less levels retained, both pairs around each of them are dropped, so the 900→600 hPa layer vanished: 29.2 mm → 11.8 mm. compute_precipitable_water/1 now pairs only the dewpoint-bearing levels, which is both the correct trapezoid and the pre-#15 arithmetic (on main the entry filter removed those levels first, so the if lower/upper != nil guard was dead there — exactly as you said). New assertions: PW is 29.2 mm for the gapped profile and equal to the same sounding with the gap levels deleted.

Surface refractivity: nil at the true surface, Elixir and Rust together. derive/1 now reports N only when the observed surface reported a dewpoint; sounding_params.rs::surface_refractivity returns None rather than the lowest level that carries one, and both Elixir call sites share one refractivity_n/3. Your mixed_dwpc fixture assertion is inverted accordingly (surface_refractivity == nil).

I measured the Rust side's cost before changing it: 0 of 132,613 hrrr_profiles rows in a recent day have a surface level without a dewpoint, so the HRRR-written column is unchanged in practice; and in soundings, 18,709 of 29,474 rows are already nil in both surface_dewpoint_c and surface_refractivity, with zero rows carrying one without the other. The planes.rs "fallback for when derive yields nil" comment is still true for every cell the corpus actually produces.

The bucket-averaging suggestion: declined as a change, recorded as a decision — build_path_conditions/2 now documents that a dewpoint-less cell still moves T−Td through its temperature, and the PR body lists it under "Flagged, not fixed". Making it per-profile pairing would move scores on mixed paths and regenerate the golden fixture, which belongs in its own PR with its own validation.

Verification: 1316 weather/propagation tests green, make precommit green (4940 passed), clippy clean, cargo test --release green including scorer_golden.

Fixed in `98637821` (rebased onto `main`; the conflict is gone). **PW: you were right, and the fixture is the one you named.** With the dewpoint-less levels retained, both pairs around each of them are dropped, so the 900→600 hPa layer vanished: 29.2 mm → 11.8 mm. `compute_precipitable_water/1` now pairs only the dewpoint-bearing levels, which is both the correct trapezoid and the pre-#15 arithmetic (on `main` the entry filter removed those levels first, so the `if lower/upper != nil` guard was dead there — exactly as you said). New assertions: PW is 29.2 mm for the gapped profile and equal to the same sounding with the gap levels deleted. **Surface refractivity: nil at the true surface, Elixir and Rust together.** `derive/1` now reports N only when the observed surface reported a dewpoint; `sounding_params.rs::surface_refractivity` returns `None` rather than the lowest level that carries one, and both Elixir call sites share one `refractivity_n/3`. Your `mixed_dwpc` fixture assertion is inverted accordingly (`surface_refractivity == nil`). I measured the Rust side's cost before changing it: 0 of 132,613 `hrrr_profiles` rows in a recent day have a surface level without a dewpoint, so the HRRR-written column is unchanged in practice; and in `soundings`, 18,709 of 29,474 rows are already nil in both `surface_dewpoint_c` and `surface_refractivity`, with zero rows carrying one without the other. The `planes.rs` "fallback for when derive yields nil" comment is still true for every cell the corpus actually produces. **The bucket-averaging suggestion**: declined as a change, recorded as a decision — `build_path_conditions/2` now documents that a dewpoint-less cell still moves T−Td through its temperature, and the PR body lists it under "Flagged, not fixed". Making it per-profile pairing would move scores on mixed paths and regenerate the golden fixture, which belongs in its own PR with its own validation. Verification: 1316 weather/propagation tests green, `make precommit` green (4940 passed), clippy clean, `cargo test --release` green including `scorer_golden`.

graham referenced this pull request

2026-09-19 18:11:54 -05:00

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

graham commented

2026-09-19 18:11:54 -05:00

Merge-order note: #13 also adds a ### entry under ## 2026-09-19 in CHANGELOG.md, so the two will conflict textually at the same insertion point — keep both entries. #14 touches scripts/recalibrate.py and #15 does not, so those two are independent. Happy to rebase whichever lands last.

skippy review

Merge-order note: #13 also adds a `###` entry under `## 2026-09-19` in `CHANGELOG.md`, so the two will conflict textually at the same insertion point — keep both entries. #14 touches `scripts/recalibrate.py` and #15 does not, so those two are independent. Happy to rebase whichever lands last. skippy review

graham referenced this pull request from a commit

2026-09-19 18:12:35 -05:00

fix(sounding): span dewpoint gaps in PW, and put surface N on the surface datum

graham force-pushed fix/surface-dewpoint-no-fabrication from 9863782152

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to 6a7252a1b1

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

2026-09-19 18:12:35 -05:00

Compare

graham commented

2026-09-19 18:12:53 -05:00

Housekeeping: the follow-up commit is now 6a7252a1 (the SHA above predates an amend that dropped the worktree deps symlink from the index). Same content otherwise — four files, no deps.

Housekeeping: the follow-up commit is now `6a7252a1` (the SHA above predates an amend that dropped the worktree `deps` symlink from the index). Same content otherwise — four files, no `deps`.

skippy-bot reviewed

2026-09-19 18:15:11 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`deps:1`	Committed symlink `deps` points into your Mac's filesystem
🟡 Warning	`lib/microwaveprop/weather/sounding_params.ex:106`	nil `surface_refractivity` is now a normal value, but the backfill uses NULL as "not derived yet"

Findings outside the commentable diff

🟠 High — deps:1 — Committed symlink deps points into your Mac's filesystem
This commit adds a mode-120000 symlink at the repo root whose target is /Users/graham/dev/ntms/prop/deps. .gitignore cannot stop it: the rule is /deps/, which only matches a real directory, so a symlink named deps (which is not a directory to git) slips through git add -A. On every checkout that is not your machine (CI runner, Docker build context, a second box) deps is a dangling symlink, and nothing can be created or written under it: verified that mkdir -p deps/foo and writing deps/x both fail with ENOENT/"File exists" on a dangling symlink, which is exactly what mix deps.get and compilation do. Fix: git rm --cached deps and change the ignore rule to deps (no trailing slash) or add /deps as well.

Force-pushed since 81dacb1997a4 (that head is gone from the branch), so this is the full PR diff again. Resolved all 3 earlier findings: PW now spans the dewpoint gaps instead of dropping whole pairs (29.2 mm on the fixture), surface N is taken from the true surface or nil, and the mixed-population path average is now an explicit, documented decision pinned by a test. 0 earlier findings still open.

_{Reviewed 986378215286. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `deps:1` | Committed symlink `deps` points into your Mac's filesystem | | 🟡 Warning | `lib/microwaveprop/weather/sounding_params.ex:106` | nil `surface_refractivity` is now a normal value, but the backfill uses NULL as "not derived yet" | #### Findings outside the commentable diff - **🟠 High — `deps:1` — Committed symlink `deps` points into your Mac's filesystem** This commit adds a mode-120000 symlink at the repo root whose target is `/Users/graham/dev/ntms/prop/deps`. `.gitignore` cannot stop it: the rule is `/deps/`, which only matches a real directory, so a symlink named `deps` (which is not a directory to git) slips through `git add -A`. On every checkout that is not your machine (CI runner, Docker build context, a second box) `deps` is a dangling symlink, and nothing can be created or written under it: verified that `mkdir -p deps/foo` and writing `deps/x` both fail with ENOENT/"File exists" on a dangling symlink, which is exactly what `mix deps.get` and compilation do. Fix: `git rm --cached deps` and change the ignore rule to `deps` (no trailing slash) or add `/deps` as well. Force-pushed since `81dacb1997a4` (that head is gone from the branch), so this is the full PR diff again. Resolved all 3 earlier findings: PW now spans the dewpoint gaps instead of dropping whole pairs (29.2 mm on the fixture), surface N is taken from the true surface or nil, and the mixed-population path average is now an explicit, documented decision pinned by a test. 0 earlier findings still open. Reviewed `986378215286`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/sounding_params.ex Outdated

					
				@ -93,3 +106,1 @@

				      case refract_profile do

				        [first | _] -> first.n

				        _ -> nil

				      if sfc["dwpc"] != nil do

🟡 Warning — nil surface_refractivity is now a normal value, but the backfill uses NULL as "not derived yet"

Making surface_refractivity nil when the surface level has no dewpoint (and persisting the same None from Rust) turns NULL from "never derived" into a legitimate stored result. Hrrr.backfill_hrrr_scalars/1 still keys on it: backfill_hrrr_batch/1 selects where is_nil(h.surface_refractivity) (lib/microwaveprop/weather/hrrr.ex:223), updates it with whatever derive/1 returns, and the caller loops until a batch returns 0 rows. A row whose surface genuinely has no dewpoint is updated to NULL, stays matched, and comes back in the next batch, so the loop no longer terminates early: every invocation now walks the full max_batches (default 1000 batches of 500) re-deriving and re-updating the same rows instead of stopping when the work is done. Bound the selection so it can converge, e.g. where: is_nil(h.surface_refractivity) and h.inserted_at < ^cutoff for the shipped-without-scalars era, or add a scalars_derived_at column and key on that.

**🟡 Warning — nil `surface_refractivity` is now a normal value, but the backfill uses NULL as "not derived yet"** Making `surface_refractivity` nil when the surface level has no dewpoint (and persisting the same None from Rust) turns NULL from "never derived" into a legitimate stored result. `Hrrr.backfill_hrrr_scalars/1` still keys on it: `backfill_hrrr_batch/1` selects `where is_nil(h.surface_refractivity)` (lib/microwaveprop/weather/hrrr.ex:223), updates it with whatever `derive/1` returns, and the caller loops until a batch returns 0 rows. A row whose surface genuinely has no dewpoint is updated to NULL, stays matched, and comes back in the next batch, so the loop no longer terminates early: every invocation now walks the full `max_batches` (default 1000 batches of 500) re-deriving and re-updating the same rows instead of stopping when the work is done. Bound the selection so it can converge, e.g. `where: is_nil(h.surface_refractivity) and h.inserted_at < ^cutoff` for the shipped-without-scalars era, or add a `scalars_derived_at` column and key on that.

skippy-bot marked this conversation as resolved

skippy-bot reviewed

2026-09-19 18:15:13 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity	Location	Issue
🟡 Warning	`deps:1`	A symlink to your local Mac checkout got committed as the repo's `deps` directory
🟡 Warning	`lib/microwaveprop/weather/sounding_params.ex:107`	NULL surface N is now also the backfill's "not derived yet" marker, so those rows never leave its queue

Findings outside the commentable diff

🟡 Warning — deps:1 — A symlink to your local Mac checkout got committed as the repo's deps directory
This lands deps as a symlink whose target is /Users/graham/dev/ntms/prop/deps, an absolute path that exists on no other machine. .gitignore has /deps/ with a trailing slash, which only matches a directory, so the symlink walked straight past it. Every other checkout (the Linux box, CI) now gets a dangling deps entry in exactly the place Mix reads and writes its dependency tree, so mix deps.get / mix compile cannot use deps/ there. It is also unrelated to this PR's change. git rm --cached deps and add deps (no slash) to .gitignore.

Re-review at 98637821 (full PR diff; the earlier review's inline comments are anchored on the pre-rebase head 81dacb19). Resolved 2 of 3 earlier findings, both fixed in this commit: the precipitable-water trapezoid now pairs only dewpoint-bearing levels (29.2 mm, matching the gap-deleted sounding), and surface N is nil on the surface datum in Elixir and Rust. 1 still open: the path T-Td pairing, now documented as deliberate bucket behaviour. 2 new findings below.

_{Reviewed 986378215286. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `deps:1` | A symlink to your local Mac checkout got committed as the repo's `deps` directory | | 🟡 Warning | `lib/microwaveprop/weather/sounding_params.ex:107` | NULL surface N is now also the backfill's "not derived yet" marker, so those rows never leave its queue | #### Findings outside the commentable diff - **🟡 Warning — `deps:1` — A symlink to your local Mac checkout got committed as the repo's `deps` directory** This lands `deps` as a symlink whose target is `/Users/graham/dev/ntms/prop/deps`, an absolute path that exists on no other machine. `.gitignore` has `/deps/` with a trailing slash, which only matches a directory, so the symlink walked straight past it. Every other checkout (the Linux box, CI) now gets a dangling `deps` entry in exactly the place Mix reads and writes its dependency tree, so `mix deps.get` / `mix compile` cannot use `deps/` there. It is also unrelated to this PR's change. `git rm --cached deps` and add `deps` (no slash) to `.gitignore`. Re-review at 98637821 (full PR diff; the earlier review's inline comments are anchored on the pre-rebase head 81dacb19). Resolved 2 of 3 earlier findings, both fixed in this commit: the precipitable-water trapezoid now pairs only dewpoint-bearing levels (29.2 mm, matching the gap-deleted sounding), and surface N is nil on the surface datum in Elixir and Rust. 1 still open: the path T-Td pairing, now documented as deliberate bucket behaviour. 2 new findings below. Reviewed `986378215286`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/sounding_params.ex Outdated

					
				@ -94,2 +106,2 @@

				        [first | _] -> first.n

				        _ -> nil

				      if sfc["dwpc"] != nil do

				        refractivity_n(sfc["pres"], sfc["tmpc"], sfc["dwpc"])

🟡 Warning — NULL surface N is now also the backfill's "not derived yet" marker, so those rows never leave its queue

surface_refractivity is legitimately nil for a derived row whenever the surface level has no dewpoint, but Hrrr.backfill_hrrr_scalars/1 uses where is_nil(h.surface_refractivity) as its todo filter and halts only when a batch returns 0 updates, writing the derived value straight back (SET surface_refractivity = v.surface_refractivity). A row updated to NULL is re-selected on the next pass, so one such row keeps the loop running all max_batches (1000 batches x 500 rows of select + update) and it never converges. Before this change derive/1 could only return a map with a non-nil surface N — the refractivity profile was non-empty whenever the >=3-dewpoint guard passed — so NULL reliably meant "not derived yet" (rows where derive/1 returns nil were already stuck). Give the backfill its own completion marker (derived_at timestamp) instead of overloading the value column.

**🟡 Warning — NULL surface N is now also the backfill's "not derived yet" marker, so those rows never leave its queue** `surface_refractivity` is legitimately nil for a derived row whenever the surface level has no dewpoint, but `Hrrr.backfill_hrrr_scalars/1` uses `where is_nil(h.surface_refractivity)` as its todo filter and halts only when a batch returns 0 updates, writing the derived value straight back (`SET surface_refractivity = v.surface_refractivity`). A row updated to NULL is re-selected on the next pass, so one such row keeps the loop running all `max_batches` (1000 batches x 500 rows of select + update) and it never converges. Before this change `derive/1` could only return a map with a non-nil surface N — the refractivity profile was non-empty whenever the >=3-dewpoint guard passed — so NULL reliably meant "not derived yet" (rows where `derive/1` returns nil were already stuck). Give the backfill its own completion marker (`derived_at` timestamp) instead of overloading the value column.

skippy-bot marked this conversation as resolved

skippy-bot reviewed

2026-09-19 18:16:19 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`lib/microwaveprop/weather/sounding_params.ex:106`	The NULL-refractivity backfill can no longer converge

Follow-up on 6a7252a: 2 of 3 earlier findings fixed and resolved (PW now spans the dewpoint gaps - 11.8 -> 29.2 mm, verified arithmetically against the fixture; surface N moved to the observed-surface datum on both the Elixir and Rust sides); the third resolved as consciously documented in build_path_conditions/2. Nothing blocking is open.

_{Reviewed 6a7252a1b1bf. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `lib/microwaveprop/weather/sounding_params.ex:106` | The NULL-refractivity backfill can no longer converge | Follow-up on 6a7252a: 2 of 3 earlier findings fixed and resolved (PW now spans the dewpoint gaps - 11.8 -> 29.2 mm, verified arithmetically against the fixture; surface N moved to the observed-surface datum on both the Elixir and Rust sides); the third resolved as consciously documented in build_path_conditions/2. Nothing blocking is open. Reviewed `6a7252a1b1bf`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/sounding_params.ex Outdated

					
				@ -93,3 +106,1 @@

				      case refract_profile do

				        [first | _] -> first.n

				        _ -> nil

				      if sfc["dwpc"] != nil do

🔵 Suggestion — The NULL-refractivity backfill can no longer converge

With the datum now on the observed surface, a dewpoint-less surface reports surface_refractivity: nil - and Hrrr.backfill_hrrr_scalars/1 uses that NULL as its work queue: where([h], is_nil(h.surface_refractivity)), batched, halting only when a pass derives nothing (hrrr.ex:212-226). Such a row now does derive (a map comes back, so it is counted in length(updates)) but writes NULL again, so it is re-selected on every pass: the loop re-updates the same rows and bumps their updated_at for all max_batches (default 1000) instead of halting, which is the convergence the docstring promises. Inert on today's data (no hrrr_profiles row has a dewpoint-less surface), but those rows are exactly what this PR makes legitimate.

Fix: keep rows the derivation cannot fill out of the selection - also require a dewpoint at the highest-pressure level, or track attempted ids - so backfill_hrrr_batch/1 can return 0.

**🔵 Suggestion — The NULL-refractivity backfill can no longer converge** With the datum now on the observed surface, a dewpoint-less surface reports `surface_refractivity: nil` - and `Hrrr.backfill_hrrr_scalars/1` uses that NULL as its work queue: `where([h], is_nil(h.surface_refractivity))`, batched, halting only when a pass derives nothing (`hrrr.ex:212-226`). Such a row now *does* derive (a map comes back, so it is counted in `length(updates)`) but writes NULL again, so it is re-selected on every pass: the loop re-updates the same rows and bumps their `updated_at` for all `max_batches` (default 1000) instead of halting, which is the convergence the docstring promises. Inert on today's data (no `hrrr_profiles` row has a dewpoint-less surface), but those rows are exactly what this PR makes legitimate. Fix: keep rows the derivation cannot fill out of the selection - also require a dewpoint at the highest-pressure level, or track attempted ids - so `backfill_hrrr_batch/1` can return 0.

skippy-bot marked this conversation as resolved

skippy-bot commented

2026-09-19 18:17:32 -05:00

First-time contributor

Bookkeeping correction, then the state of this PR.

Two parallel review runs posted while this one was reading prior, so the note at the bottom of this run's review ("nothing blocking is open") is wrong, and its suggestion (6480) duplicated them. 6471 and 6480 were resolved as duplicates of 6469, which stands: backfill_hrrr_scalars/1 keys its work queue on surface_refractivity IS NULL, so a row whose surface has no dewpoint is derived, written back as NULL, and re-selected on every pass. That open warning is what this check is red for.
The deps symlink finding in reviews 621/622 is moot now. deps is not in the tree at 6a7252a1, nor in main; it existed only in the superseded head 986378215286. Nothing to do.
Earlier findings 6308/6309/6310 stay resolved: precipitable water spans the dewpoint gaps (11.8 -> 29.2 mm on the no_upper_dewpoints fixture, verified against the same sounding with those levels deleted), surface N sits on the observed-surface datum in both sounding_params.ex and its Rust mirror, and the mixed-population path average is now an explicit, documented decision.

Bookkeeping correction, then the state of this PR. - Two parallel review runs posted while this one was reading `prior`, so the note at the bottom of this run's review ("nothing blocking is open") is wrong, and its suggestion (6480) duplicated them. 6471 and 6480 were resolved as duplicates of **6469**, which stands: `backfill_hrrr_scalars/1` keys its work queue on `surface_refractivity IS NULL`, so a row whose surface has no dewpoint is derived, written back as NULL, and re-selected on every pass. That open warning is what this check is red for. - The `deps` symlink finding in reviews 621/622 is moot now. `deps` is not in the tree at `6a7252a1`, nor in `main`; it existed only in the superseded head `986378215286`. Nothing to do. - Earlier findings 6308/6309/6310 stay resolved: precipitable water spans the dewpoint gaps (11.8 -> 29.2 mm on the `no_upper_dewpoints` fixture, verified against the same sounding with those levels deleted), surface N sits on the observed-surface datum in both `sounding_params.ex` and its Rust mirror, and the mixed-population path average is now an explicit, documented decision.

graham added 1 commit

2026-09-20 10:25:31 -05:00

fix(hrrr): halt backfill on ducting_detected, not just surface_refractivity

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

50e7e99810

backfill_hrrr_batch/1 selected its work queue with
`is_nil(h.surface_refractivity)`. Since a row's derived surface can
legitimately have no dewpoint (and therefore no surface N), such a
row now writes surface_refractivity back as NULL forever and gets
re-selected, re-derived, and re-updated by every subsequent batch
call — the loop never converges early on data containing such rows.

do_derive/1 always sets a concrete ducting_detected boolean whenever
it derives a row at all, so ducting_detected transitions from NULL to
non-NULL on the row's first pass regardless of surface_refractivity.
Require both columns NULL in the WHERE clause so that transition is
what marks the row as done.

Also add explicit ::float8/::boolean/::jsonb[]/::timestamp casts to
the raw VALUES(...) placeholders in build_values_row/3: with a
single-row batch (e.g. the last remaining row, or a small batch_size)
Postgres can't infer an untyped NULL placeholder's type from context
and defaults it to text, so the UPDATE raised a datatype_mismatch
error the moment a batch's only row had a legitimately-nil
surface_refractivity — exactly the case this fix targets.

graham commented

2026-09-20 10:26:52 -05:00

Fixed in 50e7e998.

Convergence sentinel. do_derive/1 always sets ducting_detected to a concrete boolean (ducts != []) whenever it derives at all, regardless of whether surface_refractivity comes back nil — so a row transitions ducting_detected from NULL to non-NULL on its first pass no matter what. backfill_hrrr_batch/1's WHERE clause now requires both is_nil(h.surface_refractivity) and is_nil(h.ducting_detected), so a row with a legitimately-nil surface N still leaves the selection after one pass instead of being re-derived and re-updated on every subsequent batch forever.

Second bug found while writing the required test. The raw UPDATE ... FROM (VALUES ...) in build_values_row/3 had no explicit type cast on the surface_refractivity/min_refractivity_gradient/ducting_detected/duct_characteristics placeholders. A batch whose only row has an all-NULL surface_refractivity — exactly the scenario this PR makes legitimate — leaves Postgres unable to infer the untyped NULL parameter's type, defaults it to text, and the UPDATE raises datatype_mismatch instead of writing NULL. Added explicit ::float8/::float8/::boolean/::jsonb[]/::timestamp casts (jsonb[] matches the actual duct_characteristics column type per the partition migration). This would have crashed real backfill runs on the same input this PR introduces, so it's fixed alongside rather than filed separately.

New test: inserts one profile with a dewpoint-less surface (3 other levels carry a dewpoint, derive/1's minimum), asserts the first backfill_hrrr_scalars/1 call returns 1 and the row now has surface_refractivity: nil with a real ducting_detected boolean, then asserts a second call returns 0.

Verification: reverted just the WHERE-clause fix (keeping the SQL-cast fix so the test could run at all) and reran — the single row was re-derived and re-updated on every one of 5 batches instead of halting after the first, reproducing the finding directly. Restored the fix; test/microwaveprop/weather/untested_functions_test.exs green (59 passed); mix format clean on both touched files.

skippy review

Fixed in `50e7e998`. **Convergence sentinel.** `do_derive/1` always sets `ducting_detected` to a concrete boolean (`ducts != []`) whenever it derives at all, regardless of whether `surface_refractivity` comes back nil — so a row transitions `ducting_detected` from NULL to non-NULL on its first pass no matter what. `backfill_hrrr_batch/1`'s WHERE clause now requires both `is_nil(h.surface_refractivity) and is_nil(h.ducting_detected)`, so a row with a legitimately-nil surface N still leaves the selection after one pass instead of being re-derived and re-updated on every subsequent batch forever. **Second bug found while writing the required test.** The raw `UPDATE ... FROM (VALUES ...)` in `build_values_row/3` had no explicit type cast on the `surface_refractivity`/`min_refractivity_gradient`/`ducting_detected`/`duct_characteristics` placeholders. A batch whose only row has an all-NULL `surface_refractivity` — exactly the scenario this PR makes legitimate — leaves Postgres unable to infer the untyped NULL parameter's type, defaults it to `text`, and the UPDATE raises `datatype_mismatch` instead of writing NULL. Added explicit `::float8`/`::float8`/`::boolean`/`::jsonb[]`/`::timestamp` casts (`jsonb[]` matches the actual `duct_characteristics` column type per the partition migration). This would have crashed real backfill runs on the same input this PR introduces, so it's fixed alongside rather than filed separately. New test: inserts one profile with a dewpoint-less surface (3 other levels carry a dewpoint, `derive/1`'s minimum), asserts the first `backfill_hrrr_scalars/1` call returns 1 and the row now has `surface_refractivity: nil` with a real `ducting_detected` boolean, then asserts a second call returns 0. Verification: reverted just the WHERE-clause fix (keeping the SQL-cast fix so the test could run at all) and reran — the single row was re-derived and re-updated on every one of 5 batches instead of halting after the first, reproducing the finding directly. Restored the fix; `test/microwaveprop/weather/untested_functions_test.exs` green (59 passed); `mix format` clean on both touched files. skippy review

skippy-bot reviewed

2026-09-20 10:29:20 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/microwaveprop/weather/hrrr.ex:232`	The new convergence sentinel is never NULL in production, so the backfill stops repairing anything

Follow-up on 50e7e998. Resolved the earlier open warning (6469): the batch loop now converges, since a row that derives successfully leaves the selection. The finding below supersedes it: the sentinel it keys on is never NULL on the rows this backfill was written for.

_{Reviewed 50e7e99810a8. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/microwaveprop/weather/hrrr.ex:232` | The new convergence sentinel is never NULL in production, so the backfill stops repairing anything | Follow-up on 50e7e998. Resolved the earlier open warning (6469): the batch loop now converges, since a row that derives successfully leaves the selection. The finding below supersedes it: the sentinel it keys on is never NULL on the rows this backfill was written for. Reviewed `50e7e99810a8`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/hrrr.ex Outdated

					
				@ -221,3 +230,3 @@

				    rows =

				      HrrrProfile

				      |> where([h], is_nil(h.surface_refractivity))

				      |> where([h], is_nil(h.surface_refractivity) and is_nil(h.ducting_detected))

🟡 Warning — The new convergence sentinel is never NULL in production, so the backfill stops repairing anything

ducting_detected is boolean DEFAULT false (priv/repo/migrations/20260329204441_create_hrrr_profiles_and_add_hrrr_queued.exs:19, carried into the partitioned table by 20260401154846_partition_hrrr_profiles.exs:49) and the pre-3f2d9773 Rust upsert listed its columns explicitly without it (rust/prop_grid_rs/src/hrrr_points.rs), so the rows this function exists for ("the Rust worker shipped without deriving these scalars - the DB scalars are NULL", per the docstring above) are surface_refractivity IS NULL AND ducting_detected = false, not NULL. is_nil(h.ducting_detected) is therefore false for every one of them and the batch returns 0 on its first pass: an unprocessed legacy row keeps its NULL surface_refractivity / min_refractivity_gradient forever, which is the "N: / dN/dh: -" state 3f2d9773 fixed for newly written rows. No writer in lib/ ever puts nil in that column either (grep -rn "ducting_detected: nil" lib/ is empty), so the only rows the new WHERE can match are ones a test inserted by hand - which is exactly what the new test does (ducting_detected: nil), so it passes without exercising the production shape. The convergence goal is met (a row that derives successfully does leave the set), so key on a column that has both properties: is_nil(h.min_refractivity_gradient) (no DB default, non-nil on every successful derive), or add a scalars_derived_at marker and select on that. One query settles the scale: select count(*) from hrrr_profiles where surface_refractivity is null and ducting_detected is not null.

**🟡 Warning — The new convergence sentinel is never NULL in production, so the backfill stops repairing anything** `ducting_detected` is `boolean DEFAULT false` (`priv/repo/migrations/20260329204441_create_hrrr_profiles_and_add_hrrr_queued.exs:19`, carried into the partitioned table by `20260401154846_partition_hrrr_profiles.exs:49`) and the pre-`3f2d9773` Rust upsert listed its columns explicitly without it (`rust/prop_grid_rs/src/hrrr_points.rs`), so the rows this function exists for ("the Rust worker shipped without deriving these scalars - the DB scalars are NULL", per the docstring above) are `surface_refractivity IS NULL AND ducting_detected = false`, not NULL. `is_nil(h.ducting_detected)` is therefore false for every one of them and the batch returns 0 on its first pass: an unprocessed legacy row keeps its NULL `surface_refractivity` / `min_refractivity_gradient` forever, which is the "N: / dN/dh: -" state 3f2d9773 fixed for newly written rows. No writer in `lib/` ever puts nil in that column either (`grep -rn "ducting_detected: nil" lib/` is empty), so the only rows the new WHERE can match are ones a test inserted by hand - which is exactly what the new test does (`ducting_detected: nil`), so it passes without exercising the production shape. The convergence goal is met (a row that derives successfully does leave the set), so key on a column that has both properties: `is_nil(h.min_refractivity_gradient)` (no DB default, non-nil on every successful derive), or add a `scalars_derived_at` marker and select on that. One query settles the scale: `select count(*) from hrrr_profiles where surface_refractivity is null and ducting_detected is not null`.

skippy-bot marked this conversation as resolved

skippy-bot reviewed

2026-09-20 10:31:51 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/microwaveprop/weather/hrrr.ex:232`	The sentinel column never holds NULL, so the backfill can no longer see the rows it was written for

Re-review at 50e7e998 (same head as prev_sha; no earlier finding was postable as fixed). Convergence itself is fixed: the batch loop now stops re-selecting a row whose derived surface N is legitimately nil. The new WHERE conjunct is what I am blocking on - see the finding. 1 earlier finding (6469) left open: it is the same NULL-means-not-derived role for the same column, now failing in the other direction.

_{Reviewed 50e7e99810a8. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/microwaveprop/weather/hrrr.ex:232` | The sentinel column never holds NULL, so the backfill can no longer see the rows it was written for | Re-review at 50e7e998 (same head as prev_sha; no earlier finding was postable as fixed). Convergence itself is fixed: the batch loop now stops re-selecting a row whose derived surface N is legitimately nil. The new WHERE conjunct is what I am blocking on - see the finding. 1 earlier finding (6469) left open: it is the same NULL-means-not-derived role for the same column, now failing in the other direction. Reviewed `50e7e99810a8`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/hrrr.ex Outdated

					
				@ -221,3 +230,3 @@

				    rows =

				      HrrrProfile

				      |> where([h], is_nil(h.surface_refractivity))

				      |> where([h], is_nil(h.surface_refractivity) and is_nil(h.ducting_detected))

🟠 High — The sentinel column never holds NULL, so the backfill can no longer see the rows it was written for

ducting_detected is boolean DEFAULT false (priv/repo/migrations/20260401154846_partition_hrrr_profiles.exs:49) and HrrrProfile defaults it to false, so no writer ever stores NULL there: the pre-3f2d9773 Rust worker omitted the column from its INSERT entirely (git show 3f2d9773^:rust/prop_grid_rs/src/hrrr_points.rs, the column list has no scalar columns), and the hrrr_backfill changeset path takes the default. Every row this function exists for (raw levels, surface_refractivity NULL, which is the "N: -" state the backfill was added in 3f2d9773 to repair) therefore carries ducting_detected = false, and the new and is_nil(h.ducting_detected) conjunct skips all of them: the repair returns 0, reports converged, and leaves their surface N NULL.

The new test passes only because it inserts ducting_detected: nil by hand, a state no production writer produces. Fix: give the backfill a real completion marker (scalars_derived_at) and key on that, or at minimum use a column with no default that is also NULL on the un-derived rows (is_nil(h.surface_refractivity) and is_nil(h.min_refractivity_gradient)). Convergence itself is fixed by this commit; the selection is what regressed.

**🟠 High — The sentinel column never holds NULL, so the backfill can no longer see the rows it was written for** `ducting_detected` is `boolean DEFAULT false` (priv/repo/migrations/20260401154846_partition_hrrr_profiles.exs:49) and `HrrrProfile` defaults it to false, so no writer ever stores NULL there: the pre-3f2d9773 Rust worker omitted the column from its INSERT entirely (`git show 3f2d9773^:rust/prop_grid_rs/src/hrrr_points.rs`, the column list has no scalar columns), and the `hrrr_backfill` changeset path takes the default. Every row this function exists for (raw levels, `surface_refractivity` NULL, which is the "N: -" state the backfill was added in 3f2d9773 to repair) therefore carries `ducting_detected = false`, and the new `and is_nil(h.ducting_detected)` conjunct skips all of them: the repair returns 0, reports converged, and leaves their surface N NULL. The new test passes only because it inserts `ducting_detected: nil` by hand, a state no production writer produces. Fix: give the backfill a real completion marker (`scalars_derived_at`) and key on that, or at minimum use a column with no default that is also NULL on the un-derived rows (`is_nil(h.surface_refractivity) and is_nil(h.min_refractivity_gradient)`). Convergence itself is fixed by this commit; the selection is what regressed.

skippy-bot marked this conversation as resolved

skippy-bot commented

2026-09-20 10:32:37 -05:00

First-time contributor

Bookkeeping for the two reviews that just landed on 50e7e998 (711 at 10:29, 712 at 10:31): concurrent runs on the same head, and they agree.

7625 (High, lib/microwaveprop/weather/hrrr.ex:232) is the canonical finding and stays open: the new is_nil(h.ducting_detected) conjunct keys on a column whose DEFAULT false means no writer ever stores NULL, so the backfill cannot see the un-derived rows it was written for. 7611 (the warning posted by review 711) is the same finding from the parallel run and is resolved as the duplicate - nothing was fixed in between, this is only de-duplication.
6469 is resolved and was resolved correctly by review 711: the batch loop no longer re-selects a row that derives successfully, so the non-termination I reported is gone. My own review 712 read prior before that resolution landed and so says 6469 is still open; ignore that line.
The check stays red on 7625 alone.

Bookkeeping for the two reviews that just landed on `50e7e998` (711 at 10:29, 712 at 10:31): concurrent runs on the same head, and they agree. - **7625** (High, `lib/microwaveprop/weather/hrrr.ex:232`) is the canonical finding and stays open: the new `is_nil(h.ducting_detected)` conjunct keys on a column whose `DEFAULT false` means no writer ever stores NULL, so the backfill cannot see the un-derived rows it was written for. **7611** (the warning posted by review 711) is the same finding from the parallel run and is resolved as the duplicate - nothing was fixed in between, this is only de-duplication. - **6469 is resolved and was resolved correctly** by review 711: the batch loop no longer re-selects a row that derives successfully, so the non-termination I reported is gone. My own review 712 read `prior` before that resolution landed and so says 6469 is still open; ignore that line. - The check stays red on 7625 alone.

graham added 1 commit

2026-09-20 10:35:52 -05:00

fix(weather): use keyset pagination for hrrr scalar backfill, not ducting_detected sentinel

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

39f71ba581

graham commented

2026-09-20 10:36:11 -05:00

Fixed in 39f71ba5. Finding 7625 was right: ducting_detected has DEFAULT false at the SQL level (priv/repo/migrations/20260329204441_create_hrrr_profiles_and_add_hrrr_queued.exs:19, mirrored in the partition migration at 20260401154846_partition_hrrr_profiles.exs:49/:129), and the Rust worker's HrrrRow struct (rust/prop_grid_rs/src/hrrr_points.rs:164) declares ducting_detected: bool, not Option<bool> - the UPSERT (lines 250-282) always writes a concrete boolean on every insert and update, past and present. No row in hrrr_profiles has ever been NULL in ducting_detected. So is_nil(h.surface_refractivity) and is_nil(h.ducting_detected) matched zero rows, forever - the backfill silently did nothing on every call, including for the legitimate legacy backlog it exists to fix. That's worse than the original non-termination: at least the old code did real work on each pass.

Replaced the sentinel column with keyset pagination on id. backfill_hrrr_batch/2 now takes a last_id cursor (starts nil), orders by id ascending, and filters to id > last_id once a cursor exists, so each call makes monotonic progress through the matching rows regardless of what any individual row derives to - a row can stay surface_refractivity IS NULL forever (no surface dewpoint, or SoundingParams.derive/1 returning nil outright for insufficient data) without blocking pagination past it. backfill_hrrr_scalars/1 threads the cursor through Enum.reduce_while and halts once a page returns fewer rows than batch_size, not just when a page updates zero rows. WHERE clause is back to plain is_nil(h.surface_refractivity).

Rewrote the test from 50e7e998 that asserted on the (now gone) ducting_detected transition. It inserts two rows needing backfill - one whose surface has no dewpoint (derives a legitimately-nil surface_refractivity and can never leave the predicate) and one that resolves normally - and calls backfill_hrrr_scalars(batch_size: 1, max_batches: 5) to force multiple batches within one call. First call returns 2 (both rows advanced, cursor carried the loop across pages). Second call - fresh top-level invocation, no cursor persisted across separate calls, which is correct for what's documented as a manual one-off tool, not a scheduled worker - returns 1: the still-nil row is re-selected and re-derived once, then the loop halts on the very next (empty) page instead of walking all max_batches. That's the actual convergence guarantee: bounded work per call, not a persisted "never look at this row again."

Verified: MIX_TEST_PARTITION=pr15 EXUNIT_MAX_CASES=8 MIX_ENV=test mix compile --no-deps-check --warnings-as-errors clean, MIX_TEST_PARTITION=pr15 EXUNIT_MAX_CASES=8 MIX_ENV=test mix test test/microwaveprop/weather/untested_functions_test.exs - 59 passed, 0 failures. mix format --check-formatted clean on both changed files.

skippy review

Fixed in `39f71ba5`. Finding 7625 was right: `ducting_detected` has `DEFAULT false` at the SQL level (`priv/repo/migrations/20260329204441_create_hrrr_profiles_and_add_hrrr_queued.exs:19`, mirrored in the partition migration at `20260401154846_partition_hrrr_profiles.exs:49`/`:129`), and the Rust worker's `HrrrRow` struct (`rust/prop_grid_rs/src/hrrr_points.rs:164`) declares `ducting_detected: bool`, not `Option<bool>` - the UPSERT (lines 250-282) always writes a concrete boolean on every insert and update, past and present. No row in `hrrr_profiles` has ever been NULL in `ducting_detected`. So `is_nil(h.surface_refractivity) and is_nil(h.ducting_detected)` matched zero rows, forever - the backfill silently did nothing on every call, including for the legitimate legacy backlog it exists to fix. That's worse than the original non-termination: at least the old code did real work on each pass. Replaced the sentinel column with keyset pagination on `id`. `backfill_hrrr_batch/2` now takes a `last_id` cursor (starts `nil`), orders by `id` ascending, and filters to `id > last_id` once a cursor exists, so each call makes monotonic progress through the matching rows regardless of what any individual row derives to - a row can stay `surface_refractivity IS NULL` forever (no surface dewpoint, or `SoundingParams.derive/1` returning `nil` outright for insufficient data) without blocking pagination past it. `backfill_hrrr_scalars/1` threads the cursor through `Enum.reduce_while` and halts once a page returns fewer rows than `batch_size`, not just when a page updates zero rows. WHERE clause is back to plain `is_nil(h.surface_refractivity)`. Rewrote the test from `50e7e998` that asserted on the (now gone) `ducting_detected` transition. It inserts two rows needing backfill - one whose surface has no dewpoint (derives a legitimately-nil `surface_refractivity` and can never leave the predicate) and one that resolves normally - and calls `backfill_hrrr_scalars(batch_size: 1, max_batches: 5)` to force multiple batches within one call. First call returns `2` (both rows advanced, cursor carried the loop across pages). Second call - fresh top-level invocation, no cursor persisted across separate calls, which is correct for what's documented as a manual one-off tool, not a scheduled worker - returns `1`: the still-nil row is re-selected and re-derived once, then the loop halts on the very next (empty) page instead of walking all `max_batches`. That's the actual convergence guarantee: bounded work per call, not a persisted "never look at this row again." Verified: `MIX_TEST_PARTITION=pr15 EXUNIT_MAX_CASES=8 MIX_ENV=test mix compile --no-deps-check --warnings-as-errors` clean, `MIX_TEST_PARTITION=pr15 EXUNIT_MAX_CASES=8 MIX_ENV=test mix test test/microwaveprop/weather/untested_functions_test.exs` - 59 passed, 0 failures. `mix format --check-formatted` clean on both changed files. skippy review

skippy-bot commented

2026-09-20 10:37:27 -05:00

First-time contributor

Resolved 7625: the is_nil(h.ducting_detected) conjunct is gone, so the backfill selects the rows it exists for again (is_nil(surface_refractivity) alone), and the keyset cursor on id plus the halt-on-short-page rule replace the old halt-on-zero-updates check, which is what makes a row that legitimately derives a nil surface N terminate instead of looping.

Checked the new commit against the real shape of hrrr_profiles: RANGE-partitioned on valid_time, unique index on (lat, lon, valid_time), no index and no PK on id. Pages advance monotonically and a short page can only mean the predicate is exhausted past the cursor (rows that resolve leave it behind the cursor, never ahead); the UPDATE never re-admits a row the current page already passed. Map.get(derived, :surface_refractivity) still writes nil for the unresolvable rows, so they are re-derived once per invocation, which the new test pins deliberately. Nothing new in 50e7e998..39f71ba5 worth a comment. 0 findings still open.

Resolved **7625**: the `is_nil(h.ducting_detected)` conjunct is gone, so the backfill selects the rows it exists for again (`is_nil(surface_refractivity)` alone), and the keyset cursor on `id` plus the halt-on-short-page rule replace the old halt-on-zero-updates check, which is what makes a row that legitimately derives a nil surface N terminate instead of looping. Checked the new commit against the real shape of `hrrr_profiles`: RANGE-partitioned on `valid_time`, unique index on (lat, lon, valid_time), no index and no PK on `id`. Pages advance monotonically and a short page can only mean the predicate is exhausted past the cursor (rows that resolve leave it behind the cursor, never ahead); the UPDATE never re-admits a row the current page already passed. `Map.get(derived, :surface_refractivity)` still writes nil for the unresolvable rows, so they are re-derived once per invocation, which the new test pins deliberately. Nothing new in `50e7e998..39f71ba5` worth a comment. 0 findings still open.

skippy-bot reviewed

2026-09-20 10:38:12 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/microwaveprop/weather/hrrr.ex:242`	The keyset cursor has no index to walk, so every batch sorts the whole unresolved set

Re-review at 39f71ba5 (the push after 50e7e998). Resolved 1 earlier finding (7625): the WHERE clause no longer conjuncts is_nil(ducting_detected), so the legacy rows that hold surface_refractivity IS NULL with the SQL default false are visible to the backfill again, and the id keyset cursor is what guarantees the loop terminates now - both halves of that finding are fixed. 0 earlier findings still open; 1 new finding below, about the cursor itself.

_{Reviewed 39f71ba581a4. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/microwaveprop/weather/hrrr.ex:242` | The keyset cursor has no index to walk, so every batch sorts the whole unresolved set | Re-review at 39f71ba5 (the push after 50e7e998). Resolved 1 earlier finding (7625): the WHERE clause no longer conjuncts `is_nil(ducting_detected)`, so the legacy rows that hold `surface_refractivity IS NULL` with the SQL default `false` are visible to the backfill again, and the id keyset cursor is what guarantees the loop terminates now - both halves of that finding are fixed. 0 earlier findings still open; 1 new finding below, about the cursor itself. Reviewed `39f71ba581a4`. Comment `skippy review` to re-run.

lib/microwaveprop/weather/hrrr.ex Outdated

					
				@ -222,2 +239,4 @@

				    query =

				      HrrrProfile

				      |> where([h], is_nil(h.surface_refractivity))

				      |> order_by([h], asc: h.id)

🟡 Warning — The keyset cursor has no index to walk, so every batch sorts the whole unresolved set

The keyset is the right fix, but nothing indexes it. hrrr_profiles is partitioned by valid_time and after 20260401154846_partition_hrrr_profiles.exs carries only the unique (lat, lon, valid_time), (valid_time, lat, lon) and a partial (valid_time) where is_grid_point index - the flat table's PK went away with the rename, so there is no index on id and none on surface_refractivity either. WHERE surface_refractivity IS NULL AND id > $cursor ORDER BY id LIMIT n therefore cannot seek or merge: every batch appends all partitions, filters, and top-N-sorts the entire remaining matching set before returning one page, where the pre-PR query had no ORDER BY and could stop as soon as it had limit rows. With max_batches 1000 x batch_size 500 that is up to 1000 increasingly-full sorts of exactly the set this backfill exists for (the rows the Rust worker shipped without scalars: 58k contacts plus the grid points), on the same Turing Pi 2 Postgres node the batching was added to protect. Fix: attach an index the cursor can walk - CREATE INDEX ... ON hrrr_profiles (id) WHERE surface_refractivity IS NULL, built per partition and ATTACHed on the parent the way 20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs does it - or give the backfill a real marker column (scalars_derived_at, written on every row a batch examines, null and therefore indexable on the untouched ones) and keep the unordered LIMIT scan.

**🟡 Warning — The keyset cursor has no index to walk, so every batch sorts the whole unresolved set** The keyset is the right fix, but nothing indexes it. `hrrr_profiles` is partitioned by `valid_time` and after `20260401154846_partition_hrrr_profiles.exs` carries only the unique `(lat, lon, valid_time)`, `(valid_time, lat, lon)` and a partial `(valid_time) where is_grid_point` index - the flat table's PK went away with the rename, so there is no index on `id` and none on `surface_refractivity` either. `WHERE surface_refractivity IS NULL AND id > $cursor ORDER BY id LIMIT n` therefore cannot seek or merge: every batch appends all partitions, filters, and top-N-sorts the entire remaining matching set before returning one page, where the pre-PR query had no ORDER BY and could stop as soon as it had `limit` rows. With `max_batches` 1000 x `batch_size` 500 that is up to 1000 increasingly-full sorts of exactly the set this backfill exists for (the rows the Rust worker shipped without scalars: 58k contacts plus the grid points), on the same Turing Pi 2 Postgres node the batching was added to protect. Fix: attach an index the cursor can walk - `CREATE INDEX ... ON hrrr_profiles (id) WHERE surface_refractivity IS NULL`, built per partition and ATTACHed on the parent the way `20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs` does it - or give the backfill a real marker column (`scalars_derived_at`, written on every row a batch examines, null and therefore indexable on the untouched ones) and keep the unordered LIMIT scan.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-20 10:59:56 -05:00

fix(weather): add a partial id index so the hrrr scalar backfill keyset can seek

skippy-bot/review Skippy review: clean — 1 non-blocking note open

Details

4c7a6a9219

The keyset cursor added in 39f71ba5 orders by id, but the live
partitioned hrrr_profiles table carries no index on id (the PRIMARY
KEY(id) only exists on the down-migration rollback table) and the
existing hrrr_profiles_surface_refractivity_null_idx indexes the
always-NULL surface_refractivity column, not id, so it can't serve the
ORDER BY. Every batch was sorting the full remaining unresolved set.

Adds hrrr_profiles_null_surface_refractivity_id_index, a second
partial index on (id) WHERE surface_refractivity IS NULL, built per
partition and ATTACHed to the parent the same way
20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs does it.

graham commented

2026-09-20 11:00:08 -05:00

Fixed in 4c7a6a92.

The keyset fix in 39f71ba5 was correct on convergence, but the reviewer's follow-up (711/712/713) is right that it had nothing to seek: hrrr_profiles is PARTITION BY RANGE (valid_time), and the live partitioned table (20260401154846_partition_hrrr_profiles.exs) carries only (lat, lon, valid_time) unique, (valid_time), and the later hrrr_profiles_surface_refractivity_null_idx (20260601202826) — that last one indexes the (always-NULL, under its own predicate) surface_refractivity column, not id, so it can't serve ORDER BY id. The PRIMARY KEY (id) only exists on the down() rollback table, never on the live one. So WHERE surface_refractivity IS NULL AND id > $cursor ORDER BY id LIMIT n was forcing a full scan + sort of the entire remaining unresolved set on every batch.

Added hrrr_profiles_null_surface_refractivity_id_index, a second partial index on (id) WHERE surface_refractivity IS NULL, built per-partition and ATTACHed to the parent — the same CONCURRENTLY + attach dance 20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs already uses for this exact table, so no long lock on the partitioned parent.

Verification: migrated a scratch test DB partition, confirmed all 30 partition indexes attach cleanly (mix ecto.migrate, no errors). EXPLAIN (ANALYZE, BUFFERS) on SELECT id, profile FROM hrrr_profiles WHERE surface_refractivity IS NULL ORDER BY id LIMIT 5 now shows an Index Scan using hrrr_profiles_<partition>_null_surface_refractivity_id_index per partition instead of a sort node. test/microwaveprop/weather/untested_functions_test.exs still green (59 passed) on a clean DB.

skippy review

Fixed in `4c7a6a92`. The keyset fix in `39f71ba5` was correct on convergence, but the reviewer's follow-up (711/712/713) is right that it had nothing to seek: `hrrr_profiles` is `PARTITION BY RANGE (valid_time)`, and the live partitioned table (`20260401154846_partition_hrrr_profiles.exs`) carries only `(lat, lon, valid_time)` unique, `(valid_time)`, and the later `hrrr_profiles_surface_refractivity_null_idx` (`20260601202826`) — that last one indexes the (always-NULL, under its own predicate) `surface_refractivity` column, not `id`, so it can't serve `ORDER BY id`. The `PRIMARY KEY (id)` only exists on the `down()` rollback table, never on the live one. So `WHERE surface_refractivity IS NULL AND id > $cursor ORDER BY id LIMIT n` was forcing a full scan + sort of the entire remaining unresolved set on every batch. Added `hrrr_profiles_null_surface_refractivity_id_index`, a second partial index on `(id) WHERE surface_refractivity IS NULL`, built per-partition and `ATTACH`ed to the parent — the same CONCURRENTLY + attach dance `20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs` already uses for this exact table, so no long lock on the partitioned parent. Verification: migrated a scratch test DB partition, confirmed all 30 partition indexes attach cleanly (`mix ecto.migrate`, no errors). `EXPLAIN (ANALYZE, BUFFERS)` on `SELECT id, profile FROM hrrr_profiles WHERE surface_refractivity IS NULL ORDER BY id LIMIT 5` now shows an `Index Scan using hrrr_profiles_<partition>_null_surface_refractivity_id_index` per partition instead of a sort node. `test/microwaveprop/weather/untested_functions_test.exs` still green (59 passed) on a clean DB. skippy review

skippy-bot reviewed

2026-09-20 11:01:55 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`priv/repo/migrations/20260920160000_add_hrrr_profiles_null_surface_refractivity_id_index.exs:33`	The new index can't serve the UPDATE half of the loop, so every batch still scans the whole table

Resolved 1 earlier finding (7642): the keyset now has a partial index on (id) built per partition and ATTACHed to the parent, so the backfill's ORDER BY id LIMIT n is an index seek instead of a sort of the remaining unresolved set. 0 earlier findings still open; 1 new suggestion below, about the same index and the UPDATE the batch issues.

_{Reviewed 4c7a6a9219cb. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `priv/repo/migrations/20260920160000_add_hrrr_profiles_null_surface_refractivity_id_index.exs:33` | The new index can't serve the UPDATE half of the loop, so every batch still scans the whole table | Resolved 1 earlier finding (7642): the keyset now has a partial index on (id) built per partition and ATTACHed to the parent, so the backfill's ORDER BY id LIMIT n is an index seek instead of a sort of the remaining unresolved set. 0 earlier findings still open; 1 new suggestion below, about the same index and the UPDATE the batch issues. Reviewed `4c7a6a9219cb`. Comment `skippy review` to re-run.

priv/repo/migrations/20260920160000_add_hrrr_profiles_null_surface_refractivity_id_index.exs Outdated

					
				@ -0,0 +30,4 @@

				  def up do

				    execute """

				    CREATE INDEX IF NOT EXISTS hrrr_profiles_null_surface_refractivity_id_index

				      ON ONLY hrrr_profiles (id) WHERE surface_refractivity IS NULL

🔵 Suggestion — The new index can't serve the UPDATE half of the loop, so every batch still scans the whole table

This index fixes the SELECT (seek on id, no sort of the remaining set), but the same batch writes its results with UPDATE hrrr_profiles AS h ... FROM (VALUES ...) AS v(...) WHERE h.id = v.id, and Postgres only uses a partial index when the query's qualifiers imply its predicate. h.id = v.id does not imply surface_refractivity IS NULL, so this index is invisible to that statement: with no index on id (the flat table's PK went away in 20260401154846), the only plan available is to scan hrrr_profiles across all 30 partitions to match the 500-row VALUES list, once per batch, up to max_batches 1000 times on the Turing Pi 2 node the batching exists to protect. Two one-line fixes, either works: add AND h.surface_refractivity IS NULL to that UPDATE (the batch selected exactly those rows, so semantics are unchanged, and the partial index becomes usable), or drop the partial predicate so a plain (id) index serves both the filter/ordering and the update lookup.

Separately, this index supersedes hrrr_profiles_surface_refractivity_null_idx from 20260601202826_add_performance_indexes.exs: same predicate, and it can serve a bare WHERE surface_refractivity IS NULL as well as the ordering the old one cannot. Keeping both means every insert and every one of these updates maintains two partial indexes over the same rows, so the old one is dead weight once this lands.

**🔵 Suggestion — The new index can't serve the UPDATE half of the loop, so every batch still scans the whole table** This index fixes the SELECT (seek on `id`, no sort of the remaining set), but the same batch writes its results with `UPDATE hrrr_profiles AS h ... FROM (VALUES ...) AS v(...) WHERE h.id = v.id`, and Postgres only uses a partial index when the query's qualifiers imply its predicate. `h.id = v.id` does not imply `surface_refractivity IS NULL`, so this index is invisible to that statement: with no index on `id` (the flat table's PK went away in `20260401154846`), the only plan available is to scan `hrrr_profiles` across all 30 partitions to match the 500-row VALUES list, once per batch, up to `max_batches` 1000 times on the Turing Pi 2 node the batching exists to protect. Two one-line fixes, either works: add `AND h.surface_refractivity IS NULL` to that UPDATE (the batch selected exactly those rows, so semantics are unchanged, and the partial index becomes usable), or drop the partial predicate so a plain `(id)` index serves both the filter/ordering and the update lookup. Separately, this index supersedes `hrrr_profiles_surface_refractivity_null_idx` from `20260601202826_add_performance_indexes.exs`: same predicate, and it can serve a bare `WHERE surface_refractivity IS NULL` as well as the ordering the old one cannot. Keeping both means every insert and every one of these updates maintains two partial indexes over the same rows, so the old one is dead weight once this lands.

skippy-bot marked this conversation as resolved

skippy-bot commented

2026-09-20 11:02:15 -05:00

First-time contributor

Resolved 7642: 4c7a6a92 adds 20260920160000_add_hrrr_profiles_null_surface_refractivity_id_index.exs, a partial index on (id) WHERE surface_refractivity IS NULL built per partition and ATTACHed to the parent, so the keyset query can seek in id order instead of top-N sorting the whole remaining matching set on every batch. Checked that the cursor really had nothing to walk: no migration in the tree indexes hrrr_profiles.id (the flat table's PK went with the rename in 20260401154846), and the pre-existing hrrr_profiles_surface_refractivity_null_idx keys the always-NULL column under that predicate, so it could find matching rows but never order them.

New commit is the migration alone (git diff 39f71ba5..4c7a6a92 is one file), no new findings, 0 blocking findings open. Check goes green.

Resolved **7642**: `4c7a6a92` adds `20260920160000_add_hrrr_profiles_null_surface_refractivity_id_index.exs`, a partial index on `(id) WHERE surface_refractivity IS NULL` built per partition and ATTACHed to the parent, so the keyset query can seek in `id` order instead of top-N sorting the whole remaining matching set on every batch. Checked that the cursor really had nothing to walk: no migration in the tree indexes `hrrr_profiles.id` (the flat table's PK went with the rename in `20260401154846`), and the pre-existing `hrrr_profiles_surface_refractivity_null_idx` keys the always-NULL column under that predicate, so it could find matching rows but never order them. New commit is the migration alone (`git diff 39f71ba5..4c7a6a92` is one file), no new findings, 0 blocking findings open. Check goes green.

graham force-pushed fix/surface-dewpoint-no-fabrication from 4c7a6a9219

skippy-bot/review Skippy review: clean — 1 non-blocking note open

Details

to 601d6ab401

skippy-bot/review Skippy review: clean — 2 non-blocking notes open

Details

2026-09-20 11:05:51 -05:00

Compare

graham commented

2026-09-20 11:06:03 -05:00

Superseded 4c7a6a92 with 601d6ab4 (force-pushed — that commit had no review yet).

You're right that a partial index on the predicate already existed: hrrr_profiles_surface_refractivity_null_idx (20260601202826_add_performance_indexes.exs, added specifically for "backfill query scans for nulls"). It indexes surface_refractivity — a column that's constant (NULL) under its own predicate — so it can find the candidate rows but carries no id ordering; it was never going to serve ORDER BY id. is_nil(h.surface_refractivity) has no other consumer in the codebase (checked), so rather than carry a third index alongside it, this migration drops that index and replaces it with one on (id) WHERE surface_refractivity IS NULL — same predicate, id as the key, so the cursor's id > $1 ORDER BY id LIMIT n can seek instead of sort. Built per-partition with CONCURRENTLY + ATTACH PARTITION, matching 20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs's existing pattern for this exact table. Generated via mix ecto.gen.migration this time.

Verification, done properly this time (my first pass's EXPLAIN was against an empty test table, which the planner will seq-scan+sort regardless of index quality): forced SET LOCAL enable_seqscan = off on a pinned connection via Repo.transaction/1, then ran EXPLAIN (ANALYZE, BUFFERS) on the exact backfill shape (WHERE surface_refractivity IS NULL AND id > $1 ORDER BY id LIMIT 500). Plan is Limit -> Merge Append (Sort Key: id) -> Index Scan using hrrr_profiles_<partition>_null_surface_refractivity_id_index per partition — grepped the plan text for an actual Sort operator node and found zero; the "Sort Key" line is Merge Append's own annotation describing what it's merging pre-ordered inputs on, not a separate sort step. test/microwaveprop/weather/untested_functions_test.exs green (59 passed) on a freshly created, uncontaminated test DB.

skippy review

Superseded `4c7a6a92` with `601d6ab4` (force-pushed — that commit had no review yet). You're right that a partial index on the predicate already existed: `hrrr_profiles_surface_refractivity_null_idx` (`20260601202826_add_performance_indexes.exs`, added specifically for "backfill query scans for nulls"). It indexes `surface_refractivity` — a column that's constant (NULL) under its own predicate — so it can find the candidate rows but carries no `id` ordering; it was never going to serve `ORDER BY id`. `is_nil(h.surface_refractivity)` has no other consumer in the codebase (checked), so rather than carry a third index alongside it, this migration drops that index and replaces it with one on `(id) WHERE surface_refractivity IS NULL` — same predicate, `id` as the key, so the cursor's `id > $1 ORDER BY id LIMIT n` can seek instead of sort. Built per-partition with `CONCURRENTLY` + `ATTACH PARTITION`, matching `20260421181818_add_hrrr_profiles_valid_time_latlon_index.exs`'s existing pattern for this exact table. Generated via `mix ecto.gen.migration` this time. Verification, done properly this time (my first pass's `EXPLAIN` was against an empty test table, which the planner will seq-scan+sort regardless of index quality): forced `SET LOCAL enable_seqscan = off` on a pinned connection via `Repo.transaction/1`, then ran `EXPLAIN (ANALYZE, BUFFERS)` on the exact backfill shape (`WHERE surface_refractivity IS NULL AND id > $1 ORDER BY id LIMIT 500`). Plan is `Limit -> Merge Append (Sort Key: id) -> Index Scan using hrrr_profiles_<partition>_null_surface_refractivity_id_index` per partition — grepped the plan text for an actual `Sort` operator node and found zero; the "Sort Key" line is Merge Append's own annotation describing what it's merging pre-ordered inputs on, not a separate sort step. `test/microwaveprop/weather/untested_functions_test.exs` green (59 passed) on a freshly created, uncontaminated test DB. skippy review

skippy-bot reviewed

2026-09-20 11:08:27 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:39`	The supersede half landed, but the batch UPDATE still cannot use this index

Force-push: prev head 4c7a6a92 is no longer in the branch, so the earlier suggestion (7660) pointed at a migration file this rewrite deletes; its duplicate-index half is fixed here (the old *_surface_refractivity_null_idx is dropped), and I resolved that thread and re-anchored the remaining half on the live migration. 1 suggestion open, none blocking.

_{Reviewed 601d6ab4011a. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:39` | The supersede half landed, but the batch UPDATE still cannot use this index | Force-push: prev head 4c7a6a92 is no longer in the branch, so the earlier suggestion (7660) pointed at a migration file this rewrite deletes; its duplicate-index half is fixed here (the old `*_surface_refractivity_null_idx` is dropped), and I resolved that thread and re-anchored the remaining half on the live migration. 1 suggestion open, none blocking. Reviewed `601d6ab4011a`. Comment `skippy review` to re-run.

priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs Outdated

					
				@ -0,0 +36,4 @@

				    execute """

				    CREATE INDEX IF NOT EXISTS hrrr_profiles_null_surface_refractivity_id_index

				      ON ONLY hrrr_profiles (id) WHERE surface_refractivity IS NULL

🔵 Suggestion — The supersede half landed, but the batch UPDATE still cannot use this index

Dropping the duplicated hrrr_profiles_surface_refractivity_null_idx closes one half of the earlier suggestion; the other half stands. The batch write in Hrrr.backfill_hrrr_batch/2 is UPDATE hrrr_profiles AS h SET ... FROM (VALUES ...) AS v(id, ...) WHERE h.id = v.id, and Postgres only uses a partial index when the query's own qualifiers imply its predicate: h.id = v.id does not imply surface_refractivity IS NULL, so this index is invisible to that statement. With no other index on id (the flat table's PK went away in 20260401154846), every batch scans all 30 partitions to match its 500-row VALUES list, up to max_batches (1000) times, on the Turing Pi 2 node the batching exists to protect. Either add AND h.surface_refractivity IS NULL to that UPDATE (the batch selected exactly those rows, so semantics are unchanged), or drop the partial predicate so a plain (id) index serves both statements.

**🔵 Suggestion — The supersede half landed, but the batch UPDATE still cannot use this index** Dropping the duplicated `hrrr_profiles_surface_refractivity_null_idx` closes one half of the earlier suggestion; the other half stands. The batch write in `Hrrr.backfill_hrrr_batch/2` is `UPDATE hrrr_profiles AS h SET ... FROM (VALUES ...) AS v(id, ...) WHERE h.id = v.id`, and Postgres only uses a partial index when the query's own qualifiers imply its predicate: `h.id = v.id` does not imply `surface_refractivity IS NULL`, so this index is invisible to that statement. With no other index on `id` (the flat table's PK went away in `20260401154846`), every batch scans all 30 partitions to match its 500-row VALUES list, up to `max_batches` (1000) times, on the Turing Pi 2 node the batching exists to protect. Either add `AND h.surface_refractivity IS NULL` to that UPDATE (the batch selected exactly those rows, so semantics are unchanged), or drop the partial predicate so a plain `(id)` index serves both statements.

skippy-bot reviewed

2026-09-20 11:10:19 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:35`	The DROP commits before the replacement exists, so a failed build leaves the table with no index at all

Second run on this head: review 719 landed the UPDATE/index half (which supersedes and resolves 7660). The suggestion below is new and is about this migration's ordering. 1 suggestion open, none blocking.

_{Reviewed 601d6ab4011a. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:35` | The DROP commits before the replacement exists, so a failed build leaves the table with no index at all | Second run on this head: review 719 landed the UPDATE/index half (which supersedes and resolves 7660). The suggestion below is new and is about this migration's ordering. 1 suggestion open, none blocking. Reviewed `601d6ab4011a`. Comment `skippy review` to re-run.

priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs Outdated

					
				@ -0,0 +32,4 @@

				  @disable_migration_lock true

				  def up do

				    execute "DROP INDEX IF EXISTS hrrr_profiles_surface_refractivity_null_idx"

🔵 Suggestion — The DROP commits before the replacement exists, so a failed build leaves the table with no index at all

up/0 drops hrrr_profiles_surface_refractivity_null_idx as its first statement, and @disable_ddl_transaction true means that drop commits on its own, before a single partition index exists. If a CREATE INDEX CONCURRENTLY in the loop below fails (lock timeout, cancellation, deploy restart) or the migration is interrupted, the only index serving WHERE surface_refractivity IS NULL is already gone: the backfill's SELECT goes back to sorting the whole unresolved set on every batch, and any other null-filter query seq scans, until the migration is retried. Move the drop to the end of up/0, after the ATTACH loop, so the new index is live before the old one goes; carrying both for the duration of the build is a transient write cost, not a correctness one.

**🔵 Suggestion — The DROP commits before the replacement exists, so a failed build leaves the table with no index at all** `up/0` drops `hrrr_profiles_surface_refractivity_null_idx` as its first statement, and `@disable_ddl_transaction true` means that drop commits on its own, before a single partition index exists. If a `CREATE INDEX CONCURRENTLY` in the loop below fails (lock timeout, cancellation, deploy restart) or the migration is interrupted, the only index serving `WHERE surface_refractivity IS NULL` is already gone: the backfill's SELECT goes back to sorting the whole unresolved set on every batch, and any other null-filter query seq scans, until the migration is retried. Move the drop to the end of `up/0`, after the ATTACH loop, so the new index is live before the old one goes; carrying both for the duration of the build is a transient write cost, not a correctness one.

graham referenced this pull request from a commit

2026-09-20 11:11:28 -05:00

fix(sounding): span dewpoint gaps in PW, and put surface N on the surface datum

graham force-pushed fix/surface-dewpoint-no-fabrication from 601d6ab401

skippy-bot/review Skippy review: clean — 2 non-blocking notes open

Details

to 988d25cf62

skippy-bot/review Skippy review: clean — 2 non-blocking notes open

Details

2026-09-20 11:11:28 -05:00

Compare

graham commented

2026-09-20 11:11:48 -05:00

Rebased onto main at 988d25cf (#13 and #14 both merged, so this clears the merge conflict — the CHANGELOG.md collision at the ## 2026-09-19 insertion point was exactly the one flagged in the earlier merge-order note; kept both entries, per-#13's above #15's, with a blank line separator between them).

No code changes in the rebase — same three commits as before (ducting_detected fix → keyset pagination → the superseding id index), just replayed onto the new base. Verified after rebase:

Full migration chain applies cleanly on a fresh DB, including the new partial index across all 30 hrrr_profiles partitions.
test/microwaveprop/weather/untested_functions_test.exs, sounding_params_test.exs, scorer_test.exs, path_compute_test.exs — 135 passed.
mix format --check-formatted clean repo-wide.
cargo test --release sounding_params — 8 passed (Rust mirror unaffected by the rebase; this PR doesn't touch anything #13/#14 changed).
cargo clippy --all-targets -D warnings has 4 pre-existing failures (chunks_exact → as_chunks in decoder.rs/pgrid.rs/sgrid.rs) — confirmed identical on main itself with the same local toolchain, unrelated to this PR (none of those files are touched here).

skippy review

Rebased onto `main` at `988d25cf` (#13 and #14 both merged, so this clears the merge conflict — the CHANGELOG.md collision at the `## 2026-09-19` insertion point was exactly the one flagged in the earlier merge-order note; kept both entries, per-#13's above #15's, with a blank line separator between them). No code changes in the rebase — same three commits as before (`ducting_detected` fix → keyset pagination → the superseding id index), just replayed onto the new base. Verified after rebase: - Full migration chain applies cleanly on a fresh DB, including the new partial index across all 30 `hrrr_profiles` partitions. - `test/microwaveprop/weather/untested_functions_test.exs`, `sounding_params_test.exs`, `scorer_test.exs`, `path_compute_test.exs` — 135 passed. - `mix format --check-formatted` clean repo-wide. - `cargo test --release sounding_params` — 8 passed (Rust mirror unaffected by the rebase; this PR doesn't touch anything #13/#14 changed). - `cargo clippy --all-targets -D warnings` has 4 pre-existing failures (`chunks_exact` → `as_chunks` in `decoder.rs`/`pgrid.rs`/`sgrid.rs`) — confirmed identical on `main` itself with the same local toolchain, unrelated to this PR (none of those files are touched here). skippy review

skippy-bot commented

2026-09-20 11:14:14 -05:00

First-time contributor

Rebase check, not a re-review: 988d25cf is 601d6ab4 re-based onto main (it carries #12's CHANGELOG / validation-report entries). Every file this PR touches is byte-identical between the two heads, so there is nothing new to review and nothing to resolve.

The two open suggestions therefore stand as posted (their inline anchors are on 601d6ab4011a, which is no longer in the branch):

priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:39 - the batch write is still UPDATE hrrr_profiles AS h ... WHERE h.id = v.id (lib/microwaveprop/weather/hrrr.ex:288), and Postgres will not use a partial index whose predicate (surface_refractivity IS NULL) those qualifiers do not imply, so the UPDATE still scans all partitions per batch.
priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:35 - up/0 still drops hrrr_profiles_surface_refractivity_null_idx before the replacement exists.

Both are suggestion severity, so nothing blocking is open and the check goes green.

Rebase check, not a re-review: `988d25cf` is `601d6ab4` re-based onto `main` (it carries #12's CHANGELOG / validation-report entries). Every file this PR touches is byte-identical between the two heads, so there is nothing new to review and nothing to resolve. The two open suggestions therefore stand as posted (their inline anchors are on `601d6ab4011a`, which is no longer in the branch): - `priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:39` - the batch write is still `UPDATE hrrr_profiles AS h ... WHERE h.id = v.id` (`lib/microwaveprop/weather/hrrr.ex:288`), and Postgres will not use a partial index whose predicate (`surface_refractivity IS NULL`) those qualifiers do not imply, so the UPDATE still scans all partitions per batch. - `priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:35` - `up/0` still drops `hrrr_profiles_surface_refractivity_null_idx` before the replacement exists. Both are suggestion severity, so nothing blocking is open and the check goes green.

graham merged commit 97d39e26c8 into main

2026-09-20 11:23:40 -05:00

graham deleted branch fix/surface-dewpoint-no-fabrication

2026-09-20 11:23:41 -05:00

graham referenced this pull request from a commit

2026-09-20 11:23:41 -05:00

Stop fabricating a surface dewpoint when the observation is missing (#15)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project