Skip to content

feat(usace): scale kcfs flow series to cfs - #93

Merged
mousebrains merged 1 commit into
mainfrom
usace-cda-kcfs-temp
May 30, 2026
Merged

feat(usace): scale kcfs flow series to cfs#93
mousebrains merged 1 commit into
mainfrom
usace-cda-kcfs-temp

Conversation

@mousebrains

@mousebrains mousebrains commented May 30, 2026

Copy link
Copy Markdown
Owner

Makes the usace.cda parser units-aware so the lower-Columbia dams' outflow stores correctly — a prerequisite for wiring USACE outflow onto the John Day and Bonneville gauges (companion PR #94).

What

  • parse_records reads each timeseries' units field and scales kcfs → cfs in _entry_to_record. The Willamette dams (GPR/FAL) use the .Inst.0.0.Best flow series in cfs and are unchanged; the lower-Columbia dams only publish the .Ave.1Hour.1Hour series in kcfs, which would otherwise store 1000× too small.
  • Aligns the code with its own class docstring ("unit conversion from kcfs to cfs") and fixes the contradictory module docstring.

Tests

kcfs→cfs scaling (pure parse_records + end-to-end on a stored observation), and cfs unchanged (GPR regression). Full suite green (991 passed).

Deploy

Deploy before PR #94, or the new USACE outflow lands 1000× small.

(Scope trimmed during review: an earlier revision also added USACE water-temperature support, dropped once we decided to wire USACE outflow only — the dam temps duplicated the gauges' existing USGS sensors.)

🤖 Generated with Claude Code

@mousebrains

mousebrains commented May 30, 2026

Copy link
Copy Markdown
Owner Author

Review (updated for the scope trim) — ready to merge ✅

Re-reviewed after the simplification: the PR is now a single commit and the parser change is just the kcfs→cfs unit scalingTemp-Water support was dropped (companion #94 now wires outflow only), so the _PARAM_MAP addition, the Temp-Water / John Day dual-key tests, and the property-test edit are all gone (4 files → 3). Good trim: smaller surface, and it drops the only speculative bit (temperature handling no consumer needed).

Verified

  • Scaling logic is correct & conservative: units == "kcfs" ⇒ ×1000; cfs / ft / missing-unit pass through unchanged. Keyed off the per-timeseries units string.
  • Willamette regression preserved: the .Inst.0.0.Best series reports cfs, so GPR/FAL are untouched — covered by test_parse_records_cfs_flow_not_scaled.
  • Temperature now simply isn't mapped (_PARAM_MAP unchanged), so a Temp-Water series falls through data_type is None → continue and is skipped — consistent with feat(columbia): retrim lower-river gauges — USACE dam outflow, NWS stage, drop Bridgeport #94 wiring flow only.
  • Tests cover kcfs→cfs (pure parse_records + an end-to-end parse→store on BON) and the cfs-unchanged regression. CI is green.

Self-contained

No fetch URLs are wired here (that's #94), so merging is a no-op in prod until #94 lands. Safe to merge first.

Minor (non-blocking)

  • units = (ts_info.get("units") or "").lower() has no .strip(); a stray "kcfs " would silently skip scaling. Defensive only — the feed's unit strings are clean.

Merge order

Merge/deploy before #94.

(Supersedes my earlier comments on this PR, which were written before the temp-scope trim — and before that, corrected for a tool-output glitch. This reflects the current single-commit state.)


🤖 Review by Claude Code

@mousebrains
mousebrains force-pushed the usace-cda-kcfs-temp branch from 0935258 to cbf95da Compare May 30, 2026 17:40
@mousebrains mousebrains changed the title feat(usace): scale kcfs flow to cfs and ingest water temperature feat(usace): scale kcfs flow series to cfs May 30, 2026
mousebrains added a commit that referenced this pull request May 30, 2026
…res)

Review decision: wire only USACE Flow-Out at John Day and Bonneville, not
water temperature.
- John Day's USACE temp (JDY, "John Day Dam Water Quality") is an at-dam
  sensor ~3 mi upstream of the gauge's downstream USGS temp
  (454249120423500, "near Cliffs") -- different water.
- Bonneville's USACE temp (BON) is co-located with the gauge's USGS Cascade
  Island temp (~0.06 F apart) -- pure redundancy.

Both gauges keep their existing USGS temperature; USACE adds only the flow
(discharge) reading they lacked. Both query URLs drop to Flow-Out alone, and
0069 is renamed ...flow_temp -> ...outflow. The companion parser PR (#93) is
likewise trimmed to the kcfs->cfs flow fix.

Re-verified on a fresh prod copy: migrate applies 0069-0071, orphan-check
clean, both USACE sources flow-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The USACE getjson parser stored every value raw ("Values are already in
CFS"), which holds for the two Willamette dams (GPR/FAL) whose
`.Inst.0.0.Best` flow series is in cfs. But the lower-Columbia dams only
publish the `.Ave.1Hour.1Hour` series in **kcfs**, so their outflow would
land 1000x too small.

Make `parse_records` units-aware: read each timeseries' `units` field and
scale `kcfs` -> cfs in `_entry_to_record` (cfs/ft pass through unchanged, so
GPR/FAL are untouched). Aligns the code with its own class docstring's
"unit conversion from kcfs to cfs" and fixes the contradictory module
docstring.

Tests: kcfs Flow-Out -> cfs (pure parse + end-to-end on a stored
observation) and cfs Flow-Out unchanged (GPR regression).

Prerequisite for wiring USACE outflow onto the John Day and Bonneville
Columbia gauges.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains
mousebrains force-pushed the usace-cda-kcfs-temp branch from cbf95da to 3ace9a7 Compare May 30, 2026 17:56
@mousebrains

Copy link
Copy Markdown
Owner Author

Addressed the non-blocking nit: units now uses .strip().lower(), so a stray " kcfs " (trailing space / odd case) still scales — with a test_parse_records_kcfs_units_tolerate_whitespace_and_case regression test. Amended to keep the single-commit shape; CI re-running.

@mousebrains
mousebrains merged commit 64c34ce into main May 30, 2026
8 checks passed
mousebrains added a commit that referenced this pull request May 30, 2026
…age, drop Bridgeport (#94)

Retrim the Columbia mainstem corridor to the downstream-of-John-Day-Dam scope
(docs/PLAN_add_gauges_reaches.md; the migration + nightly-snapshot spine).

- 0069: wire USACE outflow (cfs) onto the John Day (454249120423500) and
  Bonneville (Bonneville_merge) gauges, which lacked a flow reading. Flow only;
  temperature stays on the existing USGS sensors.
- 0070: add NWS stage gauges Vancouver (VAPW1, RM 106) and St. Helens (SHNO3,
  RM 86), state='OR,WA' + HUC8 17080003, river-mile sort_name.
- 0071: drop Bridgeport (12438000, RM 544, above John Day Dam, out of scope),
  by-name source DELETE recognized by the R4.4 guard's new _deleted_sources().

Adds the 4 fetch URLs to sources.yaml; JDA/BON/VAPW1/SHNO3 in
PENDING_RECONCILIATION until the nightly snapshot lands them in source.csv.
Documents a "Shape 4 — dropping a gauge" section in the runbook.

Requires the kcfs->cfs parser fix (#93), merged first.
mousebrains added a commit that referenced this pull request May 31, 2026
* docs: round-6 deep project review (graded B+, ▲ from B−)

Sixth deep project review of the entire tracked repo — 6 cold facet
auditors (Python, PHP/security, schema/data, tests/CI, ops, docs) +
synthesizer hand-re-verification, judging two bands: (A) did round-5's
fixes durably stick, and (B) what did #93#98 + migrations 0069–0071 +
the two direct-to-main commits introduce.

The recursive integrity check passes cleanly for the first time in the
series: every round-5 fix (R1.1/R1.2/R1.3/R1.5/R2.1/R3.x/R4.x) landed as
a committed PR and is still present at HEAD, and every mechanized guard
is proven non-vacuous by break-it experiment. New code is clean — no
CRIT/HIGH: #93 USACE kcfs→cfs (correct, per-series), migrations
0069/0070/0071 (idempotent, FK-clean, Bridgeport DROP cascade
residue-free), #96/#97 multi-state pickers, #95/#98 gradient JS.

Two MED findings, both recurrences of round-5 classes closed by
documentation not mechanization: (1) two direct-to-main commits, one of
which broke CI on main (the {}-is-a-dict bug); (2) a nightly snapshot
overrode migration 0067's sort_name for gauge 217 with no migration.
Root cause is shared — main accepts un-CI-gated direct pushes from both
humans and the snapshot bot. Lever: route everything through a CI gate
(branch protection + a self-gating/auto-merging snapshot), a
snapshot-column drift guard, and teach seed_gauge_display to preserve
migration-pinned sort_names.

Two facet over-claims dissolved on hand-re-verification (the USACE
temperature-docstring drop is a correct fix; check_reaches DOES
range-check vertices via validate_lat_lon).

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

* docs: fold external-review corrections into the round-6 review (#99)

The PR #99 external verification pass re-confirmed every finding,
severity, and the B+ grade against db34ae0 (recommendation: merge), and
flagged one inaccurate evidence line plus three off-by-one citations.
Corrected:

  - MED #1: drop the `git branch --contains` "reachable only from main"
    claim — feature branches later cut from main now contain 9b428bb /
    6007c21, so containment no longer distinguishes them. The direct-to-
    main conclusion stands on the durable evidence (linear f3ed673..HEAD,
    no merge commit, missing (#NN) suffix).
  - citations: ci.yml:114→115, SourceUrlTest.php:83-84→84-85,
    check_reaches.py:212→213.

Added an External-review note recording the pass + the one below-LOW item
it surfaced (the 0069/0070 header comments' now-stale PENDING_RECONCILIATION
wording).

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

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mousebrains
mousebrains deleted the usace-cda-kcfs-temp branch June 1, 2026 18:01
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