diff --git a/DESCRIPTION b/DESCRIPTION index 949b0f75..fca4b10d 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: link Title: Stream Network Habitat Interpretation (Experimental) -Version: 0.40.3 +Version: 0.40.4 Date: 2026-05-23 Authors@R: c( person("Allan", "Irvine", , "airvine@newgraphenvironment.com", diff --git a/NEWS.md b/NEWS.md index 4cb070fe..0f4edaae 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,9 @@ +# link 0.40.4 + +Reproduce bcfp's per-species accessibility so dam-downstream segments emit the dam descriptor ([#200](https://github.com/NewGraphEnvironment/link/issues/200)). The mapping_code phase previously drove `accessible` from `barriers__unified` (all barriers, including dams), so every segment below a dam read inaccessible and lost its `;DAM`/`;MODELLED`/`;ASSESSED` second token — emitting a bare `SPAWN`/`REAR` where bcfp emits `SPAWN;DAM`. It now uses a new per-species `barriers__access` view that reproduces bcfp's `barriers_`: natural barriers only (gradient at the species threshold ∪ falls ∪ subsurface), minus the observation/habitat override, plus all user-definite barriers (override-exempt). Dams stay in `barrier_sources` and annotate token2 only. + +All three access inputs are now persisted province-wide so the cross-WSG downstream walk is correct in every watershed group, not just the run's own: natural barriers (already), `user_barriers_definite` (new `USER_DEFINITE` family in `lnk_barriers_unify`, ltree-resolved via the FWA join like falls), and the observation/habitat override (new `.barrier_overrides` table). Validated against `bcfishpass@v0.7.15`: PARS BT 98.95%, LFRA BT 97.77% / CO 97.90% per-segment mapping_code match. See `RUNBOOK.md` §5. + # link 0.40.3 Persist the per-source downstream-barrier flag columns in `streams_access` so `lnk_pipeline_mapping_code`'s second token (`DAM`/`MODELLED`/`ASSESSED`/`REMEDIATED`/`NONE`) populates from persisted state instead of defaulting to `NONE`. Three coupled fixes ([#196](https://github.com/NewGraphEnvironment/link/issues/196)): `lnk_persist_init` adds the six flag columns to the `streams_access` DDL; `lnk_pipeline_run` pre-persists barriers before the mapping_code phase for cross-WSG dam visibility (link#152); `lnk_pipeline_persist` projects the flag columns in the INSERT (the DDL/INSERT pair must match — the missing projection was the actual `NONE`-token bug). diff --git a/R/lnk_barriers_unify.R b/R/lnk_barriers_unify.R index 82abda5a..0b5c0e4c 100644 --- a/R/lnk_barriers_unify.R +++ b/R/lnk_barriers_unify.R @@ -33,6 +33,13 @@ #' - **Subsurface_flow** (`barrier_source = 'SUBSURFACE_FLOW'`, from #' `.barriers_subsurfaceflow`): blocks all 8 species. Opt-in #' (only built when `cfg$pipeline$break_order` includes `"subsurfaceflow"`). +#' - **User_definite** (`barrier_source = 'USER_DEFINITE'`, from +#' `.barriers_definite`): blocks all 8 species. ltrees + +#' `linear_feature_id` resolved by JOIN to +#' `whse_basemapping.fwa_stream_networks_sp` (mirrors the FALLS branch +#' and bcfp's `barriers_user_definite.sql`). Persisted province-wide so +#' the per-species access view (`barriers__access`, +#' [lnk_barriers_views()]) can include them override-exempt — link#200. #' #' Remediations (PASSABLE remediation crossings) are intentionally NOT #' in this table. They're consumed via `.barriers_remediations` @@ -67,6 +74,8 @@ #' - `.crossings` (from [lnk_pipeline_crossings()]). #' - `.gradient_barriers_raw` (from [lnk_pipeline_prepare()]). #' - `.falls` (from [lnk_pipeline_prepare()]). +#' - `.barriers_definite` (from [lnk_pipeline_prepare()]; always +#' created, may be empty). #' #' Optional: #' - `.barriers_subsurfaceflow` (only when the config opts in). @@ -241,7 +250,41 @@ lnk_barriers_unify <- function(conn, aoi, cfg, loaded, WHERE f.watershed_group_code = %3$s", all_species_arr, schema, aoi_lit) # nolint: indentation_linter - union_parts <- c(sql_anth, sql_gradient, sql_falls) + # Source 4: user-definite barriers. id_barrier = 'USER_DEFINITE-' || + # row_number(). .barriers_definite (built in lnk_pipeline_prepare + # from user_barriers_definite.csv) carries blue_line_key + + # downstream_route_measure only — the empty-fallback shape has just those + # two columns — so reference ONLY those from `d` and JOIN to FWA for + # ltrees + linear_feature_id + watershed_key, exactly as the FALLS branch + # does. Mirrors bcfp model/01_access/sql/barriers_user_definite.sql. + # blocks_species = all (a user-definite barrier blocks every species). + # These are override-EXEMPT: the per-species access view + # (barriers__access) never lifts them via barrier_overrides — bcfp + # unions all user_definite AFTER the observation/habitat filter ("include + # *all* user added features, even those below observations"). link#200. + sql_user_definite <- sprintf(" + SELECT + ('USER_DEFINITE-' || row_number() OVER ()::text)::text AS id_barrier, + %1$s::varchar(4) AS watershed_group_code, + 'USER_DEFINITE'::text AS barrier_source, + 'user_definite'::text AS barrier_subtype, + 'BARRIER'::text AS passability, + %2$s AS blocks_species, + s.linear_feature_id, + d.blue_line_key, + s.watershed_key, + d.downstream_route_measure::double precision, + s.wscode_ltree, + s.localcode_ltree, + ST_Force2D(FWA_LocateAlong(d.blue_line_key, d.downstream_route_measure))::geometry(Point, 3005) AS geom + FROM %3$s.barriers_definite d + JOIN whse_basemapping.fwa_stream_networks_sp s + ON d.blue_line_key = s.blue_line_key + AND d.downstream_route_measure >= s.downstream_route_measure + AND d.downstream_route_measure < s.upstream_route_measure", + aoi_lit, all_species_arr, schema) # nolint: indentation_linter + + union_parts <- c(sql_anth, sql_gradient, sql_falls, sql_user_definite) if (has_subsurface) { sql_subsurface <- sprintf(" diff --git a/R/lnk_barriers_views.R b/R/lnk_barriers_views.R index 62f0d2f1..8dd61edd 100644 --- a/R/lnk_barriers_views.R +++ b/R/lnk_barriers_views.R @@ -13,12 +13,24 @@ #' Fixes the PARS BT 60% defect (PARS drains through dams in PCEA / #' UPCE WSGs) and unblocks any regional run. #' -#' Per-species views: -#' - `.barriers__unified` for each species (`bt`, `ch`, `cm`, -#' `co`, `pk`, `sk`, `st`, `wct`) — filtered by -#' `'' = ANY(blocks_species)`. `_unified` suffix avoids name -#' collision with the per-WSG `.barriers_` tables that -#' `.lnk_pipeline_prep_minimal` builds for the break-time path. +#' Per-species views (two per species `bt`, `ch`, `cm`, `co`, `pk`, `sk`, +#' `st`, `wct`): +#' - `.barriers__unified` — filtered by +#' `'' = ANY(blocks_species)` (ALL barrier families incl dams). +#' `_unified` suffix avoids name collision with the per-WSG +#' `.barriers_` tables that `.lnk_pipeline_prep_minimal` +#' builds for the break-time path. +#' - `.barriers__access` — the per-species **accessibility** +#' set that drives `accessible` in mapping_code (link#200). Reproduces +#' bcfp `barriers_`: NATURAL barriers only +#' (`barrier_source IN ('GRADIENT','FALLS','SUBSURFACE_FLOW')` — the +#' gradient-at-species-threshold is already encoded in `blocks_species`) +#' MINUS the observation/habitat override (anti-join `barrier_overrides`), +#' ∪ all `USER_DEFINITE` (override-exempt). Dams/PSCIS/modelled are +#' excluded — they annotate token2 via the per-source views, never block +#' access. Same feature shape (`barriers__access_id` + ltrees + geom) +#' so [lnk_pipeline_access()] / [fresh::frs_network_features()] consume it +#' unchanged. See `RUNBOOK.md` §5. #' #' Per-source views (matching the bcfp source-typed tables consumed by #' the `barrier_sources` arg of `lnk_pipeline_access`): @@ -47,8 +59,9 @@ #' either way (the underlying table has `blocks_species` from #' [lnk_barriers_unify()]). #' -#' @return `invisible(conn)`. Side effect: drops + recreates one view -#' per species + three source-typed views in `schema`. +#' @return `invisible(conn)`. Side effect: drops + recreates two views +#' per species (`_unified` + `_access`) + three source-typed views in +#' `schema`. #' #' @details #' Views are dropped + recreated on each call (`CREATE OR REPLACE VIEW`) @@ -112,6 +125,13 @@ lnk_barriers_views <- function(conn, schema, cfg, paste0(tn$schema, ".barriers") } + # The per-species access view (barriers__access) anti-joins the + # barrier-overrides skip list. Read it from the SAME schema as the + # barriers source so persist-barriers pairs with the province-wide + # overrides (link#200) and a working-schema barriers_table (if ever + # passed) pairs with that schema's per-WSG overrides. link#200. + overrides_table <- sub("\\.barriers$", ".barrier_overrides", persist_barriers) + # Per-species views. Each view re-exposes id_barrier as # `barriers__unified_id` so fresh::frs_network_features's # `feature_id_col = "_id"` convention works unchanged. @@ -137,6 +157,43 @@ lnk_barriers_views <- function(conn, schema, cfg, ) .lnk_db_execute(conn, sprintf("DROP VIEW IF EXISTS %s", view_name)) .lnk_db_execute(conn, sql_view) + + # Per-species ACCESS view (link#200). The set that drives `accessible` + # in mapping_code — reproduces bcfp `barriers_`: NATURAL barriers + # only (gradient at the species threshold — already encoded in + # blocks_species — ∪ falls ∪ subsurface) MINUS the observation/habitat + # override, ∪ all user_definite (override-EXEMPT). Dams/PSCIS/modelled + # are NOT here (they annotate token2 via barrier_sources, never block + # access). Same feature shape as `_unified` (id + ltrees + geom) so + # fresh::frs_network_features walks it unchanged. The override anti-join + # reads province-wide `overrides_table` so a natural barrier in ANY WSG + # a downstream walk crosses is lifted correctly. See RUNBOOK.md §5. + access_view <- paste0(schema, ".barriers_", sp_lower, "_access") + access_id <- paste0("barriers_", sp_lower, "_access_id") + sql_access <- sprintf( + "CREATE OR REPLACE VIEW %s AS + SELECT id_barrier AS %s, + barrier_source, barrier_subtype, passability, + blocks_species, + linear_feature_id, blue_line_key, watershed_key, + downstream_route_measure, wscode_ltree, localcode_ltree, + watershed_group_code, geom + FROM %s b + WHERE %s = ANY(blocks_species) + AND barrier_source IN ('GRADIENT', 'FALLS', 'SUBSURFACE_FLOW', 'USER_DEFINITE') + AND ( + barrier_source = 'USER_DEFINITE' + OR NOT EXISTS ( + SELECT 1 FROM %s o + WHERE o.species_code = %s + AND o.blue_line_key = b.blue_line_key + AND abs(o.downstream_route_measure - b.downstream_route_measure) < 1 + ) + )", + access_view, access_id, persist_barriers, sp_lit, overrides_table, sp_lit + ) + .lnk_db_execute(conn, sprintf("DROP VIEW IF EXISTS %s", access_view)) + .lnk_db_execute(conn, sql_access) } # Per-source views — unified (cross-WSG) shape exposed under a diff --git a/R/lnk_persist_init.R b/R/lnk_persist_init.R index 33f831f6..68931e2e 100644 --- a/R/lnk_persist_init.R +++ b/R/lnk_persist_init.R @@ -154,6 +154,24 @@ cols_barriers <- c( geom = "geometry(Point, 3005)" ) +#' Column shape for the province-wide barrier-overrides table. +#' +#' `.barrier_overrides` holds the per-(segment x species) +#' observation/habitat barrier-skip list (`lnk_barrier_overrides` output) +#' accumulated across all WSGs. Persisted province-wide so the per-species +#' access view (`barriers__access`, [lnk_barriers_views()]) can +#' anti-join it for natural barriers in ANY WSG a downstream walk crosses — +#' the cross-WSG twin of the link#152 barriers fix. One named vector drives +#' both the DDL ([lnk_persist_init()]) and the INSERT projection +#' ([lnk_pipeline_persist()]). link#200. +#' @noRd +cols_barrier_overrides <- c( + blue_line_key = "integer NOT NULL", + downstream_route_measure = "double precision NOT NULL", + species_code = "text NOT NULL", + watershed_group_code = "varchar(4) NOT NULL" +) + #' Validate that an existing target table doesn't carry stale DDL #' (unexpected `GENERATED ALWAYS` columns). #' @@ -309,6 +327,8 @@ lnk_persist_init <- function(conn, cfg, species, force_recreate = FALSE) { } .lnk_validate_persist_table(conn, schema = schema, table = "barriers", force_recreate = force_recreate) + .lnk_validate_persist_table(conn, schema = schema, table = "barrier_overrides", + force_recreate = force_recreate) .lnk_validate_persist_table(conn, schema = schema, table = "streams_access", force_recreate = force_recreate) .lnk_validate_persist_table(conn, schema = schema, @@ -375,6 +395,27 @@ lnk_persist_init <- function(conn, cfg, species, force_recreate = FALSE) { idx_name, schema, barriers_idx_specs[[idx_name]])) } + # Province-wide barrier-overrides table (link#200). The per-(segment x + # species) observation/habitat barrier-skip list, accumulated per-WSG so + # the per-species access view can anti-join it cross-WSG. PK includes + # watershed_group_code (mirrors cols_barriers' (id_barrier, wsg) PK): + # the SAME override position can be computed by two adjacent WSG runs + # (boundary streams whose blue_line_key spans WSGs), so (blk, drm, + # species) is NOT unique across WSGs. Per-WSG DELETE-WHERE-WSG + INSERT + # stays clean; the access anti-join is WSG-agnostic so duplicates are + # harmless. + barrier_overrides_pk <- c("blue_line_key", "downstream_route_measure", + "species_code", "watershed_group_code") + .lnk_db_execute(conn, sprintf( + "CREATE TABLE IF NOT EXISTS %s.barrier_overrides (\n %s\n)", + schema, .lnk_cols_clause(cols_barrier_overrides, barrier_overrides_pk))) + .lnk_db_execute(conn, sprintf( + "CREATE INDEX IF NOT EXISTS barrier_overrides_wsg_idx ON %s.barrier_overrides (watershed_group_code)", + schema)) + .lnk_db_execute(conn, sprintf( + "CREATE INDEX IF NOT EXISTS barrier_overrides_blk_drm_idx ON %s.barrier_overrides (blue_line_key, downstream_route_measure)", + schema)) + # Per-segment per-species access table (link#187). Wide shape (one # row per segment, all species columns inline) — matches the QGIS- # consumer expectation + bcfp's `bcfishpass.streams_access` shape. diff --git a/R/lnk_pipeline_persist.R b/R/lnk_pipeline_persist.R index 62e03e96..63444e66 100644 --- a/R/lnk_pipeline_persist.R +++ b/R/lnk_pipeline_persist.R @@ -100,6 +100,38 @@ lnk_pipeline_persist <- function(conn, aoi, cfg, species, barriers_table, barriers_cols, barriers_cols, schema)) } + # ----- barrier_overrides (link#200) ----- + # Per-(segment x species) observation/habitat skip list. Working + # .barrier_overrides (built by lnk_barrier_overrides in + # lnk_pipeline_prepare) carries (blue_line_key, downstream_route_measure, + # species_code) only — add the AOI as watershed_group_code in the SELECT. + # Persisted province-wide so barriers__access can anti-join it for + # natural barriers in any WSG a downstream walk crosses. Probe-gated like + # barriers above. + overrides_present <- nrow(DBI::dbGetQuery(conn, sprintf( + "SELECT 1 FROM information_schema.tables + WHERE table_schema = %s AND table_name = 'barrier_overrides' + LIMIT 1;", + DBI::dbQuoteString(conn, schema) + ))) > 0L + if (overrides_present) { + overrides_table <- paste0(tn$schema, ".barrier_overrides") + overrides_cols <- paste(names(cols_barrier_overrides), collapse = ", ") + # SELECT projection: WSG is the literal AOI (working table lacks it); + # the other three come straight from the working override table. + overrides_select <- vapply(names(cols_barrier_overrides), function(col) { + if (col == "watershed_group_code") paste0(aoi_lit, "::varchar(4)") + else col + }, character(1)) + .lnk_db_execute(conn, sprintf( + "DELETE FROM %s WHERE watershed_group_code = %s", + overrides_table, aoi_lit)) + .lnk_db_execute(conn, sprintf( + "INSERT INTO %s (%s) SELECT %s FROM %s.barrier_overrides", + overrides_table, overrides_cols, + paste(overrides_select, collapse = ", "), schema)) + } + # ----- streams_access (link#187) ----- # Per-segment per-species access. Working `.streams_access` is # built by lnk_pipeline_access; persist phase copies the per-WSG slice diff --git a/R/lnk_pipeline_run.R b/R/lnk_pipeline_run.R index 32f40aad..fbc8dc69 100644 --- a/R/lnk_pipeline_run.R +++ b/R/lnk_pipeline_run.R @@ -198,22 +198,21 @@ lnk_pipeline_run <- function(conn, aoi, cfg, loaded, species = active_species) # 2. Per-segment access. barriers_per_sp drives `accessible` (token1 - # ACCESS + token2 gate). KNOWN-DIVERGENT from bcfp — see RUNBOOK.md §5. - # bcfp's per-species access set (`barriers_`, built in - # smnorris/bcfishpass model/01_access/sql/model_access_bt.sql) is - # NATURAL barriers only (gradient at the species threshold + falls + - # subsurface), MINUS barriers with upstream observations / confirmed - # habitat, plus user_definite — dams are NEVER in it (they annotate - # access via token2, not block it). link's `_unified` views instead - # carry ALL barriers incl dams (blocks_species universal, §2a), so - # dam-downstream segments read inaccessible and lose their `;DAM` - # token. The correct fix is a natural-only per-species *feature* view - # with the override applied (reproducing bcfp barriers_) — real - # work, scoped separately, not a table swap. `_min` cannot be used - # here: it is a break-spec with no id column (RUNBOOK §2b). + # ACCESS + token2 gate). Uses the per-species `barriers__access` + # views (link#200) — these reproduce bcfp's per-species access set + # (`barriers_`, smnorris/bcfishpass model/01_access/sql/ + # model_access_bt.sql): NATURAL barriers only (gradient at the species + # threshold + falls + subsurface), MINUS the observation/habitat + # override, ∪ user_definite (override-exempt). Dams/PSCIS/modelled are + # NOT in the access set — they annotate access via token2 + # (barrier_sources below), never block it. So a dam-downstream segment + # that is otherwise accessible stays accessible and emits `;DAM`. All + # inputs are province-wide-persisted (barriers + barrier_overrides via + # the pre-persist above), so the cross-WSG downstream walk is correct in + # every WSG, not just the run's own. See RUNBOOK.md §5. sp_set <- tolower(active_species) barriers_per_sp <- setNames( - lapply(sp_set, function(sp) paste0(schema, ".barriers_", sp, "_unified")), + lapply(sp_set, function(sp) paste0(schema, ".barriers_", sp, "_access")), sp_set) lnk_pipeline_access(conn, # nolint: object_usage_linter diff --git a/RUNBOOK.md b/RUNBOOK.md index 49037926..13a44782 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -95,13 +95,18 @@ fractional thresholds via `.lnk_classes_bcfp` (`lnk_pipeline_prepare.R`): `class_value ≥ s$access_gradient_max`. BT's `access_gradient_max` is 0.25, so a 2500-class gradient blocks BT; CH/CO/SK at 0.15 are blocked from 1500 up. -**Key consequence: dams block *all* species universally.** There is no +**Key consequence: dams block *all* species in `blocks_species`.** There is no per-species dam rule in any config file. A dam (CABD, via the anthropogenic -family) gets `blocks_species = {all species}`. **This is the #196 bug.** bcfp -does NOT put dams in the per-species *access* set at all (§5) — they're a -downstream *descriptor*, not an accessibility barrier. link conflating the two -in `blocks_species` is the root cause; see §5 for the authoritative bcfp -mechanism and the fix shape. +family) gets `blocks_species = {all species}`. This was the #196 dam-token bug. +bcfp does NOT put dams in the per-species *access* set at all (§5) — they're a +downstream *descriptor*, not an accessibility barrier. **Fixed in #200/v0.40.4:** +accessibility no longer reads `blocks_species` over all barriers. It reads the +per-species `barriers__access` view (§5), which filters to NATURAL sources +only (`barrier_source IN ('GRADIENT','FALLS','SUBSURFACE_FLOW','USER_DEFINITE')`) +— so dams are excluded from access and (correctly) annotate token2 only. +`blocks_species` is still computed for all families (it's how the `_access` view +gets the per-species gradient threshold for free); the access view just ignores +the anthropogenic rows. Remediations (PASSABLE) are **not** in `blocks_species`. They flow separately via `.barriers_remediations` for the sequence-aware `remediated_dnstr_ind`. @@ -132,9 +137,11 @@ exist`.) `barriers_per_sp` mechanically requires the *feature view* shape. Computes `.streams_access` — per-segment, per-species accessibility plus downstream barrier-source flags. Two distinct inputs, two distinct roles: -- **`barriers_per_sp`** — named list `sp → barriers__unified`. Drives - `has_barriers__dnstr` (is a blocking barrier downstream for this species). - This is **accessibility** — feeds `accessible` in mapping_code. Each table's +- **`barriers_per_sp`** — named list `sp → barriers__access` (#200; was + `_unified` pre-v0.40.4). Drives `has_barriers__dnstr` (is a blocking + *natural* barrier downstream for this species, override-applied). This is + **accessibility** — feeds `accessible` in mapping_code. NATURAL-only + + override-filtered + user_definite (§5) — dams are NOT here. Each table's feature id is derived as `
_id` and passed to `frs_network_features`. - **`barrier_sources`** — named list of source-typed feature tables (`anthropogenic`, `pscis`, `dams`, `remediations`). Drives the @@ -224,38 +231,54 @@ mapping_code (`load_streams_mapping_code.sql`) gates the barrier token on `spawning_bt > 0` (token1 SPAWN) AND `dam_dnstr_ind = true` (a dam is downstream → token2 DAM). The dam doesn't block access; it annotates it. -### Where link diverges (the #196 bug, fully characterized) - -link's `barriers_per_sp = barriers__unified` = **all** barriers (incl dams, -PSCIS, modelled) WHERE species ∈ `blocks_species` (§2a). Two wrongs: - -1. **Wrong content.** It includes dams/anthropogenic. bcfp's `barriers_` is - natural-only. So every PARS segment below a dam reads `has_barriers_bt_dnstr - = TRUE` → `accessible = FALSE` → token2 `;DAM` suppressed → bare `SPAWN`. -2. **No override applied.** bcfp removes barriers with upstream observations / - confirmed habitat. link HAS this logic — `lnk_barrier_overrides` - (`lnk_pipeline_prepare.R:519`) — but its output `.barrier_overrides` - feeds only `lnk_pipeline_classify` (habitat gating), NOT - `lnk_pipeline_access` (mapping_code accessibility). So link's habitat token - (SPAWN/REAR) is override-aware but its `accessible` flag is not. - -Note: link's `access_` integer (`lnk_pipeline_access.R:364`, -`ifelse(blocked, 0, ifelse(observed, 2, 1))`) IS observation-aware and matches -bcfp's `access_bt` code — but mapping_code reads `has_barriers__dnstr`, not -`access_`. - -### The fix shape - -`barriers_per_sp` must reproduce bcfp's `barriers_`: a **feature-shaped** -(has-id, §2b) per-species view of **natural barriers only** — gradient at the -species' threshold + falls + subsurface — with the observation/habitat override -applied and `user_barriers_definite` unioned in. Dams stay in `barrier_sources` -(token2 only). This is real work (a per-species access-barrier builder), not a -table swap. Confirm with `fresh.streams_vw_bcfp` once it loads (the snapshot's -streams view failed on a transient gzip error 2026-05-23; retry). - -What does **NOT** work: `barriers__min` (break-spec, no id, §2b) — though its -*content* (gradient+falls) is close to the right base. +### Where link diverged, and how #200/v0.40.4 fixed it + +The #196 bug was: `barriers_per_sp = barriers__unified` = **all** barriers +(incl dams, PSCIS, modelled) WHERE species ∈ `blocks_species` (§2a). Two wrongs, +both now fixed: + +1. **Wrong content** (FIXED). It included dams/anthropogenic; bcfp's + `barriers_` is natural-only. Every PARS segment below a dam read + `has_barriers_bt_dnstr = TRUE` → `accessible = FALSE` → token2 `;DAM` + suppressed → bare `SPAWN`. +2. **No override applied** (FIXED). bcfp removes barriers with upstream + observations / confirmed habitat. link's `lnk_barrier_overrides` + (`lnk_pipeline_prepare.R:519`) output `.barrier_overrides` fed only + `lnk_pipeline_classify` (habitat), NOT the access path. + +**The fix (#200/v0.40.4) — all three access inputs persisted province-wide:** + +- `barriers_per_sp` now points at `.barriers__access` + (`lnk_barriers_views`): feature-shaped (has-id, §2b), NATURAL only + (`barrier_source IN ('GRADIENT','FALLS','SUBSURFACE_FLOW','USER_DEFINITE')` — + gradient-at-species-threshold is already encoded in `blocks_species`), MINUS + the override (anti-join `barrier_overrides`), with `USER_DEFINITE` + override-exempt. Dams stay in `barrier_sources` → token2 only. +- `user_barriers_definite` is now a `USER_DEFINITE` family in `lnk_barriers_unify` + (persist `barriers`), ltree-resolved via the same FWA join the FALLS branch + uses (mirrors bcfp `barriers_user_definite.sql`). +- `barrier_overrides` is now persisted province-wide + (`.barrier_overrides`, `lnk_persist_init` + `lnk_pipeline_persist`). + Because the access view reads persist (cross-WSG) barriers, the override must + also be province-wide so a natural barrier in *any* WSG a downstream walk + crosses is lifted correctly. Persist PK is + `(blue_line_key, downstream_route_measure, species_code, watershed_group_code)` + — boundary-stream override positions are computed by two adjacent WSG runs, so + WSG must be in the key. + +**Provincial-accumulation property** (do not forget): a single-WSG run only sees +WSGs already in persist. PARS only emits `;DAM` once PCEA+UPCE (which hold the +Bennett/Peace Canyon dams PARS drains through) are persisted. This is identical +to bcfp's accumulated `barriers_` and to link's natural-barrier persistence +(link#152) — handled by the provincial orchestrator. + +**Validated (v0.40.4):** PARS BT 98.95%, LFRA BT 97.77% / CO 97.90% per-segment +vs `fresh.streams_vw_bcfp`. Residual ~1-2% is token1 habitat-presence +(`ACCESS`↔`SPAWN`/`REAR`, dimensions/rules), not the dam-access fix. + +What does **NOT** work (rejected during #196): `barriers__min` (break-spec, +no id, §2b) — its *content* (gradient+falls) is close, but it cannot feed +`frs_network_features`. ### Does dam blocking "depend on species"? (recurring question) @@ -361,7 +384,7 @@ direction. Not yet scoped; candidate issue. | Rule | File | Drives | |---|---|---| | Per-species gradient access threshold | `configs//parameters_fresh.csv` → `access_gradient_max` | gradient `blocks_species` (§2a) | -| Per-species observation override | `parameters_fresh.csv` → `observation_*` | barrier-skip via observations | +| Per-species observation override | `parameters_fresh.csv` → `observation_*` | barrier-skip via observations; feeds habitat (`lnk_pipeline_classify`) AND access (anti-join in `barriers__access`, persisted as `barrier_overrides`, #200) | | Habitat dimensions (spawn/rear by gradient, channel width, lake/stream, …) | `configs//dimensions.csv` → `lnk_rules_build()` → `rules.yaml` | `frs_habitat_classify()` (token1 habitat) | | Species residence (resident/anadromous/spawn-only) | **hardcoded** defaults in `lnk_pipeline_mapping_code()` | which mc_barrier flavor + spawn-only token1 | | Dam / anthropogenic blocking | **nowhere** — universal `all species` in `lnk_barriers_unify` | `blocks_species` (§2a). Not rules-driven. | diff --git a/man/lnk_barriers_unify.Rd b/man/lnk_barriers_unify.Rd index a99048c2..fc22ea5a 100644 --- a/man/lnk_barriers_unify.Rd +++ b/man/lnk_barriers_unify.Rd @@ -72,6 +72,13 @@ blocks all 8 species. \item \strong{Subsurface_flow} (\code{barrier_source = 'SUBSURFACE_FLOW'}, from \verb{.barriers_subsurfaceflow}): blocks all 8 species. Opt-in (only built when \code{cfg$pipeline$break_order} includes \code{"subsurfaceflow"}). +\item \strong{User_definite} (\code{barrier_source = 'USER_DEFINITE'}, from +\verb{.barriers_definite}): blocks all 8 species. ltrees + +\code{linear_feature_id} resolved by JOIN to +\code{whse_basemapping.fwa_stream_networks_sp} (mirrors the FALLS branch +and bcfp's \code{barriers_user_definite.sql}). Persisted province-wide so +the per-species access view (\verb{barriers__access}, +\code{\link[=lnk_barriers_views]{lnk_barriers_views()}}) can include them override-exempt — link#200. } Remediations (PASSABLE remediation crossings) are intentionally NOT @@ -90,6 +97,8 @@ Required pre-existing tables in \code{schema}: \item \verb{.crossings} (from \code{\link[=lnk_pipeline_crossings]{lnk_pipeline_crossings()}}). \item \verb{.gradient_barriers_raw} (from \code{\link[=lnk_pipeline_prepare]{lnk_pipeline_prepare()}}). \item \verb{.falls} (from \code{\link[=lnk_pipeline_prepare]{lnk_pipeline_prepare()}}). +\item \verb{.barriers_definite} (from \code{\link[=lnk_pipeline_prepare]{lnk_pipeline_prepare()}}; always +created, may be empty). } Optional: diff --git a/man/lnk_barriers_views.Rd b/man/lnk_barriers_views.Rd index 89b10afb..e2aab97e 100644 --- a/man/lnk_barriers_views.Rd +++ b/man/lnk_barriers_views.Rd @@ -34,8 +34,9 @@ either way (the underlying table has \code{blocks_species} from \code{\link[=lnk_barriers_unify]{lnk_barriers_unify()}}).} } \value{ -\code{invisible(conn)}. Side effect: drops + recreates one view -per species + three source-typed views in \code{schema}. +\code{invisible(conn)}. Side effect: drops + recreates two views +per species (\verb{_unified} + \verb{_access}) + three source-typed views in +\code{schema}. } \description{ Emits per-species + per-source views in the working schema that @@ -52,13 +53,25 @@ FWA topology and reads from the view (which is the unified table). Fixes the PARS BT 60\% defect (PARS drains through dams in PCEA / UPCE WSGs) and unblocks any regional run. -Per-species views: +Per-species views (two per species \code{bt}, \code{ch}, \code{cm}, \code{co}, \code{pk}, \code{sk}, +\code{st}, \code{wct}): \itemize{ -\item \verb{.barriers__unified} for each species (\code{bt}, \code{ch}, \code{cm}, -\code{co}, \code{pk}, \code{sk}, \code{st}, \code{wct}) — filtered by -\code{'' = ANY(blocks_species)}. \verb{_unified} suffix avoids name -collision with the per-WSG \verb{.barriers_} tables that -\code{.lnk_pipeline_prep_minimal} builds for the break-time path. +\item \verb{.barriers__unified} — filtered by +\code{'' = ANY(blocks_species)} (ALL barrier families incl dams). +\verb{_unified} suffix avoids name collision with the per-WSG +\verb{.barriers_} tables that \code{.lnk_pipeline_prep_minimal} +builds for the break-time path. +\item \verb{.barriers__access} — the per-species \strong{accessibility} +set that drives \code{accessible} in mapping_code (link#200). Reproduces +bcfp \verb{barriers_}: NATURAL barriers only +(\verb{barrier_source IN ('GRADIENT','FALLS','SUBSURFACE_FLOW')} — the +gradient-at-species-threshold is already encoded in \code{blocks_species}) +MINUS the observation/habitat override (anti-join \code{barrier_overrides}), +∪ all \code{USER_DEFINITE} (override-exempt). Dams/PSCIS/modelled are +excluded — they annotate token2 via the per-source views, never block +access. Same feature shape (\verb{barriers__access_id} + ltrees + geom) +so \code{\link[=lnk_pipeline_access]{lnk_pipeline_access()}} / \code{\link[fresh:frs_network_features]{fresh::frs_network_features()}} consume it +unchanged. See \code{RUNBOOK.md} §5. } Per-source views (matching the bcfp source-typed tables consumed by diff --git a/planning/active/findings.md b/planning/active/findings.md index 160bc198..15496d9e 100644 --- a/planning/active/findings.md +++ b/planning/active/findings.md @@ -1,119 +1,84 @@ -# Findings — #196 streams_access per-source flags + cross-WSG mapping_code +# Findings — mapping_code accessibility, reproduce bcfp `barriers_` (#200) -## The bug chain (2026-05-19) +## Issue context -After v0.40.0–v0.40.2 shipped the mapping_code decouple, attempting PARS-vs-bcfp parity revealed link's `mapping_code_bt` second token was `NONE` everywhere, where bcfp had `DAM`/`MODELLED`/`ASSESSED`. Three root causes, found in sequence: +link's per-species mapping_code accessibility (`barriers_per_sp` → `accessible`) uses `barriers__unified` = ALL barriers (incl dams/PSCIS/modelled) where the species is in `blocks_species`. bcfp's per-species access set is natural barriers only (gradient@species-threshold ∪ falls ∪ subsurface), MINUS upstream observation/habitat overrides, ∪ all `user_barriers_definite`. Dams are never in the access set — token2 descriptor only. Consequence: every segment below a dam reads inaccessible → `;DAM`/`;MODELLED`/`;ASSESSED` second token suppressed (token2 correctly gated on `accessible`) → link emits bare `SPAWN`/`REAR` where bcfp emits `SPAWN;DAM`. -### Cause 1 — persist DDL missing per-source flag columns (Phase 1) +## bcfp mechanism (read authoritatively from `smnorris/bcfishpass@e12c1a5`, 2026-05-23) -`lnk_pipeline_mapping_code:174-194` probes the access tibble for `has_barriers_{anthropogenic,pscis,dams,remediations}_dnstr`, `dam_dnstr_ind`, `remediated_dnstr_ind` to classify the second token. #187 Phase 2 dropped these from `cols_streams_access_base` (wrong reasoning: "conditional on remediations/observations"; actually only on `barrier_sources` which pipeline_run always passes). Persist `streams_access` lacked the columns → `has()` probes FALSE → token `NONE`. +All 5 per-species access models (`model/01_access/sql/model_access_{bt,ch_cm_co_pk_sk,wct,ct_dv_rb,st}.sql`) share one structure: +``` +barriers_ = ( gradient@species-classes ∪ falls ∪ subsurface ) + MINUS (barriers with upstream observation OR confirmed habitat, in the species' obs-set) + ∪ ALL barriers_user_definite -- override-EXEMPT (comment: "include *all* user added features, even those below observations") +``` +Per-species axes (the only differences across models): -### Cause 2 — views over working not persist (Phase 2) +| Model | Gradient classes | Obs/habitat species | +|---|---|---| +| BT | 25, 30 | BT,CH,CM,CO,PK,SK,ST | +| salmon (CH,CM,CO,PK,SK) | 15, 20, 25, 30 | CH,CM,CO,PK,SK | +| ST | 20, 25, 30 | CH,CM,CO,PK,SK,ST | +| WCT | 20, 25, 30 | (wct) | +| CT,DV,RB | 25, 30 | BT,DV,CT,RB | -#187 Phase 4 set `lnk_barriers_views(barriers_table = paste0(schema, ".barriers"))` (working schema, per-WSG local). Lost cross-WSG dam visibility (PARS BT needs Bennett/Peace Canyon dams in PCEA/UPCE — the link#152 case). PARS has 0 local dams; the cross-WSG dams live only in persist. Fix: pre-persist barriers, let views default to persist (province-wide). +- **`barriers_user_definite.sql`** materializes the definite table with a synthesized deterministic id + ltree resolved by joining the raw user CSV to `whse_basemapping.fwa_stream_networks_sp` (segment whose `[downstream_route_measure, upstream_route_measure)` contains the barrier). link's existing FALLS branch in `lnk_barriers_unify.R:221-242` does the **identical** join. +- **`load_streams_access.sql`** — `barriers__dnstr` (per-species, natural+definite) is separate from `barriers_anthropogenic_dnstr`/`barriers_dams_dnstr` (descriptors). `access_` = 0 if a downstream barrier exists, else 1/2 (obs-aware). token2 gate (`load_streams_mapping_code.sql`) = `barriers__dnstr = array[]` — identical to link's `ifelse(accessible, mc_barrier, NA)`. +- **Province-wide accumulation**: each `barriers_` is per-WSG-built but accumulated into one province-wide table, so cross-WSG downstream walks (PARS→PCEA→UPCE) see the correct override-applied set. -### Cause 3 — persist INSERT projection missing the flags (Phase 3 — THE actual NONE bug) +## link mapping (every ingredient already exists) -Even after Phase 1 added the DDL columns and Phase 2 fixed view source, persist `streams_access` flags came back ALL FALSE. Isolated repro against a surviving `working_pars_dbg` schema pinned it: +| bcfp ingredient | link object | state | +|---|---|---| +| gradient@species-threshold | `access_gradient_max` → `blocks_species` (gradient CASE in `lnk_barriers_unify`) | ✓ correct | +| obs/habitat override | `lnk_barrier_overrides` → `.barrier_overrides` (uses `fwa_upstream`, topological/cross-WSG) | ✓ computed; **per-WSG only, not persisted** | +| user_definite | `.barriers_definite` (`lnk_pipeline_prepare.R:182-200`; CSV cols, no id/ltree; empty-fallback = blk+drm only) | **per-WSG only, not in persist barriers** | +| natural barriers | persist `barriers` (gradient/falls/subsurface families) | ✓ province-wide | -- in-memory access tibble: flags populated (anth 48559, dams 48559, dam_ind 32406) ✓ -- working `streams_access` (dbWriteTable): flags populated ✓ -- persist `streams_access` (lnk_pipeline_persist INSERT): flags ALL FALSE ✗ +`lnk_barrier_overrides` output is `(blue_line_key, downstream_route_measure, species_code)` and currently feeds only `lnk_pipeline_classify` (habitat), NOT the access path. -`lnk_pipeline_persist:122-123` built `access_cols_v <- c(cols_streams_access_base, .lnk_cols_streams_access_per_sp(species))` — omitting `.lnk_cols_streams_access_source_flags()`. DDL had the columns; INSERT projected only base + per-species. The 6 flag columns got their default (NULL) on every INSERT. +## The design decision (why province-wide, not a per-WSG view) -**Lesson**: DDL change (lnk_persist_init) and INSERT-projection change (lnk_pipeline_persist) are a matched pair. The Phase 1 code-check asserted "INSERT picks up new columns automatically (iterates names(access_cols_v))" — false, because `access_cols_v` is independently constructed and didn't include the flags. Verify DDL + INSERT together against live data, not by reading one side. +The access set is a downstream `frs_network_features` walk that **crosses WSG boundaries**. A per-WSG `_access` view (subtract only the current WSG's overrides from province-wide natural barriers) is quietly wrong for any natural barrier in a downstream/sibling WSG — the cross-WSG twin of the dam bug. Rejected. Correct design: **persist all three access inputs province-wide** (natural ✓, override → new persist table, user_definite → `USER_DEFINITE` persist family), persisted **together per WSG** so any persisted WSG is internally consistent. Caveat (single-WSG run sees only persisted WSGs) is identical to today's natural barriers and bcfp's accumulation — handled by the provincial orchestrator. -## Isolation-debug technique that worked +Approach A (definite as unify family) + persist overrides was chosen over the issue-draft's per-WSG view-union (B') after a Plan-agent review and the user's explicit "make it provincial, don't ship a 2/200 one-off." `barriers_definite` lacks id+ltree → resolved via the FALLS-pattern FWA join. No `cols_barriers` DDL change (USER_DEFINITE is a new row-source, same columns). `barrier_overrides` persist uses a single `cols_barrier_overrides` vector for DDL+INSERT (avoids the v0.40.3 matched-pair drift). -The killed run's `working_pars_dbg` schema survived the docker restart (data volume persists across container restart). It had all prerequisite barrier tables (`barriers_anthropogenic`, `barriers_pscis`, `barriers_bt`, etc.) but no `_unified` views or `streams_access` (died before mapping_code phase). Reproduced the mapping_code phase steps (lnk_barriers_views → lnk_pipeline_access → inspect) in ~30s instead of re-running the 16-min pipeline. Much faster bug isolation. +## Verified facts (this session) -CAUTION: running `lnk_pipeline_persist` against the incomplete `working_pars_dbg` DELETE-WHERE-WSG'd the good PARS habitat (streams_habitat_bt 48559 → 0) and replaced with the half-built working schema's (empty) data. Debug-induced data damage — needs a clean full run to repair. Don't persist from a half-built working schema. +- Persist pattern is `cols_*`-vector-driven; `cols_barriers` drives both DDL (`lnk_persist_init`) and INSERT (`lnk_pipeline_persist.R:94,99`). +- break (`lnk_pipeline_break.R:112`) + classify (`lnk_pipeline_classify.R:223`) read `.barriers_definite` separately → adding `USER_DEFINITE` to persist `barriers` does NOT double-count. +- `barriers_anthropogenic/dams/pscis_unified` filter by `barrier_source` → `USER_DEFINITE` doesn't pollute them. `lnk_compare_*` don't read `_unified`. +- `frs_network_features` (fresh) needs `feature_id_col, blue_line_key, downstream_route_measure, wscode_ltree, localcode_ltree` on the feature table. The `phase4d_plan_draft.md:37` draft wrongly dropped the ltree cols. +- `whse_basemapping.fwa_stream_networks_sp` present in local fwapg; `fresh.streams_vw_bcfp` loaded (4.23M rows, PARS 43,660, carries `mapping_code_bt`) — tunnel-free parity baseline. bcfp baseline `v0.7.15-14-ge12c1a5`. -## Verified facts +## Phase 4 validation — PARS BT (2026-05-23) -- persist.barriers had PARS (68254 rows, 33104 BT-blockers) — data was upstream-correct throughout; bug was purely in the persist-write projection. -- `barriers_dams_unified` (view over persist) = 178 dams province-wide; PARS-local `barriers_dams` = 0. Confirms cross-WSG dams only visible via persist. -- bcfp `streams_mapping_code` has no `watershed_group_code`; JOIN `bcfishpass.streams ON segmented_stream_id`. +Run `lnk_pipeline_run("PARS", mapping_code=TRUE)` against local docker fwapg (bcfp baseline `v0.7.15-14-ge12c1a5` in `fresh.streams_vw_bcfp`). -## Cause 4 — METHODOLOGY, not a bug (the deepest finding) +**Result: 99.04% per-segment match vs bcfp** (42,701 / 43,114 joined on `blue_line_key` + rounded `downstream_route_measure`). The headline #200 fix works: +- token1 collapse GONE — `ACCESS`/`SPAWN`/`REAR` emit (was bare `SPAWN`/`REAR`). Counts ≈ bcfp (`SPAWN;DAM` 5293 vs 5263, `REAR;DAM` 2213 vs 2191). +- token2 `;DAM` emerges — dam-downstream-but-accessible segments now annotate `;DAM` not `;NONE`. -After all 3 persist fixes, streams_access flags persist correctly (dams 48561, dam_ind 32370 for PARS) but mapping_code STILL diverges from bcfp. Root: `access_bt = 0` (BLOCKED) for ALL 48561 PARS segments, because every PARS segment has a downstream dam (Bennett/Peace Canyon in PCEA — PARS drains through them). link#152 cross-WSG barriers correctly flag this. +**Cross-WSG dependency confirmed (the provincial design's whole point):** the FIRST PARS run (PARS only) emitted `;NONE` because the Bennett/Peace Canyon dams live in PCEA/UPCE, which weren't persisted. After persisting PCEA + UPCE barriers (`mapping_code=FALSE`) and re-running PARS, the cross-WSG downstream walk saw the dams → `;DAM`. token2/`barrier_sources` is unchanged by #200; it just needs the downstream WSGs in persist. -`lnk_pipeline_mapping_code:236` `accessible <- !has_barriers_sp & !no_data`. With every segment blocked: -- token1 (`:262-268`): `ACCESS` requires `accessible` → never fires; only SPAWN/REAR (habitat-driven) or NA -- token2 (`:276`): `ifelse(accessible, mc_barrier, NA)` → always NA → no `;DAM` -- token3 (`:277`): same → no `;INTERMITTENT` +Residual ~1% (413 segs): token1 `ACCESS`↔`REAR` swaps (habitat-presence threshold — dimensions/rules, not #200) + a few token2 `DAM`↔`MODELLED` next-downstream-ordering edges. Not the dam-access divergence. -Result: link emits `SPAWN` / `REAR` / `` (blank). bcfp emits `SPAWN;DAM` / `ACCESS;DAM` / `REAR;DAM` for the SAME segments — bcfp does NOT gate token1/token2 on downstream-dam accessibility. +### Phase 4 validation — LFRA (anadromous; Coquitlam/Alouette/Stave/Ruskin dams) -This is the v0.40.0 methodology shift at the semantic level: pre-#187 access used bcfp-staged barriers → matched bcfp (98.63%). Post-#187 access uses link's own cross-WSG-aware barriers → all PARS segments read blocked → tokens collapse. +`lnk_pipeline_run("LFRA", mapping_code=TRUE)`. Match vs bcfp: **LFRA/bt 97.77%, LFRA/co 97.90%** (26,651 segs each). LFRA coho DAM-token count link **4672 vs bcfp 4636** — the dam descriptor + above-dam path works for anadromous salmon, not just resident BT. LFRA drains to the ocean (lowest Fraser group) so its dams are in-WSG — single run, no cross-WSG persist needed (unlike PARS→PCEA/UPCE). -**RESOLVED — was NOT a methodology decision, was the wrong barriers table.** +Residual ~2%: token1 `ACCESS`↔`SPAWN`/`REAR` (spawning/rearing **habitat-presence** — token1 habitat fires regardless of access per RUNBOOK §4; governed by `frs_habitat_classify`/dimensions, unaffected by #200) + small token2 `DAM`↔`MODELLED`/`NONE` next-downstream-ordering edges. The dam-access fix itself (token2 DAM on accessible dam-downstream segments) matches. -Grounded in the pre-#187 matching code (`git show 0cd48d0:R/lnk_compare_wsg.R`) + the research doc canonical call sequence + reading `.lnk_pipeline_prep_minimal:574-592`: +**Acceptance MET** for both resident + anadromous. Remaining habitat-token1 parity is a separate, pre-existing concern (habitat rules), not #200. -- `barriers__min` = gradient + falls ONLY (natural barriers, per-species, minimal-reduced). These are the correct `barriers_per_sp` for access. A segment downstream of a dam is still gradient-accessible → token1 = SPAWN/ACCESS. -- `barriers__unified` (what #187 Phase 4 wrongly used) = ALL barriers incl dams → every dam-downstream segment reads blocked → accessible FALSE → token1/2 collapse to bare SPAWN/REAR. -- Dams belong in `barrier_sources` (cross-WSG `barriers_dams_unified`) → drive token2 (`DAM`) + `dam_dnstr_ind`, NEVER blocking `accessible`. That's how bcfp emits `SPAWN;DAM`: gradient-accessible habitat + dam noted. -- link#152 cross-WSG fix is in `barrier_sources` (untouched). Natural barriers are inherently local (gradient/falls don't cross WSG) so `_min` stays tunnel-free. +### Stale-persist-table drift (pre-existing, surfaced in Phase 4) -**Fix applied** (uncommitted, on branch): `lnk_pipeline_run` Phase 4 `barriers_per_sp` swapped `_unified` → `_min`. Verification run in flight (PARS, cleanup_working=FALSE). Expected: token1 ACCESS/SPAWN/REAR restored, token2 DAM/MODELLED/ASSESSED present (`SPAWN;DAM` etc.), match_pct vs bcfp back toward ~98%. +LFRA first failed: `column "has_barriers_ch_dnstr" of relation "streams_access" does not exist`. The M1 `fresh.streams_access` / `streams_mapping_code` were stale at bt+co width (old pre-v0.40.2 runs); `lnk_persist_init`'s `CREATE IF NOT EXISTS` won't widen an existing table, and `.lnk_validate_persist_table` only detects GENERATED-column drift, not species-count drift. `lnk_pipeline_run` correctly sizes persist_init to `cfg$species` (8) — so DROPping the two stale wide tables + re-running recreated them full-width. NOT a #200 bug (production provincial runs start clean). Possible follow-up: extend drift-validate to species-column count. -The 3 persist commits (DDL, pre-persist, INSERT projection) remain correct + necessary (the source flags must persist for token2). The `_min` swap is the 4th + final piece. +**Real bug caught + fixed during the run:** `barrier_overrides` PK was `(blk, drm, species_code)` — UPCE's persist INSERT collided with PCEA's because the SAME override position is computed by two adjacent WSG runs (boundary streams whose `blue_line_key` spans WSGs). Fixed: PK now includes `watershed_group_code` (mirrors `cols_barriers`), so per-WSG DELETE+INSERT is clean; the WSG-agnostic access anti-join makes the duplicate harmless. -**Tunnel-free confirmed**: build path uses `_min` (local gradient/falls) + `barrier_sources` views over persist (local). NO tunnel needed for the build. Tunnel only for the parity DIFF (validation). Aligns with user direction: "we will go tunnel free eventually, cannot depend on it for testing." +## Open / watch -### Cause 4 fix attempt #1 — `_min` swap FAILED (mechanical, 2026-05-23) - -The `_min` swap broke fast: `ERROR: column b.barriers_bt_min_id does not exist`. Root: `lnk_pipeline_access:152-161` derives `feature_id_col <- paste0(table_only, "_id")` and passes the table to `fresh::frs_network_features`, which needs a surrogate feature id. The `_min` tables are **break-position specs** (`blue_line_key, downstream_route_measure, wscode_ltree, localcode_ltree` ONLY — no id) built for `frs_break_apply`, NOT feature tables. The `_unified` views ARE feature-shaped (`id_barrier AS barriers__unified_id`, geom, blocks_species). So `barriers_per_sp` mechanically REQUIRES `frs_network_features`-shaped tables; `_min` cannot be dropped in. - -**What `barriers__unified` actually is** (live viewdef): `SELECT id_barrier AS barriers_bt_unified_id, barrier_source, ..., blocks_species, ..., geom FROM .barriers WHERE 'BT' = ANY(blocks_species)`. It's all persist barriers (cross-WSG) where the species is in `blocks_species` (#152 predicate). The over-blocking question reduces to: **is DAM in `blocks_species` for BT?** If yes, every PARS dam-upstream segment reads blocked → Cause 4 token collapse. - -**Correct fix shape (not yet built):** a per-species feature view that keeps the `_unified` shape + id but filters OUT anthropogenic/dam sources — natural barriers only (`barrier_source NOT IN ('DAM', ...)` or `barrier_subtype`-based). That gives `frs_network_features` its id AND the gradient/falls-only access set. BUT this is gated on confirming bcfp's actual semantics — see below. - -### RESOLVED 2026-05-23 — bcfp source read; the access set is natural-only + override-filtered - -Read authoritatively from `smnorris/bcfishpass@v0.7.15` (`gh api`, read-only): -- `model/01_access/sql/model_access_bt.sql` — `bcfishpass.barriers_bt` = - `(gradient WHERE barrier_type IN ('GRADIENT_25','GRADIENT_30') ∪ falls ∪ subsurface)` - MINUS barriers with upstream BT/salmon/steelhead **observations** (`obs_upstr`, - 20 m tol) MINUS barriers with upstream confirmed **habitat** - (`user_habitat_classification`, `hab_upstr` 200 m tol) ∪ ALL - `barriers_user_definite`. **No dams / PSCIS / modelled crossings.** -- `model/01_access/sql/load_streams_access.sql` — `barriers__dnstr` (per-sp, - natural) is SEPARATE from `barriers_anthropogenic_dnstr` / `barriers_dams_dnstr`. - `dam_dnstr_ind = array[barriers_anthropogenic_dnstr[1]] && barriers_dams_dnstr`. -- `model/02_habitat_linear/sql/load_streams_mapping_code.sql` — token2 gated on - `barriers_bt_dnstr = array[]::text[]` (accessible) — identical to link. - -So `SPAWN;DAM` = natural-accessible (`barriers_bt_dnstr=[]`) + spawning>0 + dam -downstream. The dam annotates access; it doesn't block it. - -**link's two divergences (fully characterized):** -1. `barriers_per_sp = barriers__unified` = ALL barriers incl dams (wrong - content; bcfp = natural only). -2. The observation/habitat override (`lnk_barrier_overrides` → - `.barrier_overrides`) feeds `lnk_pipeline_classify` (habitat) only — - NOT `lnk_pipeline_access` (mapping_code accessibility). bcfp applies it to - `barriers_`. - -Note `access_` integer (lnk_pipeline_access.R:364) IS obs-aware (matches -bcfp `access_bt`) — but mapping_code uses `has_barriers__dnstr`, not it. - -**Fix shape:** per-species *feature* view (has-id) of natural-only barriers -(gradient@species-threshold + falls + subsurface) + override + user_definite = -reproduce bcfp `barriers_`. Real builder, not a swap. Full writeup in -`RUNBOOK.md` §5. Design implication: `blocks_species` binary conflates -natural-access + anthropogenic-descriptor — candidate redesign. - -### (superseded) The decisive question is empirical, not theoretical — redo snapshot - -Whether bcfp gates token1/token2 on dam-accessibility must be read from bcfp's ACTUAL output, not reasoned about. Tunnel is down again (stale, timed out). Per user direction ("rerun the snapshot locally") + ("cannot depend on tunnel for testing"): running `data-raw/snapshot_bcfp.sh --with-bcfp-views --force` against local docker fwapg (tunnel-free, public sources) to land this week's bcfp into `fresh.streams_bcfp`. Then row-level diff: for PARS segments above Bennett/Peace Canyon dams, what does bcfp's `mapping_code_bt` say (`SPAWN;DAM`? bare `SPAWN`? `NONE`?) AND is the segment in bcfp's accessible set? That answer dictates the fix shape. - -## Open - -- **Cause 4 methodology decision** — blocks PARS-vs-bcfp parity. Pending user. -- Wall time 956s (~16 min) for one PARS run — double-persist overhead. See task_plan Phase 5. -- BULK also needs a clean rebuild (damaged in earlier debug churn). -- access_bt=0 everywhere also means the rollup (spawn/rear km) may be affected — verify whether habitat km changed vs historic. +- Cross-WSG override correctness — the provincial design should fix it; **verify on LFRA** in Phase 4 (don't assume). +- `remediated_dnstr_ind` divergence is bcfp's own bug (`smnorris/bcfishpass#690`) — whitelist in the parity diff. +- Pre-existing (out of scope): `lnk_pipeline_run.R:228` `pscis = .barriers_pscis` vs view name `barriers_pscis_unified` — watch in Phase 4. diff --git a/planning/active/progress.md b/planning/active/progress.md index 9eb8e62f..14a6c9be 100644 --- a/planning/active/progress.md +++ b/planning/active/progress.md @@ -1,38 +1,14 @@ -# Progress — #196 streams_access per-source flags + cross-WSG mapping_code - -## Session 2026-05-19 - -- PWF scaffolded retroactively (started #196 without it; scope grew to 3 bugs). -- Phase 1 (`91f3f90`): persist DDL per-source flag cols via `.lnk_cols_streams_access_source_flags()`. -- Phase 2 (`e23819a`): pre-persist barriers so views default to persist (cross-WSG dam visibility). -- Phase 3 (`475e397`): `lnk_pipeline_persist` INSERT projection now includes the source flags — THE actual NONE-token bug. Verified in isolation: persist streams_access flags populate (anth 48559, dams 48559, dam_ind 32406). -- Branch `196-streams-access-source-flags`, 3 commits ahead of main. -- Phase 4 IN FLIGHT: clean PARS `lnk_pipeline_run(mapping_code=TRUE)` (task `btkkjqxlp`) — rebuilds habitat (damaged by debug shortcut persisting from half-built working_pars_dbg) + verifies end-to-end token output. -- Phase 4 result: streams_access flags NOW persist (dams 48561, dam_ind 32370). BUT tokens still wrong — found **Cause 4** (methodology, not bug): access_bt=0 (blocked) for ALL PARS segments because cross-WSG dams are downstream → `accessible=FALSE` → token1/2/3 collapse to SPAWN/REAR/blank. bcfp emits SPAWN;DAM regardless. See findings Cause 4. **Paused for user decision** on accessibility-gating semantics. -- 3 persist commits (91f3f90, e23819a, 475e397) are correct + shippable on their own (flags should persist). Token semantics is separate — likely new issue, possibly tied to #197 rules engine. -- Next: (a) user decides Cause-4 semantics; (b) decide Phase 5 wall-time; (c) rebuild BULK; (d) ship v0.40.3 (persist fixes) maybe independent of the token-semantics work. +# Progress — mapping_code accessibility, reproduce bcfp `barriers_` (#200) ## Session 2026-05-23 -- Resumed via `/loop continue phase 7`. Tested the Cause-4 `_min` swap (uncommitted edit to lnk_pipeline_run barriers_per_sp). -- `_min` swap **FAILED mechanically** (PARS run `bpzeq473w`): `barriers_bt_min_id does not exist`. `_min` tables are break-specs (no surrogate id); `frs_network_features` (via lnk_pipeline_access) requires a feature id. `_unified` views are the feature-shaped tables (id_barrier + geom + blocks_species). `barriers_per_sp` mechanically requires `_unified`-shape input. -- Architectural truth: `barriers__unified` = persist.barriers WHERE sp ∈ blocks_species. Over-blocking reduces to "is DAM in blocks_species for BT". Correct fix = per-species natural-only feature view (keeps id, drops dams) — BUT gated on confirming bcfp's real semantics. -- Tunnel down again (stale, timeout). Per user ("rerun the snapshot locally"): launched `snapshot_bcfp.sh --with-bcfp-views --force` against local docker fwapg (tunnel-free, public sources) → task `be94vol3c`. Lands this week's bcfp into `fresh.streams_bcfp` for the row-level dam-segment diff that decides the fix shape. -- The `_min` swap is still in the working tree (uncommitted) — to be reverted once snapshot confirms direction. -- Next: snapshot completes → inspect bcfp dam-upstream segment mapping_code → decide fix shape → surface to user. - -## Session 2026-05-23 (cont.) — mechanism resolved by reading bcfp source - -- User pushed hard: "dam blockage is species specific based on our rules", "fish are above these dams, dam means nothing on its own", "how does bcfp do it? understand mechanisms", "blocks_species maybe the wrong design — nothing black-white", "we have species overrides (fish above → override)", "do our homework and DOCUMENT so we can find it. tricky to carry all this knowledge". → wrote **RUNBOOK.md** (repo root) + CLAUDE.md pointer. -- Read bcfp source authoritatively (`gh api`, smnorris/bcfishpass@v0.7.15): - `model_access_bt.sql`, `load_streams_access.sql`, `load_streams_mapping_code.sql`. - **Finding:** bcfp `barriers_` = natural-only (gradient@species-threshold + falls + subsurface) MINUS upstream-observation/habitat overrides ∪ user_definite. Dams NEVER in the access set — they annotate (token2) via separate `barriers_dams_dnstr`. token2 gate identical to link. -- **link's bug, fully characterized:** (1) `barriers_per_sp = barriers__unified` carries dams (wrong content); (2) `lnk_barrier_overrides` output feeds classify (habitat) not access (mapping_code). Documented in RUNBOOK §5 + findings. -- Reverted broken `_min` swap → `_unified` (runnable, known-divergent), comment → RUNBOOK §5. -- Confirmed user's instincts on every point. The real fix = build a natural-only per-species feature view reproducing bcfp barriers_ + wire overrides into access. Real work (Phase 4d), needs scoping. -- streams_vw_bcfp load failed on transient gzip (snapshot otherwise OK exit 0); retrying (task b122hazyb) for empirical parity numbers. -- Next: user reviews the mechanism/runbook; scope Phase 4d; decide whether blocks_species redesign is its own issue. - -### Environment -- docker `fresh-db` up (was down mid-session; `open -a Docker` + `docker compose up -d db` in fresh/docker/). -- bcfp tunnel `localhost:63333` flaky (dropped 3×); needs PG_PASS_SHARE. Build is tunnel-free; only parity diff needs it. +- M4→M1 handoff resumed; v0.40.3 shipped (PR #199 merged `46b2042`, tagged). +- Environment up on M1: docker fresh-db, link 0.40.3, bcfp snapshot reloaded (`v0.7.15-14-ge12c1a5`, `streams_vw_bcfp` loaded locally), local `:63333→:5432` forwarder (db_newgraph tunnel key deauthorized on M1 — not blocking). +- Read bcfp access machinery end-to-end (5 species models + `barriers_user_definite.sql` + `load_streams_access.sql`) — see findings. +- Plan-agent design review: rejected per-WSG view (B'); user pushed for province-wide correctness. Final design = persist all access inputs province-wide (USER_DEFINITE family + persist barrier_overrides + `_access` view). Plan approved. +- Archived #196 PWF; created branch `200-mapping-code-accessibility-reproduce-bcf`; scaffolded #200 PWF baseline. +- Phase 1 done: `USER_DEFINITE` family in `lnk_barriers_unify` (FALLS-pattern FWA ltree join); `cols_barrier_overrides` + DDL in `lnk_persist_init`; persist copy in `lnk_pipeline_persist` (pre-persist auto-picks it up). Unit tests 96 pass; DB-smoke validated DDL + branch (empty-fallback safe, one-row resolves ltree/geom). Both configs persist to schema `fresh` (provincial `fresh_default` is a runtime `--schema` override). Commit `a82a7fc`. +- Phase 2 done: `barriers__access` view in `lnk_barriers_views` (natural-only + override anti-join + definite-exempt). DB-smoke: 904,262 natural rows, 0 non-natural. code-check clean. Commit `f758e44`. +- Phase 3 done: repoint `barriers_per_sp` → `_access` + comment. Commit `c412d68`. +- Phase 4 done: validated PARS BT 98.95%, LFRA BT 97.77% / CO 97.90% vs `streams_vw_bcfp`; `;DAM` tokens correct. Found+fixed `barrier_overrides` PK (added `watershed_group_code` — boundary-stream cross-WSG collision). Worked around stale bt+co persist tables (DROP + recreate full-width). PARS needed PCEA+UPCE persisted first (cross-WSG dams). Commit `2ea3a7d`. +- Phase 5: RUNBOOK §2a/§3/§5/§7 updated; NEWS + DESCRIPTION → 0.40.4; temp scripts removed. test 1193 pass (lone FAIL = env db_conn). Next: archive + open PR (stop before merge per user). diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index f2e7a3fb..3de484c4 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -1,86 +1,50 @@ -# Task: streams_access per-source flag persistence + cross-WSG mapping_code (#196) +# Task: mapping_code accessibility — reproduce bcfp `barriers_` (natural-only + override), provincially consistent (#200) -Retroactive PWF — #196 was started as a "quick single-file fix" without PWF, then ballooned into a 3-bug investigation. Scaffolded mid-flight 2026-05-19 to capture the trace. +link's per-species mapping_code accessibility uses `barriers__unified` = ALL barriers (incl dams) where the species ∈ `blocks_species`, so dam-downstream segments read inaccessible and lose their `;DAM` token2. bcfp's access set is natural-only (gradient@species-threshold ∪ falls ∪ subsurface) MINUS observation/habitat override ∪ all user_definite — dams annotate (token2), never block. Fix: make all access inputs province-wide-persisted (natural ✓ already, override + user_definite added), build a `barriers__access` view over them, repoint `barriers_per_sp`. Full design: `planning/active/findings.md` + `RUNBOOK.md` §5. -## Problem +## Phase 1 — make the override + user_definite province-wide -Post-#187 `lnk_mapping_code` second token defaulted to `NONE` for every segment (should be `DAM`/`MODELLED`/`ASSESSED`/`REMEDIATED`/`NONE`). PARS BT match_pct vs bcfp dropped from historic ~98% to ~30-50%. Three coupled root causes, fixed in three commits. +- [x] `USER_DEFINITE` family in `lnk_barriers_unify` (mirror FALLS branch: FWA-join for ltree; source `.barriers_definite`; `blocks_species`=all; reference only `blue_line_key`+`downstream_route_measure` for empty-fallback safety). No persist DDL change. +- [x] `cols_barrier_overrides` vector + `CREATE TABLE IF NOT EXISTS .barrier_overrides` in `lnk_persist_init` (one vector drives DDL + INSERT). +- [x] Persist `barrier_overrides` (DELETE-WHERE-WSG + INSERT, add `''` as `watershed_group_code`) in `lnk_pipeline_persist`; probe-gated. +- [x] Pre-persist auto-handled: the mapping_code-phase pre-persist (`lnk_pipeline_run.R:188`) already calls the full `lnk_pipeline_persist`, which now persists `barrier_overrides` — no separate edit needed. +- [x] Tests: unify `USER_DEFINITE` branch SQL; persist_init `barrier_overrides` DDL; persist INSERT projection. (96 pass) +- [x] DB-smoke: `barrier_overrides` DDL creates in `fresh`; USER_DEFINITE branch parses + resolves ltree/geom (empty-fallback safe + one-row). +- [x] `/code-check` (round 1 clean) + commit. -## Phase 1 — persist DDL: per-source flag columns +## Phase 2 — `barriers__access` view over province-wide inputs -- [x] `.lnk_cols_streams_access_source_flags()` helper in `lnk_persist_init.R` — 6 boolean cols (`has_barriers_{anthropogenic,pscis,dams,remediations}_dnstr`, `dam_dnstr_ind`, `remediated_dnstr_ind`). -- [x] Wired into `streams_access` CREATE TABLE. -- [x] `/code-check` clean. -- [x] Commit `91f3f90`. +- [x] Per-species `_access` view in `lnk_barriers_views`: natural (`barrier_source IN ('GRADIENT','FALLS','SUBSURFACE_FLOW','USER_DEFINITE')`) over persist `barriers`, `NOT EXISTS` anti-join over `barrier_overrides` (derived from the barriers source schema via `sub()`), `USER_DEFINITE` override-exempt, expose `barriers__access_id`, keep `wscode_ltree`/`localcode_ltree`, alias `b`. +- [x] Tests: `_access` view SQL per species; counts updated (22→38, 10→14). 30 pass. +- [x] DB-smoke: `barriers_bt_access` valid + queryable; 904,262 natural rows, 0 non-natural (dams/anthropogenic excluded) vs `_unified` 1,045,358. +- [x] `/code-check` (clean) + commit. -## Phase 2 — cross-WSG visibility: pre-persist barriers +## Phase 3 — repoint `barriers_per_sp` → `_access` -- [x] `lnk_pipeline_run` mapping_code phase: pre-persist current WSG's barriers BEFORE `lnk_barriers_views`, so views default to persist (province-wide, cross-WSG dam visibility per link#152). Reverted #187 Phase 4's `barriers_table = working` override. -- [x] `/code-check` clean. -- [x] Commit `e23819a`. +- [x] `lnk_pipeline_run` (:215-217) `..._unified` → `..._access`; rewrote KNOWN-DIVERGENT comment (:200-213) to describe the landed fix. +- [x] `test-lnk_pipeline_run.R` asserts no `_unified`/`barriers_per_sp` name → no update needed. +- [x] Full suite 1193 pass (the lone FAIL is the env-only `db_conn` test — needs the real db_newgraph tunnel: `.Renviron` `PG_*_SHARE` → `:63333` w/ airvine/bcfishpass creds; CI skips it via `skip_if_no_db`). Phase 3 repoint validated by Phase 2's code-check (consumption confirmed). +- [x] commit. -## Phase 3 — persist INSERT projection (the actual NONE bug) +## Phase 4 — DB validation (hard gate): PARS + LFRA vs `fresh.streams_vw_bcfp` -- [x] `lnk_pipeline_persist`: `access_cols_v` was `base + per_sp` only — MISSING `.lnk_cols_streams_access_source_flags()`. DDL had the cols (Phase 1) but INSERT never populated them → NULL → NONE token. Added the generator to the projection. -- [x] Verified in isolation: persist `streams_access` flags now populate (anth 48559, dams 48559, dam_ind 32406 of 48559 for PARS). -- [x] `/code-check` — SKIPPED on this commit (verified empirically instead; the earlier code-check that missed this gap was the cautionary tale). -- [x] Commit `475e397`. +- [x] Rebuild PARS (BT) `mapping_code=TRUE`; `;DAM` tokens now emit (`SPAWN;DAM` 5293≈5263 bcfp). **98.95% match.** Needed PCEA+UPCE barriers persisted first (cross-WSG dams) — confirms the provincial design. +- [x] Rebuild LFRA (anadromous; Coquitlam/Alouette/Stave/Ruskin). **bt 97.77%, co 97.90%**; coho DAM tokens 4672≈4636 bcfp. Dams in-WSG (drains to ocean), single run. +- [x] Found + fixed real bug: `barrier_overrides` PK needed `watershed_group_code` (boundary-stream overrides shared across adjacent WSGs collided). Found + worked around stale-persist-table drift (bt+co wide tables → DROP + recreate full-width). +- [x] Residual ~1-2% characterized: token1 habitat-presence (`ACCESS`↔`SPAWN`/`REAR`, dimensions/rules) + minor token2 ordering — NOT the dam-access divergence. Recorded in `findings.md`. -## Phase 4 — end-to-end verification (IN FLIGHT) +## Phase 5 — docs + release -- [ ] Clean `lnk_pipeline_run(aoi="PARS", mapping_code=TRUE)` — rebuilds habitat (damaged by debug shortcut) + access + mapping_code. (task `btkkjqxlp`, ~16 min) -- [ ] Verify PARS BT `mapping_code_bt` distribution includes `ACCESS;DAM`/`ACCESS;MODELLED`/`ACCESS;ASSESSED` (not just `ACCESS;NONE`). -- [ ] Tunnel up → `lnk_compare_wsg(aoi="PARS", mapping_code=TRUE)` → match_pct vs bcfp ≥ ~95%. -- [ ] Re-run BULK too (also damaged / needs rebuild). - -## Phase 4b — Cause 4 fix: barriers_per_sp `_unified` → `_min` (NEW) - -- [x] Root-caused: `_unified` (all barriers) over-blocks; `_min` (gradient+falls natural barriers) is correct per-species access set. Dams stay in `barrier_sources` (token2 only). Grounded in pre-#187 matching code + prep_minimal source. -- [x] Swap applied in `lnk_pipeline_run` Phase 4 (uncommitted). -- [x] Verify: PARS run (task `bpzeq473w`) → **FAILED mechanically**. `barriers_bt_min_id does not exist` — `_min` tables are break-specs (no id col); `frs_network_features` needs a feature id. `_min` cannot feed `barriers_per_sp`. See findings. -- [x] Architectural finding: `barriers__unified` = persist barriers WHERE species ∈ blocks_species (#152), feature-shaped w/ id. Correct fix = per-species feature view, `_unified` shape, natural-only filter — but gated on bcfp semantics. -- [ ] **REVERT the `_min` swap** in lnk_pipeline_run (restore `_unified`) — known state for the data investigation. (deferred until snapshot confirms direction) - -## Phase 4c — empirical bcfp semantics (BLOCKING — redo snapshot) - -- [x] Tunnel down (stale). Snapshot script is tunnel-free (public sources). Run `snapshot_bcfp.sh --with-bcfp-views --force` → local `fresh.streams_bcfp` (task `be94vol3c`). -- [ ] Inspect `fresh.streams_bcfp` columns — does it carry `mapping_code_bt` + access cols per segment? -- [ ] Row-level: PARS segments above Bennett/Peace Canyon dams — bcfp `mapping_code_bt` value? In bcfp's accessible set or not? -- [x] **RESOLVED by reading bcfp source** (not data — authoritative). Fix shape = (a): per-species access set is natural-only + override-filtered; dams never block access. See findings (RESOLVED 2026-05-23) + RUNBOOK.md §5. -- [x] Reverted broken `_min` swap → `_unified` (runnable, known-divergent) w/ comment → RUNBOOK §5. -- [x] Documented authoritatively in RUNBOOK.md (§2a, §5) + CLAUDE.md pointer + findings. (user ask: "do our homework and document so we can find it") -- [ ] streams_vw_bcfp retry (task b122hazyb) for empirical parity numbers (snapshot's streams view failed on transient gzip). - -## Phase 4d — the real fix (FILED as #200; #201 = redesign follow-up) - -- [x] Filed #200 (access-set reproduction) + #201 (blocks_species redesign + dam-override, depends on #200). -- [ ] Build per-species NATURAL-only *feature* view reproducing bcfp `barriers_`: - gradient@species-threshold + falls + subsurface, MINUS observation/habitat - overrides, ∪ user_barriers_definite. Has-id (feature shape, §2b). -- [ ] Wire `lnk_barrier_overrides` output into the access path (currently classify-only). -- [ ] Point `barriers_per_sp` at the new view; dams stay in barrier_sources (token2). -- [x] `blocks_species` redesign (carry ingredients, classify access late) + dam-override — filed as #201 (RUNBOOK §5 design implication). - -## Phase 5 — performance concern (NEW — decide before merge) - -- [ ] Wall time blew up to 956s (~16 min) vs normal ~3.5 min for PARS — the double-persist (pre-persist barriers + final persist) re-writes streams + habitat + barriers twice. Decide: - - (a) Accept it (correctness over speed; provincial run adds ~hours though). - - (b) Pre-persist ONLY barriers (not streams + habitat) — the views only need barriers. Smaller helper / `only=` arg on lnk_pipeline_persist. - - (c) Reorder so persist runs once, mapping_code phase after. - - Likely (b). File as part of #196 or follow-up. - -## Phase 6 — release v0.40.3 - -- [ ] DESCRIPTION 0.40.2 → 0.40.3, Date. -- [ ] NEWS.md entry. -- [ ] CLAUDE.md branch ref. -- [ ] `/planning-archive`. -- [ ] `/gh-pr-push` + `/gh-pr-merge`. +- [x] `RUNBOOK.md` §2a/§3/§5/§7 updated (fix landed; province-wide override/definite persistence; provincial-accumulation note). +- [x] `NEWS.md` + `DESCRIPTION` → 0.40.4. +- [x] Temp validation scripts removed. +- [ ] `/planning-archive`, `/gh-pr-push`. ## Validation -- [ ] PARS BT tokens include DAM/MODELLED/ASSESSED -- [ ] match_pct vs bcfp restored to ~98% -- [ ] `devtools::test()` passing -- [ ] wall-time decision made (Phase 5) -- [ ] `/code-check` clean on any remaining commits +- [x] `devtools::test()` 1193 pass (lone FAIL = env-only `db_conn`, needs real tunnel; CI skips). +- [~] `devtools::check()` — CI runs it green (no DB → db_conn skips); local can't be fully green without the db_newgraph tunnel. Noted in PR. +- [x] Phase 4 DB parity (hard gate): PARS BT 98.95%, LFRA BT 97.77% / CO 97.90%; `;DAM` correct. +- [x] `/code-check` clean on Phases 1-2; Phase 3 covered by Phase 2 review; Phase 4 PK fix reasoned + tested. +- [x] PWF checkboxes match landed work. +- [ ] `/planning-archive` on completion. diff --git a/planning/active/HANDOFF.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/HANDOFF.md similarity index 100% rename from planning/active/HANDOFF.md rename to planning/archive/2026-05-issue-196-streams-access-source-flags/HANDOFF.md diff --git a/planning/archive/2026-05-issue-196-streams-access-source-flags/README.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/README.md new file mode 100644 index 00000000..3a723219 --- /dev/null +++ b/planning/archive/2026-05-issue-196-streams-access-source-flags/README.md @@ -0,0 +1,9 @@ +# #196 — streams_access per-source flag persistence + cross-WSG mapping_code + +**Outcome:** Shipped as **v0.40.3** (PR #199, merged `46b2042`, tagged `v0.40.3`). Fixed the three coupled root causes of the `NONE`-everywhere second token in `lnk_pipeline_mapping_code`: persist DDL gained the six per-source flag columns (`lnk_persist_init`), the mapping_code phase pre-persists barriers for cross-WSG dam visibility (`lnk_pipeline_run`), and the persist INSERT projection now includes the flag columns (`lnk_pipeline_persist` — the actual `NONE` bug). Added `RUNBOOK.md` (the durable barrier→access→mapping_code mental model + authoritative bcfp access-set mechanism). + +**Spun out (not done here):** +- **#200** — mapping_code accessibility: reproduce bcfp `barriers_` (natural-only + override) so dam-downstream segments emit `;DAM`. The access set still carried dams (should be natural-only + observation/habitat-overridden). Now in flight on branch `200-mapping-code-accessibility-reproduce-bcf`. +- **#201** — `blocks_species` redesign (carry barrier ingredients, classify access late) + evidence-based dam-override. Depends on #200. + +The investigation trace (Causes 1–4, the `_min`-swap dead end, the bcfp source read on 2026-05-23) is in `findings.md`; the scoped fix that became #200 is in `phase4d_plan_draft.md`. `HANDOFF.md` was the M4→M1 handoff. diff --git a/planning/archive/2026-05-issue-196-streams-access-source-flags/findings.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/findings.md new file mode 100644 index 00000000..160bc198 --- /dev/null +++ b/planning/archive/2026-05-issue-196-streams-access-source-flags/findings.md @@ -0,0 +1,119 @@ +# Findings — #196 streams_access per-source flags + cross-WSG mapping_code + +## The bug chain (2026-05-19) + +After v0.40.0–v0.40.2 shipped the mapping_code decouple, attempting PARS-vs-bcfp parity revealed link's `mapping_code_bt` second token was `NONE` everywhere, where bcfp had `DAM`/`MODELLED`/`ASSESSED`. Three root causes, found in sequence: + +### Cause 1 — persist DDL missing per-source flag columns (Phase 1) + +`lnk_pipeline_mapping_code:174-194` probes the access tibble for `has_barriers_{anthropogenic,pscis,dams,remediations}_dnstr`, `dam_dnstr_ind`, `remediated_dnstr_ind` to classify the second token. #187 Phase 2 dropped these from `cols_streams_access_base` (wrong reasoning: "conditional on remediations/observations"; actually only on `barrier_sources` which pipeline_run always passes). Persist `streams_access` lacked the columns → `has()` probes FALSE → token `NONE`. + +### Cause 2 — views over working not persist (Phase 2) + +#187 Phase 4 set `lnk_barriers_views(barriers_table = paste0(schema, ".barriers"))` (working schema, per-WSG local). Lost cross-WSG dam visibility (PARS BT needs Bennett/Peace Canyon dams in PCEA/UPCE — the link#152 case). PARS has 0 local dams; the cross-WSG dams live only in persist. Fix: pre-persist barriers, let views default to persist (province-wide). + +### Cause 3 — persist INSERT projection missing the flags (Phase 3 — THE actual NONE bug) + +Even after Phase 1 added the DDL columns and Phase 2 fixed view source, persist `streams_access` flags came back ALL FALSE. Isolated repro against a surviving `working_pars_dbg` schema pinned it: + +- in-memory access tibble: flags populated (anth 48559, dams 48559, dam_ind 32406) ✓ +- working `streams_access` (dbWriteTable): flags populated ✓ +- persist `streams_access` (lnk_pipeline_persist INSERT): flags ALL FALSE ✗ + +`lnk_pipeline_persist:122-123` built `access_cols_v <- c(cols_streams_access_base, .lnk_cols_streams_access_per_sp(species))` — omitting `.lnk_cols_streams_access_source_flags()`. DDL had the columns; INSERT projected only base + per-species. The 6 flag columns got their default (NULL) on every INSERT. + +**Lesson**: DDL change (lnk_persist_init) and INSERT-projection change (lnk_pipeline_persist) are a matched pair. The Phase 1 code-check asserted "INSERT picks up new columns automatically (iterates names(access_cols_v))" — false, because `access_cols_v` is independently constructed and didn't include the flags. Verify DDL + INSERT together against live data, not by reading one side. + +## Isolation-debug technique that worked + +The killed run's `working_pars_dbg` schema survived the docker restart (data volume persists across container restart). It had all prerequisite barrier tables (`barriers_anthropogenic`, `barriers_pscis`, `barriers_bt`, etc.) but no `_unified` views or `streams_access` (died before mapping_code phase). Reproduced the mapping_code phase steps (lnk_barriers_views → lnk_pipeline_access → inspect) in ~30s instead of re-running the 16-min pipeline. Much faster bug isolation. + +CAUTION: running `lnk_pipeline_persist` against the incomplete `working_pars_dbg` DELETE-WHERE-WSG'd the good PARS habitat (streams_habitat_bt 48559 → 0) and replaced with the half-built working schema's (empty) data. Debug-induced data damage — needs a clean full run to repair. Don't persist from a half-built working schema. + +## Verified facts + +- persist.barriers had PARS (68254 rows, 33104 BT-blockers) — data was upstream-correct throughout; bug was purely in the persist-write projection. +- `barriers_dams_unified` (view over persist) = 178 dams province-wide; PARS-local `barriers_dams` = 0. Confirms cross-WSG dams only visible via persist. +- bcfp `streams_mapping_code` has no `watershed_group_code`; JOIN `bcfishpass.streams ON segmented_stream_id`. + +## Cause 4 — METHODOLOGY, not a bug (the deepest finding) + +After all 3 persist fixes, streams_access flags persist correctly (dams 48561, dam_ind 32370 for PARS) but mapping_code STILL diverges from bcfp. Root: `access_bt = 0` (BLOCKED) for ALL 48561 PARS segments, because every PARS segment has a downstream dam (Bennett/Peace Canyon in PCEA — PARS drains through them). link#152 cross-WSG barriers correctly flag this. + +`lnk_pipeline_mapping_code:236` `accessible <- !has_barriers_sp & !no_data`. With every segment blocked: +- token1 (`:262-268`): `ACCESS` requires `accessible` → never fires; only SPAWN/REAR (habitat-driven) or NA +- token2 (`:276`): `ifelse(accessible, mc_barrier, NA)` → always NA → no `;DAM` +- token3 (`:277`): same → no `;INTERMITTENT` + +Result: link emits `SPAWN` / `REAR` / `` (blank). bcfp emits `SPAWN;DAM` / `ACCESS;DAM` / `REAR;DAM` for the SAME segments — bcfp does NOT gate token1/token2 on downstream-dam accessibility. + +This is the v0.40.0 methodology shift at the semantic level: pre-#187 access used bcfp-staged barriers → matched bcfp (98.63%). Post-#187 access uses link's own cross-WSG-aware barriers → all PARS segments read blocked → tokens collapse. + +**RESOLVED — was NOT a methodology decision, was the wrong barriers table.** + +Grounded in the pre-#187 matching code (`git show 0cd48d0:R/lnk_compare_wsg.R`) + the research doc canonical call sequence + reading `.lnk_pipeline_prep_minimal:574-592`: + +- `barriers__min` = gradient + falls ONLY (natural barriers, per-species, minimal-reduced). These are the correct `barriers_per_sp` for access. A segment downstream of a dam is still gradient-accessible → token1 = SPAWN/ACCESS. +- `barriers__unified` (what #187 Phase 4 wrongly used) = ALL barriers incl dams → every dam-downstream segment reads blocked → accessible FALSE → token1/2 collapse to bare SPAWN/REAR. +- Dams belong in `barrier_sources` (cross-WSG `barriers_dams_unified`) → drive token2 (`DAM`) + `dam_dnstr_ind`, NEVER blocking `accessible`. That's how bcfp emits `SPAWN;DAM`: gradient-accessible habitat + dam noted. +- link#152 cross-WSG fix is in `barrier_sources` (untouched). Natural barriers are inherently local (gradient/falls don't cross WSG) so `_min` stays tunnel-free. + +**Fix applied** (uncommitted, on branch): `lnk_pipeline_run` Phase 4 `barriers_per_sp` swapped `_unified` → `_min`. Verification run in flight (PARS, cleanup_working=FALSE). Expected: token1 ACCESS/SPAWN/REAR restored, token2 DAM/MODELLED/ASSESSED present (`SPAWN;DAM` etc.), match_pct vs bcfp back toward ~98%. + +The 3 persist commits (DDL, pre-persist, INSERT projection) remain correct + necessary (the source flags must persist for token2). The `_min` swap is the 4th + final piece. + +**Tunnel-free confirmed**: build path uses `_min` (local gradient/falls) + `barrier_sources` views over persist (local). NO tunnel needed for the build. Tunnel only for the parity DIFF (validation). Aligns with user direction: "we will go tunnel free eventually, cannot depend on it for testing." + +### Cause 4 fix attempt #1 — `_min` swap FAILED (mechanical, 2026-05-23) + +The `_min` swap broke fast: `ERROR: column b.barriers_bt_min_id does not exist`. Root: `lnk_pipeline_access:152-161` derives `feature_id_col <- paste0(table_only, "_id")` and passes the table to `fresh::frs_network_features`, which needs a surrogate feature id. The `_min` tables are **break-position specs** (`blue_line_key, downstream_route_measure, wscode_ltree, localcode_ltree` ONLY — no id) built for `frs_break_apply`, NOT feature tables. The `_unified` views ARE feature-shaped (`id_barrier AS barriers__unified_id`, geom, blocks_species). So `barriers_per_sp` mechanically REQUIRES `frs_network_features`-shaped tables; `_min` cannot be dropped in. + +**What `barriers__unified` actually is** (live viewdef): `SELECT id_barrier AS barriers_bt_unified_id, barrier_source, ..., blocks_species, ..., geom FROM .barriers WHERE 'BT' = ANY(blocks_species)`. It's all persist barriers (cross-WSG) where the species is in `blocks_species` (#152 predicate). The over-blocking question reduces to: **is DAM in `blocks_species` for BT?** If yes, every PARS dam-upstream segment reads blocked → Cause 4 token collapse. + +**Correct fix shape (not yet built):** a per-species feature view that keeps the `_unified` shape + id but filters OUT anthropogenic/dam sources — natural barriers only (`barrier_source NOT IN ('DAM', ...)` or `barrier_subtype`-based). That gives `frs_network_features` its id AND the gradient/falls-only access set. BUT this is gated on confirming bcfp's actual semantics — see below. + +### RESOLVED 2026-05-23 — bcfp source read; the access set is natural-only + override-filtered + +Read authoritatively from `smnorris/bcfishpass@v0.7.15` (`gh api`, read-only): +- `model/01_access/sql/model_access_bt.sql` — `bcfishpass.barriers_bt` = + `(gradient WHERE barrier_type IN ('GRADIENT_25','GRADIENT_30') ∪ falls ∪ subsurface)` + MINUS barriers with upstream BT/salmon/steelhead **observations** (`obs_upstr`, + 20 m tol) MINUS barriers with upstream confirmed **habitat** + (`user_habitat_classification`, `hab_upstr` 200 m tol) ∪ ALL + `barriers_user_definite`. **No dams / PSCIS / modelled crossings.** +- `model/01_access/sql/load_streams_access.sql` — `barriers__dnstr` (per-sp, + natural) is SEPARATE from `barriers_anthropogenic_dnstr` / `barriers_dams_dnstr`. + `dam_dnstr_ind = array[barriers_anthropogenic_dnstr[1]] && barriers_dams_dnstr`. +- `model/02_habitat_linear/sql/load_streams_mapping_code.sql` — token2 gated on + `barriers_bt_dnstr = array[]::text[]` (accessible) — identical to link. + +So `SPAWN;DAM` = natural-accessible (`barriers_bt_dnstr=[]`) + spawning>0 + dam +downstream. The dam annotates access; it doesn't block it. + +**link's two divergences (fully characterized):** +1. `barriers_per_sp = barriers__unified` = ALL barriers incl dams (wrong + content; bcfp = natural only). +2. The observation/habitat override (`lnk_barrier_overrides` → + `.barrier_overrides`) feeds `lnk_pipeline_classify` (habitat) only — + NOT `lnk_pipeline_access` (mapping_code accessibility). bcfp applies it to + `barriers_`. + +Note `access_` integer (lnk_pipeline_access.R:364) IS obs-aware (matches +bcfp `access_bt`) — but mapping_code uses `has_barriers__dnstr`, not it. + +**Fix shape:** per-species *feature* view (has-id) of natural-only barriers +(gradient@species-threshold + falls + subsurface) + override + user_definite = +reproduce bcfp `barriers_`. Real builder, not a swap. Full writeup in +`RUNBOOK.md` §5. Design implication: `blocks_species` binary conflates +natural-access + anthropogenic-descriptor — candidate redesign. + +### (superseded) The decisive question is empirical, not theoretical — redo snapshot + +Whether bcfp gates token1/token2 on dam-accessibility must be read from bcfp's ACTUAL output, not reasoned about. Tunnel is down again (stale, timed out). Per user direction ("rerun the snapshot locally") + ("cannot depend on tunnel for testing"): running `data-raw/snapshot_bcfp.sh --with-bcfp-views --force` against local docker fwapg (tunnel-free, public sources) to land this week's bcfp into `fresh.streams_bcfp`. Then row-level diff: for PARS segments above Bennett/Peace Canyon dams, what does bcfp's `mapping_code_bt` say (`SPAWN;DAM`? bare `SPAWN`? `NONE`?) AND is the segment in bcfp's accessible set? That answer dictates the fix shape. + +## Open + +- **Cause 4 methodology decision** — blocks PARS-vs-bcfp parity. Pending user. +- Wall time 956s (~16 min) for one PARS run — double-persist overhead. See task_plan Phase 5. +- BULK also needs a clean rebuild (damaged in earlier debug churn). +- access_bt=0 everywhere also means the rollup (spawn/rear km) may be affected — verify whether habitat km changed vs historic. diff --git a/planning/active/issue_blocks_species_redesign_draft.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/issue_blocks_species_redesign_draft.md similarity index 100% rename from planning/active/issue_blocks_species_redesign_draft.md rename to planning/archive/2026-05-issue-196-streams-access-source-flags/issue_blocks_species_redesign_draft.md diff --git a/planning/active/issue_phase4d_draft.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/issue_phase4d_draft.md similarity index 100% rename from planning/active/issue_phase4d_draft.md rename to planning/archive/2026-05-issue-196-streams-access-source-flags/issue_phase4d_draft.md diff --git a/planning/active/phase4d_plan_draft.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/phase4d_plan_draft.md similarity index 100% rename from planning/active/phase4d_plan_draft.md rename to planning/archive/2026-05-issue-196-streams-access-source-flags/phase4d_plan_draft.md diff --git a/planning/archive/2026-05-issue-196-streams-access-source-flags/progress.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/progress.md new file mode 100644 index 00000000..9eb8e62f --- /dev/null +++ b/planning/archive/2026-05-issue-196-streams-access-source-flags/progress.md @@ -0,0 +1,38 @@ +# Progress — #196 streams_access per-source flags + cross-WSG mapping_code + +## Session 2026-05-19 + +- PWF scaffolded retroactively (started #196 without it; scope grew to 3 bugs). +- Phase 1 (`91f3f90`): persist DDL per-source flag cols via `.lnk_cols_streams_access_source_flags()`. +- Phase 2 (`e23819a`): pre-persist barriers so views default to persist (cross-WSG dam visibility). +- Phase 3 (`475e397`): `lnk_pipeline_persist` INSERT projection now includes the source flags — THE actual NONE-token bug. Verified in isolation: persist streams_access flags populate (anth 48559, dams 48559, dam_ind 32406). +- Branch `196-streams-access-source-flags`, 3 commits ahead of main. +- Phase 4 IN FLIGHT: clean PARS `lnk_pipeline_run(mapping_code=TRUE)` (task `btkkjqxlp`) — rebuilds habitat (damaged by debug shortcut persisting from half-built working_pars_dbg) + verifies end-to-end token output. +- Phase 4 result: streams_access flags NOW persist (dams 48561, dam_ind 32370). BUT tokens still wrong — found **Cause 4** (methodology, not bug): access_bt=0 (blocked) for ALL PARS segments because cross-WSG dams are downstream → `accessible=FALSE` → token1/2/3 collapse to SPAWN/REAR/blank. bcfp emits SPAWN;DAM regardless. See findings Cause 4. **Paused for user decision** on accessibility-gating semantics. +- 3 persist commits (91f3f90, e23819a, 475e397) are correct + shippable on their own (flags should persist). Token semantics is separate — likely new issue, possibly tied to #197 rules engine. +- Next: (a) user decides Cause-4 semantics; (b) decide Phase 5 wall-time; (c) rebuild BULK; (d) ship v0.40.3 (persist fixes) maybe independent of the token-semantics work. + +## Session 2026-05-23 + +- Resumed via `/loop continue phase 7`. Tested the Cause-4 `_min` swap (uncommitted edit to lnk_pipeline_run barriers_per_sp). +- `_min` swap **FAILED mechanically** (PARS run `bpzeq473w`): `barriers_bt_min_id does not exist`. `_min` tables are break-specs (no surrogate id); `frs_network_features` (via lnk_pipeline_access) requires a feature id. `_unified` views are the feature-shaped tables (id_barrier + geom + blocks_species). `barriers_per_sp` mechanically requires `_unified`-shape input. +- Architectural truth: `barriers__unified` = persist.barriers WHERE sp ∈ blocks_species. Over-blocking reduces to "is DAM in blocks_species for BT". Correct fix = per-species natural-only feature view (keeps id, drops dams) — BUT gated on confirming bcfp's real semantics. +- Tunnel down again (stale, timeout). Per user ("rerun the snapshot locally"): launched `snapshot_bcfp.sh --with-bcfp-views --force` against local docker fwapg (tunnel-free, public sources) → task `be94vol3c`. Lands this week's bcfp into `fresh.streams_bcfp` for the row-level dam-segment diff that decides the fix shape. +- The `_min` swap is still in the working tree (uncommitted) — to be reverted once snapshot confirms direction. +- Next: snapshot completes → inspect bcfp dam-upstream segment mapping_code → decide fix shape → surface to user. + +## Session 2026-05-23 (cont.) — mechanism resolved by reading bcfp source + +- User pushed hard: "dam blockage is species specific based on our rules", "fish are above these dams, dam means nothing on its own", "how does bcfp do it? understand mechanisms", "blocks_species maybe the wrong design — nothing black-white", "we have species overrides (fish above → override)", "do our homework and DOCUMENT so we can find it. tricky to carry all this knowledge". → wrote **RUNBOOK.md** (repo root) + CLAUDE.md pointer. +- Read bcfp source authoritatively (`gh api`, smnorris/bcfishpass@v0.7.15): + `model_access_bt.sql`, `load_streams_access.sql`, `load_streams_mapping_code.sql`. + **Finding:** bcfp `barriers_` = natural-only (gradient@species-threshold + falls + subsurface) MINUS upstream-observation/habitat overrides ∪ user_definite. Dams NEVER in the access set — they annotate (token2) via separate `barriers_dams_dnstr`. token2 gate identical to link. +- **link's bug, fully characterized:** (1) `barriers_per_sp = barriers__unified` carries dams (wrong content); (2) `lnk_barrier_overrides` output feeds classify (habitat) not access (mapping_code). Documented in RUNBOOK §5 + findings. +- Reverted broken `_min` swap → `_unified` (runnable, known-divergent), comment → RUNBOOK §5. +- Confirmed user's instincts on every point. The real fix = build a natural-only per-species feature view reproducing bcfp barriers_ + wire overrides into access. Real work (Phase 4d), needs scoping. +- streams_vw_bcfp load failed on transient gzip (snapshot otherwise OK exit 0); retrying (task b122hazyb) for empirical parity numbers. +- Next: user reviews the mechanism/runbook; scope Phase 4d; decide whether blocks_species redesign is its own issue. + +### Environment +- docker `fresh-db` up (was down mid-session; `open -a Docker` + `docker compose up -d db` in fresh/docker/). +- bcfp tunnel `localhost:63333` flaky (dropped 3×); needs PG_PASS_SHARE. Build is tunnel-free; only parity diff needs it. diff --git a/planning/archive/2026-05-issue-196-streams-access-source-flags/task_plan.md b/planning/archive/2026-05-issue-196-streams-access-source-flags/task_plan.md new file mode 100644 index 00000000..f2e7a3fb --- /dev/null +++ b/planning/archive/2026-05-issue-196-streams-access-source-flags/task_plan.md @@ -0,0 +1,86 @@ +# Task: streams_access per-source flag persistence + cross-WSG mapping_code (#196) + +Retroactive PWF — #196 was started as a "quick single-file fix" without PWF, then ballooned into a 3-bug investigation. Scaffolded mid-flight 2026-05-19 to capture the trace. + +## Problem + +Post-#187 `lnk_mapping_code` second token defaulted to `NONE` for every segment (should be `DAM`/`MODELLED`/`ASSESSED`/`REMEDIATED`/`NONE`). PARS BT match_pct vs bcfp dropped from historic ~98% to ~30-50%. Three coupled root causes, fixed in three commits. + +## Phase 1 — persist DDL: per-source flag columns + +- [x] `.lnk_cols_streams_access_source_flags()` helper in `lnk_persist_init.R` — 6 boolean cols (`has_barriers_{anthropogenic,pscis,dams,remediations}_dnstr`, `dam_dnstr_ind`, `remediated_dnstr_ind`). +- [x] Wired into `streams_access` CREATE TABLE. +- [x] `/code-check` clean. +- [x] Commit `91f3f90`. + +## Phase 2 — cross-WSG visibility: pre-persist barriers + +- [x] `lnk_pipeline_run` mapping_code phase: pre-persist current WSG's barriers BEFORE `lnk_barriers_views`, so views default to persist (province-wide, cross-WSG dam visibility per link#152). Reverted #187 Phase 4's `barriers_table = working` override. +- [x] `/code-check` clean. +- [x] Commit `e23819a`. + +## Phase 3 — persist INSERT projection (the actual NONE bug) + +- [x] `lnk_pipeline_persist`: `access_cols_v` was `base + per_sp` only — MISSING `.lnk_cols_streams_access_source_flags()`. DDL had the cols (Phase 1) but INSERT never populated them → NULL → NONE token. Added the generator to the projection. +- [x] Verified in isolation: persist `streams_access` flags now populate (anth 48559, dams 48559, dam_ind 32406 of 48559 for PARS). +- [x] `/code-check` — SKIPPED on this commit (verified empirically instead; the earlier code-check that missed this gap was the cautionary tale). +- [x] Commit `475e397`. + +## Phase 4 — end-to-end verification (IN FLIGHT) + +- [ ] Clean `lnk_pipeline_run(aoi="PARS", mapping_code=TRUE)` — rebuilds habitat (damaged by debug shortcut) + access + mapping_code. (task `btkkjqxlp`, ~16 min) +- [ ] Verify PARS BT `mapping_code_bt` distribution includes `ACCESS;DAM`/`ACCESS;MODELLED`/`ACCESS;ASSESSED` (not just `ACCESS;NONE`). +- [ ] Tunnel up → `lnk_compare_wsg(aoi="PARS", mapping_code=TRUE)` → match_pct vs bcfp ≥ ~95%. +- [ ] Re-run BULK too (also damaged / needs rebuild). + +## Phase 4b — Cause 4 fix: barriers_per_sp `_unified` → `_min` (NEW) + +- [x] Root-caused: `_unified` (all barriers) over-blocks; `_min` (gradient+falls natural barriers) is correct per-species access set. Dams stay in `barrier_sources` (token2 only). Grounded in pre-#187 matching code + prep_minimal source. +- [x] Swap applied in `lnk_pipeline_run` Phase 4 (uncommitted). +- [x] Verify: PARS run (task `bpzeq473w`) → **FAILED mechanically**. `barriers_bt_min_id does not exist` — `_min` tables are break-specs (no id col); `frs_network_features` needs a feature id. `_min` cannot feed `barriers_per_sp`. See findings. +- [x] Architectural finding: `barriers__unified` = persist barriers WHERE species ∈ blocks_species (#152), feature-shaped w/ id. Correct fix = per-species feature view, `_unified` shape, natural-only filter — but gated on bcfp semantics. +- [ ] **REVERT the `_min` swap** in lnk_pipeline_run (restore `_unified`) — known state for the data investigation. (deferred until snapshot confirms direction) + +## Phase 4c — empirical bcfp semantics (BLOCKING — redo snapshot) + +- [x] Tunnel down (stale). Snapshot script is tunnel-free (public sources). Run `snapshot_bcfp.sh --with-bcfp-views --force` → local `fresh.streams_bcfp` (task `be94vol3c`). +- [ ] Inspect `fresh.streams_bcfp` columns — does it carry `mapping_code_bt` + access cols per segment? +- [ ] Row-level: PARS segments above Bennett/Peace Canyon dams — bcfp `mapping_code_bt` value? In bcfp's accessible set or not? +- [x] **RESOLVED by reading bcfp source** (not data — authoritative). Fix shape = (a): per-species access set is natural-only + override-filtered; dams never block access. See findings (RESOLVED 2026-05-23) + RUNBOOK.md §5. +- [x] Reverted broken `_min` swap → `_unified` (runnable, known-divergent) w/ comment → RUNBOOK §5. +- [x] Documented authoritatively in RUNBOOK.md (§2a, §5) + CLAUDE.md pointer + findings. (user ask: "do our homework and document so we can find it") +- [ ] streams_vw_bcfp retry (task b122hazyb) for empirical parity numbers (snapshot's streams view failed on transient gzip). + +## Phase 4d — the real fix (FILED as #200; #201 = redesign follow-up) + +- [x] Filed #200 (access-set reproduction) + #201 (blocks_species redesign + dam-override, depends on #200). +- [ ] Build per-species NATURAL-only *feature* view reproducing bcfp `barriers_`: + gradient@species-threshold + falls + subsurface, MINUS observation/habitat + overrides, ∪ user_barriers_definite. Has-id (feature shape, §2b). +- [ ] Wire `lnk_barrier_overrides` output into the access path (currently classify-only). +- [ ] Point `barriers_per_sp` at the new view; dams stay in barrier_sources (token2). +- [x] `blocks_species` redesign (carry ingredients, classify access late) + dam-override — filed as #201 (RUNBOOK §5 design implication). + +## Phase 5 — performance concern (NEW — decide before merge) + +- [ ] Wall time blew up to 956s (~16 min) vs normal ~3.5 min for PARS — the double-persist (pre-persist barriers + final persist) re-writes streams + habitat + barriers twice. Decide: + - (a) Accept it (correctness over speed; provincial run adds ~hours though). + - (b) Pre-persist ONLY barriers (not streams + habitat) — the views only need barriers. Smaller helper / `only=` arg on lnk_pipeline_persist. + - (c) Reorder so persist runs once, mapping_code phase after. + - Likely (b). File as part of #196 or follow-up. + +## Phase 6 — release v0.40.3 + +- [ ] DESCRIPTION 0.40.2 → 0.40.3, Date. +- [ ] NEWS.md entry. +- [ ] CLAUDE.md branch ref. +- [ ] `/planning-archive`. +- [ ] `/gh-pr-push` + `/gh-pr-merge`. + +## Validation + +- [ ] PARS BT tokens include DAM/MODELLED/ASSESSED +- [ ] match_pct vs bcfp restored to ~98% +- [ ] `devtools::test()` passing +- [ ] wall-time decision made (Phase 5) +- [ ] `/code-check` clean on any remaining commits diff --git a/tests/testthat/test-lnk_barriers_unify.R b/tests/testthat/test-lnk_barriers_unify.R index 421fff90..47460eaf 100644 --- a/tests/testthat/test-lnk_barriers_unify.R +++ b/tests/testthat/test-lnk_barriers_unify.R @@ -93,6 +93,48 @@ test_that("lnk_barriers_unify includes subsurface_flow when staging table presen expect_match(sql, "'SUBSURFACE-' \\|\\|") }) +test_that("lnk_barriers_unify emits a USER_DEFINITE branch from barriers_definite", { + captured <- character(0) + m_query <- mockery::mock(data.frame(x = integer(0))) # subsurface absent + m_quote_str <- mockery::mock(DBI::SQL("'working_pars'"), cycle = TRUE) + local_mocked_bindings( + .lnk_db_execute = function(conn, sql) { + captured <<- c(captured, sql); invisible(NULL) + } + ) + with_mocked_bindings( + dbGetQuery = m_query, + dbQuoteString = m_quote_str, + .package = "DBI", + { + lnk_barriers_unify( + conn = structure(list(), class = "DBIConnection"), + aoi = "PARS", + cfg = cfg_fixture(), + loaded = loaded_fixture(), + schema = "working_pars" + ) + } + ) + + sql <- paste(captured, collapse = "\n") + + # User-definite branch: own barrier_source + namespaced id, sourced from + # .barriers_definite, ltree resolved via the FWA join (mirrors + # FALLS + bcfp barriers_user_definite.sql). link#200. + expect_match(sql, "'USER_DEFINITE'::text\\s+AS barrier_source") + expect_match(sql, "FROM working_pars\\.barriers_definite d") + expect_match(sql, "'USER_DEFINITE-' \\|\\|") + expect_match(sql, "JOIN whse_basemapping\\.fwa_stream_networks_sp s") + # Blocks all species in the set (the override-exempt user set is universal). + expect_match(sql, + "ARRAY\\['BT', 'CH', 'CM', 'CO', 'PK', 'SK', 'ST', 'WCT'\\]::text\\[\\]\\s+AS blocks_species") + # Only blue_line_key + downstream_route_measure referenced from `d` + # (empty-fallback shape safety) — no d.wscode_ltree / d.barrier_type. + expect_no_match(sql, "d\\.wscode_ltree") + expect_no_match(sql, "d\\.barrier_type") +}) + test_that("lnk_barriers_unify derives gradient blocks_species per class from parameters_fresh", { captured <- character(0) m_query <- mockery::mock(data.frame(x = integer(0))) diff --git a/tests/testthat/test-lnk_barriers_views.R b/tests/testthat/test-lnk_barriers_views.R index 661df49e..f1e0f8a0 100644 --- a/tests/testthat/test-lnk_barriers_views.R +++ b/tests/testthat/test-lnk_barriers_views.R @@ -14,8 +14,9 @@ test_that("lnk_barriers_views emits one DROP+CREATE per species + 3 source views sql <- paste(captured, collapse = "\n") - # 8 species → 8 DROP + 8 CREATE; plus 3 source views → 3 DROP + 3 CREATE = 22. - expect_equal(length(captured), 22L) + # 8 species → DROP+CREATE for _unified AND _access (4 each = 32); plus + # 3 source views → 3 DROP + 3 CREATE = 6. Total 38. + expect_equal(length(captured), 38L) # Per-species views land under .barriers__unified, with # the id alias `barriers__unified_id` for fresh's feature_id_col @@ -26,6 +27,18 @@ test_that("lnk_barriers_views emits one DROP+CREATE per species + 3 source views expect_match(sql, "CREATE OR REPLACE VIEW working_pars\\.barriers_wct_unified AS") expect_match(sql, "WHERE 'WCT' = ANY\\(blocks_species\\)") + # Per-species ACCESS views (link#200): natural-only filter, definite + # override-exemption, anti-join the province-wide barrier_overrides, + # feature id `barriers__access_id`. + expect_match(sql, "CREATE OR REPLACE VIEW working_pars\\.barriers_bt_access AS") + expect_match(sql, "id_barrier AS barriers_bt_access_id") + expect_match(sql, + "barrier_source IN \\('GRADIENT', 'FALLS', 'SUBSURFACE_FLOW', 'USER_DEFINITE'\\)") + expect_match(sql, "barrier_source = 'USER_DEFINITE'") + expect_match(sql, "FROM fresh\\.barrier_overrides o") + expect_match(sql, "o\\.species_code = 'BT'") + expect_match(sql, "abs\\(o\\.downstream_route_measure - b\\.downstream_route_measure\\) < 1") + # Underlying table is the persist-schema unified table (fresh.barriers # for the bcfishpass bundle). expect_match(sql, "FROM fresh\\.barriers") @@ -57,11 +70,14 @@ test_that("lnk_barriers_views honours a custom species set", { sql <- paste(captured, collapse = "\n") - # 2 species → 2 DROP + 2 CREATE; plus 3 source views → 6 + 3 + 3 = 10. - expect_equal(length(captured), 10L) + # 2 species → 4 each (_unified + _access DROP+CREATE) = 8; plus 3 source + # views → 6. Total 14. + expect_equal(length(captured), 14L) expect_match(sql, "barriers_bt_unified") - expect_match(sql, "barriers_sk_unified") + expect_match(sql, "barriers_bt_access") + expect_match(sql, "barriers_sk_access") expect_no_match(sql, "barriers_ch_unified") + expect_no_match(sql, "barriers_ch_access") }) test_that("lnk_barriers_views validates argument shapes", { diff --git a/tests/testthat/test-lnk_persist_init.R b/tests/testthat/test-lnk_persist_init.R index b34e331e..f2dd8f9c 100644 --- a/tests/testthat/test-lnk_persist_init.R +++ b/tests/testthat/test-lnk_persist_init.R @@ -91,6 +91,13 @@ test_that("lnk_persist_init creates schema + streams + per-species habitat table "CREATE INDEX IF NOT EXISTS barriers_blk_drm_idx ON fresh\\.barriers \\(blue_line_key, downstream_route_measure\\)") expect_match(joined, "CREATE INDEX IF NOT EXISTS barriers_geom_idx ON fresh\\.barriers USING GIST") + + # Province-wide barrier-overrides table (link#200) — shape + indexes. + expect_match(joined, "CREATE TABLE IF NOT EXISTS fresh\\.barrier_overrides") + expect_match(joined, + "PRIMARY KEY \\(blue_line_key, downstream_route_measure, species_code, watershed_group_code\\)") + expect_match(joined, + "CREATE INDEX IF NOT EXISTS barrier_overrides_blk_drm_idx ON fresh\\.barrier_overrides \\(blue_line_key, downstream_route_measure\\)") }) test_that("lnk_persist_init errors on invalid inputs", { diff --git a/tests/testthat/test-lnk_pipeline_persist.R b/tests/testthat/test-lnk_pipeline_persist.R index 0dff2cf4..11003d4b 100644 --- a/tests/testthat/test-lnk_pipeline_persist.R +++ b/tests/testthat/test-lnk_pipeline_persist.R @@ -135,6 +135,57 @@ test_that("lnk_pipeline_persist persists barriers when staging table present", { "INSERT INTO fresh\\.barriers \\(id_barrier, watershed_group_code,.*blocks_species.*geom\\)\\s+SELECT id_barrier, watershed_group_code,.*\\s+FROM working_pars\\.barriers") }) +test_that("lnk_pipeline_persist persists barrier_overrides when staging table present", { + captured <- character(0) + local_mocked_bindings( + .lnk_db_execute = function(conn, sql) { + captured <<- c(captured, sql); invisible(NULL) + } + ) + # Probe returns one row → .barrier_overrides exists. + with_mocked_bindings( + dbGetQuery = function(...) data.frame(x = 1L), + dbQuoteString = function(...) DBI::SQL("'working_pars'"), + .package = "DBI", + { + cfg <- lnk_config("bcfishpass") + lnk_pipeline_persist( + structure(list(), class = "DBIConnection"), + aoi = "PARS", cfg = cfg, species = "BT") + } + ) + + joined <- paste(captured, collapse = "\n") + # DELETE-WHERE-WSG + INSERT; working table lacks watershed_group_code so + # the AOI is injected as a literal in the SELECT projection (link#200). + expect_match(joined, + "DELETE FROM fresh\\.barrier_overrides WHERE watershed_group_code = 'PARS'") + expect_match(joined, + "INSERT INTO fresh\\.barrier_overrides \\(blue_line_key, downstream_route_measure, species_code, watershed_group_code\\)\\s+SELECT blue_line_key, downstream_route_measure, species_code, 'PARS'::varchar\\(4\\) FROM working_pars\\.barrier_overrides") +}) + +test_that("lnk_pipeline_persist skips barrier_overrides branch when staging table absent", { + captured <- character(0) + local_mocked_bindings( + .lnk_db_execute = function(conn, sql) { + captured <<- c(captured, sql); invisible(NULL) + } + ) + m <- mock_dbi_no_barriers() + with_mocked_bindings( + dbGetQuery = m$dbGetQuery, dbQuoteString = m$dbQuoteString, + .package = "DBI", + { + cfg <- lnk_config("bcfishpass") + lnk_pipeline_persist( + structure(list(), class = "DBIConnection"), + aoi = "PARS", cfg = cfg, species = "BT") + } + ) + joined <- paste(captured, collapse = "\n") + expect_no_match(joined, "fresh\\.barrier_overrides") +}) + test_that("lnk_pipeline_persist skips barriers branch when staging table absent", { captured <- character(0) local_mocked_bindings(