Skip to content

feat(regression): per-predictor quadratic + Smith River nr Gardiner fit - #120

Merged
mousebrains merged 6 commits into
mainfrom
add-smith-gardiner
Jun 4, 2026
Merged

feat(regression): per-predictor quadratic + Smith River nr Gardiner fit#120
mousebrains merged 6 commits into
mainfrom
add-smith-gardiner

Conversation

@mousebrains

Copy link
Copy Markdown
Owner

Summary

Adds a calc-gauge estimate for the retired Smith River near Gardiner gauge (USGS 14323100, daily record 1965-10 → 1973-06 only), plus the tool feature the fit needed and a docs sweep it surfaced. Pairs with kayak_data PR carrying the 4 metadata rows (calc_expression 17 / source 347 / gauge 224 / gauge_source) — merge this PR first (or in the same deploy window) so the published report artifacts and HTML-filter fix are live when the metadata lands.

1. --quadratic-for (per-predictor quadratic) — 6d8947b

--quadratic squares every predictor; the Smith fit needed Siuslaw² (significant, halves dry-season bias from +59% → +30% of mean flow) without SF Coquille² (block-boot CI straddles zero, and would contribute ~+1,175 cfs of phantom signal at the predictor's observed max). Spec normalized to a per-predictor mask internally; bool stays accepted everywhere (existing callers unchanged); report family label, Reproduce snippet, coefficient labels, stub, and fit JSON (quadratic_sites added) all follow the mask. 6 new tests.

2. Stub flow: kayak_data CSV, not migrations — b3509cc

The generated report and both regression READMEs still said "paste the SQL stub into a migration" — which now fails CI (test_migrations_schema_only.py; calc_expression is a metadata table). The report now emits the calc_expression.csv column values (incl. provenance_slug); READMEs updated to the CSV + levels sync-metadata flow.

3. The Smith fit — 961cfed

docs/regression/smith_14323100_from_siuslaw_sfcoquille.{md,svg,json}:

Smith = round(max(0, 0.288564·Siuslaw + 0.130486·SFCoquille + 2.12545e-06·Siuslaw² − 73.45))
r² = 0.9366, RMSE = 360.4 cfs, n = 2100   (Siuslaw-only linear baseline: 0.9278 / 384.7)

Donor screening over every active gauge overlapping the target's record (block-bootstrap 95% CIs, monthly blocks): SF Coquille 14325000 is the only significant addition to the Siuslaw (+0.130, CI [+0.036, +0.225], VIF 4.3 — the one donor south of the Smith). Rejected with CIs straddling zero once the Siuslaw is in: Alsea 14306500, Siletz 14305500, Umpqua-Elkton 14321000, Long Tom-Noti 14166500, Coast Fork-London 14152500. Full screening history recorded in the kayak_data calc_expression.note.

4. Migration-doc sweep + HTML-filter fix — 7210253

  • docs/migrations.md: five references still pointed metadata edits at data/db/*.csv — repointed at the kayak_data repo (METADATA_DIR); data/db/migrations/ unchanged.
  • Bug caught in the sweep: deploy.py::_HTML_DROP_SECTIONS only matched the old ## SQL stub heading, so the renamed ## calc_expression row section would have leaked maintainer scaffolding into the published report HTML. New heading added (old kept for pre-redesign reports) + filter test.

Test plan

  • ruff check / ruff format --check / mypy (CI scope) clean
  • Full pytest: 1097 passed (3 pre-existing test_config.py failures are local-Mac python3.14 subprocess issues, present on clean main)
  • CLI round-trip: --quadratic-for fit reproduces the hand-built 4-param model exactly (same coefficients/CIs); mutually-exclusive and unknown-site error paths exit 1
  • End-to-end on a fresh live-DB pull: levels sync-metadata (4 inserts) → levels calculator → Smith = 25.0 cfs, hand-verified against the expression (Siuslaw 304, SFCoq 83.3 → 25.34 → 25)
  • levels build + local PHP serve: gauge page renders fact-box/sparkline/residuals; published report HTML drops the maintainer-only sections

🤖 Generated with Claude Code

mousebrains and others added 4 commits June 4, 2026 11:29
--quadratic squares every predictor, but an all-predictor quadratic fit
can leave one squared term whose block-bootstrap CI straddles zero — a
non-significant x² coefficient is an extrapolation hazard (at the high
end of the predictor range it contributes hundreds of cfs of phantom
signal, exactly where launch decisions happen). --quadratic-for SITE
(repeatable, mutually exclusive with --quadratic) squares only the
named predictors so the insignificant terms can be dropped.

Internally the quadratic spec is normalized to a per-predictor mask
(_quad_mask); bool stays accepted everywhere so existing callers and
the bare --quadratic flag are unchanged. The report's family label,
Reproduce snippet, coefficient labels, SQL stub, and fit JSON all
follow the mask; the JSON keeps `quadratic` as a bool (any squared
term) for backward compatibility and adds `quadratic_sites`.

Extract _default_stability_starts and _quad_spec_from_args from main()
to stay under the C901 complexity limit.

Motivating case: Smith River nr Gardiner (14323100, retired 1973) from
Siuslaw (14307620) + SF Coquille (14325000) — Siuslaw² is significant
and halves the dry-season bias, SF Coquille² is not (CI straddles 0)
and would add ~+1,175 cfs at the predictor's observed max.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igrations

The report's "SQL stub" section and both READMEs still instructed
pasting an INSERT into a data/db/migrations file — stale since the
Phase-5 metadata redesign (#106): calc_expression is a metadata table,
so a new migration writing it now fails CI
(test_migrations_schema_only.py). Replace the stub with the column
values (data_type / expression / time_expression / note /
provenance_slug) for a new calc_expression.csv row in kayak_data, and
update both READMEs' workflow steps to the CSV + levels sync-metadata
flow. Pre-redesign writeups keep their SQL stubs as immutable history.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…F Coquille

Smith R nr Gardiner's USGS record is daily means 1965-10-01..1973-06-30
only, so the fit window is capped by the Siuslaw nr Mapleton record
start (1967-10): n=2100 daily means. Donor screening over every active
gauge overlapping that window (block-bootstrap 95% CIs, monthly blocks):

- Siuslaw 14307620 solo (status quo): r2=0.928, RMSE 385 cfs
- SF Coquille 14325000 adds real signal: +0.130, CI [+0.036, +0.225],
  VIF 4.3 — the only candidate south of the Smith, independent
  storm-position information
- Rejected (CI straddles zero once Siuslaw is in): Alsea 14306500,
  Siletz 14305500, Umpqua-Elkton 14321000, Long Tom-Noti 14166500,
  Coast Fork-London 14152500 (significant vs Siuslaw alone but
  redundant given SF Coquille and the weaker duo)

Form: quadratic on the Siuslaw only (--quadratic-for). Siuslaw² CI
[+9.5e-07, +7.4e-06] excludes zero and halves the dry-season bias
(+59% -> +30% of mean flow) and the Q1 residual; SF Coquille² is not
significant and would contribute ~+1,175 cfs of phantom signal at the
predictor's observed max, so it is dropped. Final: r2=0.9366,
RMSE=360.4 cfs (vs 384.7 status quo, -6.3%).

Handles reference the existing gauges Suislaw_Mapleton_merge and
Coquille_Powers_merge; the calc_expression.csv row in kayak_data comes
with the gauge/reach metadata wiring.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…V flow

docs/migrations.md still pointed metadata edits at data/db/*.csv —
stale since the Phase-6 data-repo split moved the CSVs to the
kayak_data repo (METADATA_DIR). Repoint the five references; the
data/db/migrations/ path is unchanged (schema migrations stay in the
code repo).

Also: the regression report's maintainer-only section was renamed from
"## SQL stub" to "## `calc_expression` row" (b3509cc), but
deploy.py's _HTML_DROP_SECTIONS only matched the old heading — the new
Smith report would have leaked its calc-row scaffolding into the
published HTML. Add the new heading (keeping the old one for the
pre-redesign reports, which retain their SQL stubs as immutable
history) and a filter test for it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains

Copy link
Copy Markdown
Owner Author

Review for merging/deploying on the live server:

Finding:

  • Medium: the generated reproduce command omits the custom calc handles. In scripts/regression/gauge_pair_linear.py lines 564-581, cmd_parts includes predictors, target/window/name, and quadratic flags, but never includes --calc-handle. The new Smith report shows that mismatch: docs/regression/smith_14323100_from_siuslaw_sfcoquille.md lines 10-17 omit the handles, while the deployable row at lines 158-159 uses sm::Suislaw_Mapleton_merge and cp::Coquille_Powers_merge. Running the shown command would regenerate the row with the defaults (p1::14307620, p2::14325000), so the report is not bit-for-bit reproducible and a future regen could produce the wrong calc_expression.csv values. Please add the calc handles to the reproduce snippet, add a test using non-default handles, and regenerate the Smith report.

Notes:

  • docs/regression/README.md does not list the new Smith report in the index. Not deploy-blocking, but worth adding while the report is being regenerated.
  • Deployment ordering still matters: merge/deploy this code PR before, or in the same window as, the paired kayak_data metadata PR so the regression artifacts and HTML filter are live when the metadata row lands.

Verification:

  • pytest -q tests/test_scripts/test_gauge_pair_linear.py tests/test_build_deploy.py: 38 passed.
  • ruff check scripts/regression/gauge_pair_linear.py tests/test_scripts/test_gauge_pair_linear.py src/kayak/web/build/deploy.py tests/test_build_deploy.py: passed.
  • ruff format --check scripts/regression/gauge_pair_linear.py tests/test_scripts/test_gauge_pair_linear.py src/kayak/web/build/deploy.py tests/test_build_deploy.py: passed.
  • Local full fast suite: pytest -q produced 1106 passed / 1 skipped / 1 failure in tests/test_analytics_rdns.py::test_repeated_calls_dedup_targets_to_bound_budget; rerunning that test alone passed, so I’m treating it as an existing suite-order/shared-state flake rather than a PR regression.
  • GitHub Actions run 26978374008 completed successfully across lint, Python 3.13/3.14 tests, typecheck, audit, PHP/JS/CSS/shell checks, and PHPUnit.

Recommendation: hold merge until the reproduce-command/calc-handle issue is fixed. After that, this looks safe to merge and deploy with the paired metadata PR in the order above.

@mousebrains

Copy link
Copy Markdown
Owner Author

Review — verified against the live tree and live DB

Verdict: ready to merge. CI is green (mergeStateStatus: CLEAN), the branch contains current main tip (a4694d2), and every load-bearing claim I checked held up. Findings below are minor; none block.

Correctness — --quadratic-for mask threading (6d8947b)

Traced the mask through every consumer; all consistent:

  • build_design_matrix emits squared columns in predictor order for masked predictors only, and the expression-term walk in render_markdown (sq_idx = 1 + len(calc_handles), incremented only on masked predictors) consumes fit.coefs in exactly that order — indices line up.
  • design_row matches the matrix layout, so predict() mean-response/prediction CIs stay valid for partial masks; fit.x_means is per raw predictor (gauge_pair_linear.py:72), so the [x1, *fit.x_means[1:]] example-row construction can't over-length the mask.
  • JSON stays backward compatible: quadratic remains a bool (any(mask)), quadratic_sites is additive. Checked the PHP consumer — _render_gauge_regression() (php/includes/gauge_detail.php) never reads quadratic; it keys off provenance_slug and iterates coefs[]. No consumer break.
  • Bool specs pass through _quad_mask unchanged, so existing single/multi/--quadratic callers are unaffected (and the new tests pin that).

The Smith fit artifacts (961cfed)

  • Report expression vs. smith_…json coefficients agree to 6 sig figs (0.288564 / 0.130486 / 2.12545e-06 / −73.45 from −73.44625 at %.4g). Hand-check of the test-plan numbers reproduces: 0.288564·304 + 0.130486·83.3 + 2.12545e-06·304² − 73.45 = 25.34 → round → 25. ✓
  • The handles resolve: live DB has gauge 118 Suislaw_Mapleton_merge and gauge 16 Coquille_Powers_merge — the "Suislaw" spelling is the DB's, so _resolve_refs will match. round/greatest are both supported (_SAFE_FUNCS + greatest(max( normalization in cli/calculator.py), and the expression shape matches existing rows exactly.
  • Paired kayak_data ids check out: live MAX(id) is gauge 223 / source 346 / calc_expression 16, so 224/347/17 are exactly next.

HTML-filter fix (7210253) — good catch, and correctly sequenced

Confirmed the bug was real: _filter_regression_md_for_html matches via startswith, and "## SQL stub" would not have matched the renamed ## \calc_expression` row` heading — the column values (expression/note/provenance_slug) would have published. Both headings now covered, old reports still filtered, new test pins it. Since the fix and the first report carrying the new heading land in the same PR, the first build after pull has the filter active — no leak window.

Minor findings (non-blocking)

  1. _default_stability_starts docstring has the offsets backwards. It says {start-15y, start-10y, start-5y, start, start+5y, …} but the code (anchor + timedelta(days=365 * years_back) for years_back in (-5, 0, 5, 10, 15)) produces {start−5y, start, start+5y, start+10y, start+15y}. The Smith report's own stability table confirms (1962/1967/1972 starts — the +10y/+15y rows got capped at the window end). Behavior is unchanged from main (pure extraction), but the new docstring mis-describes it. One-line fix, fine as a follow-up.
  2. Reproduce snippet omits --calc-handle (pre-existing, surfaced by this report): re-running the Generated by command reproduces the fit bit-for-bit but emits p1::14307620-style handles, not sm::/cp:: — so "identical to the command that generated the file" isn't quite true. Worth appending the --calc-handle args to cmd_parts someday.
  3. _quad_spec_from_args error paths (mutually-exclusive, unknown site) are manually verified per the test plan but have no unit test — trivial to add alongside the existing six.

Deploy notes (live host)

  • Order: as the PR body says — merge this first, git pull on main in ~/kayak. The next scheduled pipeline run rebuilds and publishes the report HTML automatically; no manual levels build needed unless you want it live immediately.
  • kayak_data PR timing: after merging it, run levels sync-metadata on prod before the next nightly metadata snapshot (04:30 PT). snapshot_metadata.sh pulls kayak_data main and then re-exports the live DB; if the 4 rows aren't in the DB yet, the export will commit CSVs without them — silently reverting the merged kayak_data PR.
  • Until the metadata syncs, the published report is simply unlinked (no gauge carries the slug) — harmless in either ordering; gauge_detail.php also is_file()-guards the reverse case.

…andles

Address both PR #120 reviews:

- The "Generated by" snippet omitted --calc-handle, so re-running it
  on a report generated with custom handles would silently emit
  default p1::<site> handles into the calc_expression values — not
  bit-for-bit reproducible. Custom handles are now appended (defaults
  stay omitted; they self-reproduce). Smith report regenerated — fit
  unchanged, snippet now carries sm::/cp:: handles. Two snippet tests.

- _default_stability_starts docstring (and the pre-existing argparse
  help it was copied from) described the offsets backwards: the code
  sweeps {start-5y, start, +5y, +10y, +15y}, not {start-15y..+5y}.
  Docs now match the code; scripts/regression/README.md tightened too.

- Unit tests for the _quad_spec_from_args error paths (mutually
  exclusive flags, unknown site, mask construction).

- docs/regression/README.md index gains the Smith entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains

Copy link
Copy Markdown
Owner Author

All review findings addressed in 66bea7f:

  • Reproduce snippet now carries --calc-handle (the medium finding in both reviews). Custom handles are appended to cmd_parts; defaults stay omitted since p1::<site> self-reproduces. Smith report regenerated — fit byte-identical, snippet now shows --calc-handle sm::Suislaw_Mapleton_merge / cp::Coquille_Powers_merge. Two new snippet tests (custom-handles-present, defaults-omitted).
  • _default_stability_starts docstring fixed — and the pre-existing argparse --stability-starts help text it was copied from had the same backwards description ({start-15y..+5y} vs the code's {start-5y..+15y}); both now match the code, plus the README bullet.
  • _quad_spec_from_args error paths unit-tested (mutually-exclusive flags, unknown site, mask construction) — finding 3 of the second review.
  • Smith entry added to the docs/regression/README.md index.

Gate: ruff + format clean, 1100 passed (same 3 pre-existing local test_config.py env failures, green on CI). The stacked add-sfsalmon-calcs branch was rebased and its three reports regenerated with handle-carrying snippets.

Deploy-ordering notes acknowledged — code PR first, then kayak_data #2, with levels sync-metadata run on prod before the 04:30 PT snapshot.

🤖 Generated with Claude Code

@mousebrains

Copy link
Copy Markdown
Owner Author

Rereview of the updated PR head (66bea7f):

No blocking findings. The earlier merge-blocking issue is fixed: the generated Smith report now includes both custom --calc-handle arguments in the reproduce snippet, and the generator has regression tests for custom handles being included and default handles being omitted. The _quad_spec_from_args error paths are now unit-tested, and the stability-start docs/help now match the actual code.

Minor non-blocking note:

  • docs/regression/README.md lines 116-124: the Smith index entry was inserted before the McKenzie lead/lag verdict parenthetical, so the parenthetical now visually belongs to Smith even though Smith says it has no lead/lag companion. Move lines 123-124 back under the McKenzie entry, or drop that parenthetical since the sub-daily table already records the verdict. This does not affect deploy behavior.

Verification:

  • pytest -q tests/test_scripts/test_gauge_pair_linear.py tests/test_build_deploy.py: 41 passed.
  • ruff check scripts/regression/gauge_pair_linear.py tests/test_scripts/test_gauge_pair_linear.py src/kayak/web/build/deploy.py tests/test_build_deploy.py: passed.
  • ruff format --check scripts/regression/gauge_pair_linear.py tests/test_scripts/test_gauge_pair_linear.py src/kayak/web/build/deploy.py tests/test_build_deploy.py: passed.
  • Latest GitHub Actions run 26985602459 completed successfully across lint, Python 3.13/3.14 tests, typecheck, audit, PHP/JS/CSS/shell checks, PHPUnit, JS smoke tests, and gitleaks.

Recommendation: ready to merge and deploy after, or even before, the minor README index cleanup. Keep the deployment order from the earlier review: code PR first, then the paired kayak_data metadata PR with levels sync-metadata on prod before the next metadata snapshot.

@mousebrains

Copy link
Copy Markdown
Owner Author

Re-review of 66bea7f — follow-ups verified

CI still green (mergeStateStatus: CLEAN). All three findings from the previous review are addressed, and I verified each against the branch rather than the commit message:

  1. Reproduce snippet + calc handles ✅ — render_markdown now appends --calc-handle args whenever they differ from the p<i>::<site> defaults (defaults stay omitted and self-reproduce — right call, keeps old single-predictor reports' snippets byte-stable). The Smith report's Generated by block now carries sm::/cp::, and the .json/.svg are untouched in the commit — confirming the regen changed nothing about the fit. Both new snippet tests pin the behavior.
  2. Stability-sweep docs ✅ — docstring, the argparse --stability-starts help (a pre-existing instance of the same backwards description I hadn't flagged), and scripts/regression/README.md all now say {start−5y, start, +5y, +10y, +15y}, matching for years_back in (-5, 0, 5, 10, 15).
  3. _quad_spec_from_args tests ✅ — pass-through, mask order, mutual-exclusion → None + stderr, unknown-site → None + stderr. All paths covered.

One new nit (introduced by this commit)

docs/regression/README.md index — the Smith bullet is inserted one line too early. It lands between the McKenzie entry's lead/lag companion link and that entry's (verdict: …) continuation lines, so the McKenzie verdict ("the sub-daily signal is real but lives in downstream look-ahead…") now renders as the tail of the Smith bullet — directly contradicting the Smith text two lines up ("No lead/lag companion — the target predates USGS unit values"). Move the Smith bullet below the two verdict lines (or the verdict lines up). Two-line fix; docs-only, non-blocking, but worth folding in before merge since it actively misleads.

Bonus cross-check — paired kayak_data PR (add-smith-gardiner)

Fetched the kayak_data branch and verified the metadata rows against this PR's artifacts:

  • calc_expression 17: expression and time_expression match the report's column values verbatim, and the note is fully fleshed out per the report's instruction (window rationale, Siuslaw²-only justification with CIs, the six rejected donors, no DA scaling, doc pointer). ✓
  • gauge 224 Smith_Gardiner_calc — matches the name the new README index entry cites; coords/HUC/state populated. ✓
  • source 347 Smith_Gardiner_calc with calc_expression_id=17, plus the gauge_source 224↔347 link. ✓
  • id_counters.csv bumped correctly (source 347→348, gauge 224→225, calc_expression 17→18 — increment-only). ✓

Verdict

Ready to merge — ideally with the 2-line index fix, but nothing here blocks. Deploy notes from the previous review stand, in particular: after the kayak_data PR merges, run levels sync-metadata on prod before the next 04:30 PT metadata snapshot, or the export will commit CSVs without the four rows and revert them.

…index entry

The Smith bullet landed between the McKenzie entry's lead/lag link and
its "(verdict: ...)" continuation, making the McKenzie verdict read as
the Smith bullet's tail — contradicting "No lead/lag companion" two
lines up. Move the verdict lines back under the McKenzie entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains

Copy link
Copy Markdown
Owner Author

Index nit fixed in 7f6c700 — the McKenzie (verdict: …) continuation lines are reattached under the McKenzie entry, Smith bullet now sits cleanly after them. The stacked add-sfsalmon-calcs branch was rebased over the fix (its three index entries slot in below the Smith one, conflict resolved so no verdict lines dangle).

🤖 Generated with Claude Code

@mousebrains
mousebrains merged commit 8d29a02 into main Jun 4, 2026
8 checks passed
@mousebrains
mousebrains deleted the add-smith-gardiner branch June 4, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant