diff --git a/CLAUDE.md b/CLAUDE.md index 3cb56938..75ebcb64 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,6 +15,12 @@ Experimental package — breaking all the time and loving the learning curve. St **Key correction (do NOT re-rabbit-hole):** the pipeline builds `.crossings` **from DB primitives** (`lnk_pipeline_crossings` → `.lnk_crossings_union`: PSCIS + `fresh.modelled_stream_crossings` + CABD), **not** from `crossings.csv`. The CSV read at `lnk_pipeline_load.R:100` is **vestigial** — the union drops + rebuilds the table before break/classify/mapping_code touch it. So #231 ("consume weekly crossings.csv; repoint pipeline off fresh") was **closed as misdirected**; lessons in `planning/archive/2026-07-issue-231-crossings-from-primitives/README.md` (+ memory `crossings-built-from-primitives`). Freshness lever = `data-raw/snapshot_bcfp.sh` reloading the primitives into the **`fwapg`** DB (last load ~2026-05-26; `lnk_db_conn()` defaults to a `bcfishpass` DB that LACKS them). Opened **#232** — confirm link's built crossings ≈ bcfp's complete `crossings_vw` (the parity reference). Aside: `crossings.csv` was published to `s3://newgraph` (db_newgraph#15, smnorris PR #57) before we realized the models don't consume it — **db_newgraph#16** tracks reconsidering that dump. +## Status (2026-07-31) — v0.44.3 shipped (#233 config dictionaries + ownership boundary) + +Both config CSVs now have data dictionaries: `configs/dictionary_dimensions.csv` (renamed from `dimensions_columns.csv`) and the new `configs/dictionary_parameters_fresh.csv` (19 rows — type, group, `owner`, `consumed_by`, default, description). **The point was not documentation, it was stopping the re-derivation:** the fresh↔link `parameters_fresh` column-ownership split had been settled long ago by [fresh#129](https://github.com/NewGraphEnvironment/fresh/issues/129) (fresh 0.12.7 *removed* `observation_*` — "fish passage interpretation belongs in link, not the network engine") but was only findable by archaeology through two repos' planning archives, so it kept getting re-worked from scratch. It is now the `owner` column — **14 fresh-owned engine params, 5 link-owned `observation_*`** — read by `audit_configs.R` §3b instead of a hardcoded `grepl("^observation_", ...)`, and written up in [`RUNBOOK.md`](RUNBOOK.md) §7 "Who owns which `parameters_fresh` column". Adding a link-owned column is now a dictionary edit, not a regex edit. + +**Two findings from machine-verifying every `consumed_by` file:line (24/24) rather than inferring them:** link never reads the nine `cluster_*` columns at all — it only passes the frame through (`lnk_pipeline_connect.R:107`) to fresh's `.frs_run_connectivity()`; and **`rear_gradient_min` is read by no code in either package** (recorded as unused, not dropped — fresh owns that schema). **Gotcha worth knowing:** the bundles carry *different* column subsets — bcfishpass `dimensions.csv` has 30 columns to the three `default*` bundles' 32 — so any dictionary/coverage check must assert against the **union**, never a single bundle. Guarded in two layers because `data-raw/` is `.Rbuildignore`d and never runs for an installed package: `tests/testthat/test-dictionaries.R` (+23, the CI-side guard) and the audit's coverage / reverse-consistency / missing-dictionary flags (negative-tested — dropping one row exits 1). Also removed `audit_configs.R`'s hardcoded `setwd("/Users/airvine/...")`: the script now derives its repo root from its own location and resolves paths via `repo_path()`, so it runs from any cwd and mutates none. Open follow-ups unchanged: **#224**, **#225**, **#227** (the `public.wsg_outlet` builder — its absence is the one standing test failure, `test-lnk_wsg_resolve.R:138`). + ## Status (2026-07-04) — v0.44.1 shipped (#226 vignette accessible_km) Extended the PARS vignette with an **Accessible habitat (km)** section proving `accessible_km` bcfp-equivalence (link 6,822.5 vs bcfp 6,822.9 km BT, **−0.01%**; table from cached `inst/vignette-data/pars_accessible.rds`). **Gotcha that bit hard:** "regenerate the vignette artifacts" was NOT docs-only — the two persist configs drift in segmentation because only WSGs re-modelled post-#223 are dense. `fresh` (bcfp config) had PARS at 97,538 segs but `fresh_default` (default/grayling) was still pre-#223 (48,558); the gpkg's single `streams` layer joins `fresh` geometry to `fresh_default` `mapping_code_gr` on `id_segment`, so a naive regen attaches grayling tokens to mismatched geometry → corrupt GR map. Fix: re-model the lagging config (`data-raw/wsg_run_one.R` + `merge=TRUE` recompute via `wsg_recompute_one.R` for cross-WSG `;DAM`) so both share segmentation; `wsg_vignette_data.R` now carries a **segmentation-parity guard** that refuses a mixed build. Any cross-config artifact joined on `id_segment` must verify both sides share segmentation first. mapping_code parity refreshed 99.04%→98.91% (denser post-#223). Open follow-ups unchanged: **#224**, **#225**, **#227**. @@ -253,7 +259,7 @@ To run the entire province: loop over watershed groups. Or pass any AOI with `sp - #45 — Gradient classes cleanup (derive from `loaded$parameters_fresh$access_gradient_max`) - #52 — Channel-class break positions vs gradient thresholds (research) - #53 — Distribute tar_make across M4 + M1 + db_newgraph -- #75 — `dimensions_columns.csv` as source-of-truth: auto-gen README + `lnk_rules_build()` validation (CSV seeded in v0.17.0) +- #75 — `dictionary_dimensions.csv` as source-of-truth: auto-gen README + `lnk_rules_build()` validation (CSV seeded in v0.17.0) ## Recently closed diff --git a/DESCRIPTION b/DESCRIPTION index e5fa49be..ba44a191 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: link Title: Stream Network Habitat Interpretation (Experimental) -Version: 0.44.2 +Version: 0.44.3 Date: 2026-07-06 Authors@R: c( person("Allan", "Irvine", , "airvine@newgraphenvironment.com", diff --git a/NEWS.md b/NEWS.md index 9313c7e2..9996eba3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +# link 0.44.3 + +Data dictionaries for both config CSVs, and an end to re-deriving the fresh↔link column-ownership boundary ([#233](https://github.com/NewGraphEnvironment/link/issues/233)). `inst/extdata/configs/dimensions_columns.csv` is renamed `dictionary_dimensions.csv` (pure rename — no code read it; the only live reference was the #75 entry in `CLAUDE.md`), establishing a `dictionary_` convention for its new sibling `dictionary_parameters_fresh.csv`. The new file documents all 19 `parameters_fresh.csv` columns — type, group, `owner`, `consumed_by`, default-when-absent, description — where previously the 9 `cluster_*` columns were undocumented outside fresh's source and the rest lived in scattered prose. Every `consumed_by` is a machine-verified `file:line` (24/24 resolve), which corrected two things worth knowing: link does not read the `cluster_*` columns at all, it only passes the frame through (`R/lnk_pipeline_connect.R:107`) to fresh's `.frs_run_connectivity()`, and `rear_gradient_min` is read by **no code in either package** — recorded as unused rather than dropped, since fresh owns that schema. The `owner` column encodes, rather than restates, the partition settled by [fresh#129](https://github.com/NewGraphEnvironment/fresh/issues/129) (shipped fresh 0.12.7, which removed `observation_*` from fresh: "fish passage interpretation belongs in link, not the network engine"): 14 fresh-owned engine params, 5 link-owned `observation_*` interpretation params. `data-raw/audit_configs.R` §3b now reads that column instead of hardcoding `grepl("^observation_", ...)`, so a new link-owned column is documented once rather than taught to a regex, and gains three guards — undocumented-column coverage, a reverse check (dictionary claims link-owned but fresh ships it = stale dictionary), and a missing-dictionary guard; negative-tested by dropping a row, which exits 1. Because `data-raw/` is `.Rbuildignore`d and never runs for an installed package, the durable guard is the new `tests/testthat/test-dictionaries.R` (+23 tests), which checks coverage in both directions against the **union** of bundle columns — bundles legitimately carry different subsets, bcfishpass's `dimensions.csv` having 30 columns to the defaults' 32. `RUNBOOK.md` §7 gains the ownership section so the boundary is findable without archaeology through two repos' planning archives. Separately, `audit_configs.R` no longer `setwd()`s to a hardcoded `/Users/airvine/...` path — it derives the repo root from its own script location and resolves every path through a new `repo_path()` helper, so the audit runs from any working directory (verified from `/tmp`), works for anyone's checkout, and mutates no global state when `source()`d. + # link 0.44.2 Reader-facing prose cleanup on the PARS vignette (`vignettes/pars-habitat-connectivity.Rmd`), from review — no model, artifact, or numbers change. Dropped internal issue numbers and build-infrastructure ("tunnel") references, and cut the per-segment `mapping_code` disagreement detail (the top-diff-pattern columns and the "remaining disagreements" paragraph) since bull-trout parity is ~99%. The accessible / spawning / rearing km table stays but is reframed around `lnk_rollup_wsg()` as a standalone habitat-totals roll-up — link's own numbers, no reference required — that happens to match bcfishpass, rather than a proof-of-parity exercise. Removed the `mapping_code` symbology paragraph entirely: the colour semantics were easy to mis-state (e.g. red marks an accessible reach whose downstream PSCIS crossing is ranked a *barrier*, not merely "field-assessed"), and each map already carries its own colour legend. diff --git a/RUNBOOK.md b/RUNBOOK.md index 3d76b221..89f72661 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -417,9 +417,39 @@ direction. Not yet scoped; candidate issue. | 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. | - -Two gaps worth knowing: **species residence** is hardcoded (data-drive is -follow-up #189), and **dam blocking is not rules-driven** at all (universal). +| What each config column means | `configs/dictionary_dimensions.csv`, `configs/dictionary_parameters_fresh.csv` | data dictionaries — per-column type, group, default, description, and (for `parameters_fresh`) `owner` + `consumed_by` file:line | + +### Who owns which `parameters_fresh` column + +**Do not re-derive this.** `parameters_fresh.csv` is co-owned, and the split is +settled: + +- **fresh owns the 14 network-engine columns** — `species_code`, + `access_gradient_max`, the two `*_gradient_min`, and the nine `cluster_*`. + fresh ships them in its own `inst/extdata/parameters_fresh.csv`; link's + bundles are seeded from it. +- **link owns the 5 `observation_*` columns** — fish-passage interpretation + (counts, thresholds, date windows, species pooling, control veto). + +The boundary was decided in +[fresh#129](https://github.com/NewGraphEnvironment/fresh/issues/129) (shipped +fresh 0.12.7), which *removed* `observation_*` from fresh after fresh#69 had +added them: "fish passage interpretation belongs in link, not the network +engine." Values may diverge freely per bundle — link tunes them — but the +**column set** is contractual. + +Enforced in two places, both of which read `dictionary_parameters_fresh.csv`'s +`owner` column rather than hardcoding the rule: +`data-raw/audit_configs.R` §3b (pre-trifecta gate) and +`tests/testthat/test-dictionaries.R` (runs in CI, since `data-raw/` is +`.Rbuildignore`d). Direction of travel is opposite for the two shared artifacts: +`rules.yaml` flows **link → fresh** (link owns the generator, `lnk_rules_build()`); +the `parameters_fresh` column schema flows **fresh → link**. + +Three gaps worth knowing: **species residence** is hardcoded (data-drive is +follow-up #189), **dam blocking is not rules-driven** at all (universal), and +`rear_gradient_min` is carried in the schema but **read by no code in either +package** — it is fresh-owned, so removing it is a fresh-side call. If dam blocking should ever become species-specific, it's a new per-source-per-species column + `lnk_barriers_unify` change — not a tweak. diff --git a/data-raw/audit_configs.R b/data-raw/audit_configs.R index 9e2a706d..ccc6136e 100644 --- a/data-raw/audit_configs.R +++ b/data-raw/audit_configs.R @@ -11,8 +11,30 @@ suppressPackageStartupMessages({ library(yaml); library(digest); library(tibble); library(dplyr) }) -setwd("/Users/airvine/Projects/repo/link") -devtools::load_all(quiet = TRUE) +# Resolve the repo root from this script's own location — it lives at +# /data-raw/audit_configs.R. Every path below goes through repo_path(), +# so the audit neither depends on nor mutates the caller's working directory. +# (Previously this setwd()'d to a hardcoded /Users path, which made the script +# unrunnable for anyone else and silently wrong from another checkout.) +script_path <- local({ + hit <- grep("^--file=", commandArgs(trailingOnly = FALSE), value = TRUE) + if (length(hit) > 0) return(sub("^--file=", "", hit[1])) + # sourced rather than Rscript'd: find the sourcing frame's file + for (i in seq_len(sys.nframe())) { + of <- sys.frames()[[i]]$ofile + if (!is.null(of)) return(of) + } + NULL +}) +if (is.null(script_path)) { + stop("Cannot resolve this script's location. Run it as:\n", + " Rscript data-raw/audit_configs.R", call. = FALSE) +} +repo_root <- normalizePath(file.path(dirname(script_path), ".."), + mustWork = TRUE) +repo_path <- function(...) file.path(repo_root, ...) + +devtools::load_all(repo_root, quiet = TRUE) bundles <- c("bcfishpass", "default") @@ -58,8 +80,8 @@ if (nrow(drifted) > 0) { # --------------------------------------------------------------------------- cat("\n--- 2. rules.yaml regen vs committed ---\n") for (b in bundles) { - dim_csv <- sprintf("inst/extdata/configs/%s/dimensions.csv", b) - rules_committed <- sprintf("inst/extdata/configs/%s/rules.yaml", b) + dim_csv <- repo_path(sprintf("inst/extdata/configs/%s/dimensions.csv", b)) + rules_committed <- repo_path(sprintf("inst/extdata/configs/%s/rules.yaml", b)) # edge_types = "explicit" to match how the committed rules.yaml is actually # built (data-raw/build_rules.R + regen_provenance.R). Regenerating with # "categories" here is what produced the earlier spurious all-species diff. @@ -83,10 +105,10 @@ for (b in bundles) { # --------------------------------------------------------------------------- cat("\n--- 3. Species axis consistency per bundle ---\n") for (b in bundles) { - dim_csv <- sprintf("inst/extdata/configs/%s/dimensions.csv", b) - pf_csv <- sprintf("inst/extdata/configs/%s/parameters_fresh.csv", b) - wsg_csv <- sprintf("inst/extdata/configs/%s/overrides/wsg_species_presence.csv", b) - yaml_path <- sprintf("inst/extdata/configs/%s/rules.yaml", b) + dim_csv <- repo_path(sprintf("inst/extdata/configs/%s/dimensions.csv", b)) + pf_csv <- repo_path(sprintf("inst/extdata/configs/%s/parameters_fresh.csv", b)) + wsg_csv <- repo_path(sprintf("inst/extdata/configs/%s/overrides/wsg_species_presence.csv", b)) + yaml_path <- repo_path(sprintf("inst/extdata/configs/%s/rules.yaml", b)) dim_sp <- gsub('"', '', utils::read.csv(dim_csv, stringsAsFactors = FALSE, check.names = FALSE)[[1]]) @@ -140,29 +162,59 @@ for (b in bundles) { # 3b. parameters_fresh column drift (fresh canonical vs link config) # --------------------------------------------------------------------------- # fresh owns the access/cluster parameter SCHEMA; link hand-authors per-bundle -# copies seeded from it plus link-only `observation_*` extensions. Values -# legitimately diverge (link tunes them) — only the COLUMN SET matters here. -# A column fresh added that link is missing means link's copy may no longer -# load cleanly through frs_habitat(); flag it. See link#129 for the -# directionality (link->fresh for rules.yaml; fresh->link col-schema for this). +# copies seeded from it plus link-only extensions. Values legitimately diverge +# (link tunes them) — only the COLUMN SET matters here. A column fresh added +# that link is missing means link's copy may no longer load cleanly through +# frs_habitat(); flag it. See link#129 for the directionality (link->fresh for +# rules.yaml; fresh->link col-schema for this). +# +# Ownership is NOT hardcoded here. It is declared per-column in +# `dictionary_parameters_fresh.csv` (`owner` = fresh|link), which encodes the +# partition settled by NewGraphEnvironment/fresh#129 — reading it means a new +# link-owned column is documented once, not taught to a regex here. The +# dictionary is itself guarded by tests/testthat/test-dictionaries.R. cat("\n--- 3b. parameters_fresh column drift (fresh canonical vs link) ---\n") pf_fresh_path <- system.file("extdata", "parameters_fresh.csv", package = "fresh") +dict_pf_path <- repo_path("inst/extdata/configs/dictionary_parameters_fresh.csv") +dict_pf <- if (file.exists(dict_pf_path)) { + utils::read.csv(dict_pf_path, stringsAsFactors = FALSE, check.names = FALSE) +} else { + NULL +} +if (is.null(dict_pf)) { + flag("3b", sprintf("dictionary missing: %s", dict_pf_path)) +} else if (!all(c("column", "owner") %in% names(dict_pf))) { + flag("3b", sprintf("dictionary lacks column/owner fields: %s", dict_pf_path)) +} if (!nzchar(pf_fresh_path)) { cat(" (fresh's bundled parameters_fresh.csv not found — is fresh installed?)\n") -} else { +} else if (!is.null(dict_pf)) { cols_fresh <- names(utils::read.csv(pf_fresh_path, stringsAsFactors = FALSE, check.names = FALSE, nrows = 1)) + owned_link <- dict_pf$column[dict_pf$owner == "link"] cat(sprintf(" fresh canonical (%d cols): %s\n", length(cols_fresh), paste(cols_fresh, collapse = ", "))) + cat(sprintf(" dictionary declares %d link-owned: %s\n", + length(owned_link), paste(owned_link, collapse = ", "))) + + # The dictionary must also agree with fresh about who owns what. A column + # fresh ships but the dictionary calls link-owned is a stale dictionary. + mislabelled <- intersect(owned_link, cols_fresh) + if (length(mislabelled) > 0) { + flag("3b", sprintf("dictionary marks link-owned but fresh ships it: %s", + paste(mislabelled, collapse = ", "))) + } + for (b in bundles) { - pf_csv <- sprintf("inst/extdata/configs/%s/parameters_fresh.csv", b) + pf_csv <- repo_path(sprintf("inst/extdata/configs/%s/parameters_fresh.csv", b)) cols_link <- names(utils::read.csv(pf_csv, stringsAsFactors = FALSE, check.names = FALSE, nrows = 1)) in_fresh_not_link <- setdiff(cols_fresh, cols_link) extra_link <- setdiff(cols_link, cols_fresh) - link_extensions <- extra_link[grepl("^observation_", extra_link)] - unexpected_link <- setdiff(extra_link, link_extensions) + link_extensions <- intersect(extra_link, owned_link) + unexpected_link <- setdiff(extra_link, owned_link) + undocumented <- setdiff(cols_link, dict_pf$column) cat(sprintf("\n bundle: %s\n", b)) if (length(link_extensions) > 0) { @@ -176,11 +228,20 @@ if (!nzchar(pf_fresh_path)) { } if (length(unexpected_link) > 0) { flag(sprintf("3b %s", b), - sprintf("link ∖ fresh (unexpected non-observation col): %s", + sprintf("link ∖ fresh (col not declared link-owned in dictionary): %s", paste(unexpected_link, collapse = ", "))) } - if (length(in_fresh_not_link) == 0 && length(unexpected_link) == 0) { - cat(" column set aligned (link = fresh + observation_* extensions)\n") + # Coverage: a column nobody documented must not pass silently. + if (length(undocumented) > 0) { + flag(sprintf("3b %s", b), + sprintf("undocumented in dictionary_parameters_fresh.csv: %s", + paste(undocumented, collapse = ", "))) + } + aligned <- length(in_fresh_not_link) == 0 && + length(unexpected_link) == 0 && + length(undocumented) == 0 + if (aligned) { + cat(" column set aligned + fully documented\n") } } } @@ -190,7 +251,7 @@ if (!nzchar(pf_fresh_path)) { # --------------------------------------------------------------------------- cat("\n--- 4. Override files on disk vs declared in config.yaml ---\n") for (b in bundles) { - cfg_path <- sprintf("inst/extdata/configs/%s/config.yaml", b) + cfg_path <- repo_path(sprintf("inst/extdata/configs/%s/config.yaml", b)) bundle_dir <- dirname(cfg_path) cfg <- yaml::read_yaml(cfg_path) # Declared paths are bundle-relative (e.g. "parameters_fresh.csv" at root, @@ -249,8 +310,11 @@ for (b in bundles) { cat("\n--- 6. Legacy top-level parameters_habitat_* files ---\n") for (f in c("inst/extdata/parameters_habitat_dimensions.csv", "inst/extdata/parameters_habitat_rules.yaml")) { - if (file.exists(f)) { - cat(sprintf(" %s (mtime: %s)\n", f, format(file.info(f)$mtime, "%Y-%m-%d"))) + # Resolve against the repo root, but report the repo-relative name. + full <- repo_path(f) + if (file.exists(full)) { + cat(sprintf(" %s (mtime: %s)\n", f, + format(file.info(full)$mtime, "%Y-%m-%d"))) } } cat(" Note: these were the pre-bundle predecessors. Per CLAUDE.md they map to\n") diff --git a/inst/extdata/configs/dimensions_columns.csv b/inst/extdata/configs/dictionary_dimensions.csv similarity index 100% rename from inst/extdata/configs/dimensions_columns.csv rename to inst/extdata/configs/dictionary_dimensions.csv diff --git a/inst/extdata/configs/dictionary_parameters_fresh.csv b/inst/extdata/configs/dictionary_parameters_fresh.csv new file mode 100644 index 00000000..340bfe16 --- /dev/null +++ b/inst/extdata/configs/dictionary_parameters_fresh.csv @@ -0,0 +1,20 @@ +column,type,group,owner,consumed_by,default_when_absent,description,related +species_code,character,key,fresh,"fresh/R/frs_habitat_classify.R:165; fresh/R/frs_habitat.R:1160; R/lnk_barrier_overrides.R:140",required,"Species code matching the bundle's dimensions.csv and fresh's parameters_habitat_thresholds.csv. Row key - every consumer subsets parameters_fresh to a single row by this value before reading any other column.",- +access_gradient_max,numeric,access,fresh,"R/lnk_barriers_unify.R:139; R/lnk_pipeline_prepare.R:553; fresh/R/frs_habitat_classify.R:169",required,"Maximum stream gradient the species can ascend, as a proportion (0.25 = 25%). Link derives per-species gradient-barrier membership from it - a gradient class blocks species s when class_value >= s access_gradient_max. Fresh uses it as the access threshold when segmenting. The one column both packages read directly.",- +spawn_gradient_min,numeric,gradient,fresh,"fresh/R/frs_habitat_classify.R:171; fresh/R/frs_habitat_predicates.R:87",0,"Lower gradient bound for spawning habitat - excludes depositional reaches. The default bundle sets 0.0025; the bcfishpass bundle uses 0 (no lower bound), which is one of the documented method departures between the two.",- +rear_gradient_min,numeric,gradient,fresh,(none - unused),not read,"UNUSED. Present in fresh's canonical header and in every link bundle, but read by no code in either package as of link v0.44.2 / fresh v0.32.0. Retained rather than dropped because fresh owns this schema - removal is a fresh-side decision, not link's.",- +cluster_rearing,logical,cluster,fresh,"fresh/R/frs_habitat.R:1164 (link passes the frame at R/lnk_pipeline_connect.R:107)",FALSE,"Enables rearing-cluster connectivity validation. When TRUE fresh calls frs_cluster() with label_cluster=rearing and label_connect=spawning: adjacent rearing segments are grouped, and any cluster with no spawning reachable in the configured direction has rearing set FALSE. A headwater reach with perfect rearing conditions but no reachable spawning is a dead end.",- +cluster_direction,enum:upstream/downstream/both,cluster,fresh,"fresh/R/frs_habitat.R:1165 (link passes the frame at R/lnk_pipeline_connect.R:107)",upstream,"Direction from a rearing cluster in which spawning must exist for the cluster to survive. Only honoured when cluster_rearing is TRUE.",- +cluster_bridge_gradient,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1166 (link passes the frame at R/lnk_pipeline_connect.R:107)",0.05,"Maximum gradient on any single segment between a rearing cluster and the spawning it connects to. Applied segment-by-segment along the downstream trace; the upstream check is boolean with no gradient constraint, because the branching network makes path ordering unreliable.",- +cluster_bridge_distance,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1167 (link passes the frame at R/lnk_pipeline_connect.R:107)",10000,"Maximum cumulative network distance in metres searched downstream for connecting spawning habitat.",- +cluster_confluence_m,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1168 (link passes the frame at R/lnk_pipeline_connect.R:107)",10,"Confluence tolerance in metres. When a cluster's most-downstream point falls within this distance of a confluence, the upstream check also considers the parent stream.",- +cluster_spawning,logical,cluster,fresh,"fresh/R/frs_habitat.R:1183 (link passes the frame at R/lnk_pipeline_connect.R:107)",FALSE,"Enables spawn-cluster connectivity validation. Two conditions must both hold: this flag TRUE, and the species' spawn rules carrying requires_connected in rules.yaml. Used by SK and KO, where spawning is credited only within reach of rearing lakes.",- +cluster_spawn_direction,enum:upstream/downstream/both,cluster,fresh,"fresh/R/frs_habitat.R:1228 (link passes the frame at R/lnk_pipeline_connect.R:107)",both,"Direction from a spawn cluster in which the connected target (rearing) must exist. Only honoured when cluster_spawning fires.",- +cluster_spawn_bridge_gradient,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1195 (link passes the frame at R/lnk_pipeline_connect.R:107)",0.05,"Per-segment gradient cap on the trace between spawning and the rearing it requires.",- +cluster_spawn_bridge_distance,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1197 (link passes the frame at R/lnk_pipeline_connect.R:107)",3000,"Maximum network distance in metres between spawning and its required rearing. OVERRIDDEN when the spawn rule carries connected_distance_max in rules.yaml - the YAML value wins and this column is not consulted. Only reached as the fallback.",- +cluster_spawn_confluence_m,numeric,cluster,fresh,"fresh/R/frs_habitat.R:1230 (link passes the frame at R/lnk_pipeline_connect.R:107)",10,"Confluence tolerance in metres for the spawn-side cluster check. Same semantics as cluster_confluence_m, applied to the spawning cluster.",- +observation_threshold,positive_integer,observation,link,R/lnk_barrier_overrides.R:141,no override (0),"Minimum count of qualifying fish observations upstream of a barrier before that barrier is overridden - written to the barrier-skip table that fresh consults during access gating. Fish were seen up there, so the barrier evidently is not one. 1 for residents (BT, WCT), 5 for anadromous.",- +observation_date_min,date,observation,link,R/lnk_barrier_overrides.R:143,1900-01-01,"Earliest observation_date that counts toward the threshold. Anadromous species use 1990-01-01 to exclude historic records predating current barrier configurations; WCT uses 1900-01-01 (effectively no cutoff).",- +observation_buffer_m,numeric,observation,link,R/lnk_barrier_overrides.R:145,20,"Tolerance in metres passed to fwa_upstream() when testing whether an observation lies upstream of a barrier position.",- +observation_species,semicolon_list,observation,link,R/lnk_barrier_overrides.R:147,the row's own species_code,"Semicolon-separated observation species codes counting toward this species' threshold. Lets a group pool sightings - the anadromous rows share CH;CM;CO;PK;SK, so a Coho sighting can override a barrier for Chinook. Residents list only themselves (WCT) or a close pair (BT uses BT;DV).",- +observation_control_apply,yes_no,observation,link,R/lnk_barrier_overrides.R:156,FALSE,"Whether the barrier-control table can veto an observation-based override for this species. When TRUE a user_barriers_definite_control row with barrier_ind=TRUE blocks the override at that position. TRUE for anadromous (CH CM CO PK SK ST GR KO); FALSE for residents (BT WCT) which routinely inhabit reaches above anadromous-blocking falls - post-glacial headwater connectivity, no ocean-return requirement. Habitat confirmations bypass control entirely for every species. Mirrors bcfishpass per-model SQL: model_access_bt.sql has no control join.",fresh#129 diff --git a/planning/archive/2026-07-issue-233-config-dictionaries/.gitkeep b/planning/archive/2026-07-issue-233-config-dictionaries/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/planning/archive/2026-07-issue-233-config-dictionaries/README.md b/planning/archive/2026-07-issue-233-config-dictionaries/README.md new file mode 100644 index 00000000..89df131e --- /dev/null +++ b/planning/archive/2026-07-issue-233-config-dictionaries/README.md @@ -0,0 +1,15 @@ +# #233 — config data dictionaries + the fresh↔link ownership boundary + +## Outcome + +Renamed `inst/extdata/configs/dimensions_columns.csv` → `dictionary_dimensions.csv` (a pure rename — nothing in the codebase read it; the only live reference was the #75 entry in `CLAUDE.md`) and added the missing sibling `dictionary_parameters_fresh.csv`, documenting all 19 `parameters_fresh.csv` columns with type, group, `owner`, `consumed_by`, default-when-absent and description. Every `consumed_by` is a machine-verified `file:line` rather than an inferred one, which is what surfaced the two findings that made this more than a docs chore: **link never reads the nine `cluster_*` columns at all** — it only passes the frame through at `R/lnk_pipeline_connect.R:107` to fresh's `.frs_run_connectivity()`, so the issue body's "→ `lnk_pipeline_connect.R`" was wrong — and **`rear_gradient_min` is read by no code in either package**, recorded as unused rather than dropped because fresh owns that schema. + +The real point of the issue was that the fresh↔link column-ownership boundary kept getting re-derived from scratch. It had in fact been settled long ago by [fresh#129](https://github.com/NewGraphEnvironment/fresh/issues/129) (shipped fresh 0.12.7), which *removed* `observation_*` from fresh after fresh#69 had added them, on the rule "fish passage interpretation belongs in link, not the network engine" — but that decision was only findable by archaeology through two repos' planning archives. It is now encoded as the `owner` column (14 fresh-owned engine params, 5 link-owned `observation_*`), read by `data-raw/audit_configs.R` §3b in place of a hardcoded `grepl("^observation_", ...)`, and written up in `RUNBOOK.md` §7 under "Who owns which `parameters_fresh` column". Adding a link-owned column is now a dictionary edit, not a regex edit. + +Guarding it needed two layers, because `data-raw/` is `.Rbuildignore`d and the audit never runs for an installed package: the audit gained coverage, reverse-consistency and missing-dictionary checks (negative-tested — dropping one row makes it exit 1), and `tests/testthat/test-dictionaries.R` (+23 tests) became the durable CI-side guard. Worth remembering for anything that iterates the bundles: **they carry different column subsets** — bcfishpass's `dimensions.csv` has 30 columns to the three `default*` bundles' 32 — so coverage must be asserted against the union, not against any single bundle. + +Two incidental notes. Local `main` was 38 commits stale at branch time and the pull was blocked by 96 untracked `data-raw/logs/` files upstream had since committed; git's error list is truncated, so the full collision set had to be computed via `git ls-files --others` ∩ `git ls-tree origin/main` rather than parsed from the message. Those files were backed up byte-verified to `data-raw/logs/_local_pre_pull_20260729/`. Separately, the suite carries one unrelated pre-existing failure throughout — `test-lnk_wsg_resolve.R:138` needs `public.wsg_outlet`, whose builder is open follow-up #227. + +Left deliberately out of scope: #75's auto-generated README and `lnk_rules_build()` validation (this unblocks it by giving both files a consistent shape), the fate of `rear_gradient_min` (a fresh-side call), and `audit_configs.R`'s hardcoded `setwd("/Users/airvine/Projects/repo/link")` at line 14 — a genuine portability defect noticed while editing §3b, worth its own issue. + +Closed by: commits `70c694f` → `44c3f9b` (v0.44.3) on `233-rename-dimensions-columns-csv-dictionary` diff --git a/planning/archive/2026-07-issue-233-config-dictionaries/findings.md b/planning/archive/2026-07-issue-233-config-dictionaries/findings.md new file mode 100644 index 00000000..96e4422f --- /dev/null +++ b/planning/archive/2026-07-issue-233-config-dictionaries/findings.md @@ -0,0 +1,161 @@ +# Findings — Rename dimensions_columns.csv → dictionary_dimensions.csv; add dictionary_parameters_fresh.csv (#233) + +## Issue context + +Two config CSVs drive the pipeline's methodology. Only one has a data dictionary, and it's named for its shape (`_columns`) rather than for what it is. + +## Rename + +`inst/extdata/configs/dimensions_columns.csv` → `inst/extdata/configs/dictionary_dimensions.csv` + +Establishes a `dictionary_` convention so the new sibling reads naturally. + +A full-repo sweep found **no code reference** — no `system.file()` call, no `.Rd`, no test, no workflow. The repair surface is one line: + +- `CLAUDE.md:238` (the #75 entry) — update +- `NEWS.md:409` — historical changelog, accurate at the time, leave +- `planning/archive/2026-05-issue-45-gradient-classes/findings.md:108` — archive, leave + +`lnk_config()` resolves bundles via `dir.exists()` (`R/lnk_config.R:280`), so loose CSVs at the `configs/` root are inert. Adding a second one is safe. + +## New file + +`inst/extdata/configs/dictionary_parameters_fresh.csv` — one row per column of `parameters_fresh.csv` (19 columns). + +Today those semantics are scattered: `RUNBOOK.md` §7 covers `access_gradient_max` and `observation_*` as families; roxygen on `lnk_barrier_overrides()` covers the observation block only; the 9 `cluster_*` columns are undocumented outside `R/lnk_pipeline_connect.R`. + +Proposed schema — mirrors `dictionary_dimensions.csv`, with `emits` swapped for `consumed_by` (nothing is generated from this CSV) plus `owner`: + +``` +column,type,group,owner,consumed_by,default_when_absent,description,related +``` + +Groups: `key` / `access` / `gradient` / `cluster` / `observation`. + +### `owner` is not a new decision + +It makes declarative a partition that is already settled and already enforced. + +Settled by NewGraphEnvironment/fresh#129 (shipped fresh 0.12.7). Its findings state the rule: + +> The observation override from #69 put fish passage interpretation (counting, thresholds, species grouping, date filters) in fresh. That belongs in link. + +`barrier_overrides` replaced `observations` on `frs_habitat()`, and the `observation_*` columns were removed from fresh's `parameters_fresh.csv`. fresh's NEWS: *"fish passage interpretation belongs in link, not the network engine."* + +Enforced by `data-raw/audit_configs.R:138-186`, section *"3b. parameters_fresh column drift (fresh canonical vs link config)"*: + +```r +# fresh owns the access/cluster parameter SCHEMA; link hand-authors per-bundle +# copies seeded from it plus link-only `observation_*` extensions. Values +# legitimately diverge (link tunes them) — only the COLUMN SET matters here. +``` + +| condition | verdict | +|---|---| +| `fresh \ link` | FLAG — link missing an engine param, may not load through `frs_habitat()` | +| `link \ fresh`, not `observation_*` | FLAG — unexpected column | +| `link \ fresh`, matches `^observation_` | expected extension, printed not flagged | +| values differ | fine by design — link tunes them | + +So: **fresh owns the 14 access/cluster engine columns; link owns the 5 `observation_*` interpretation columns.** Only the column set is contractual. + +The same comment records the two-way directionality: `rules.yaml` flows link → fresh (link owns the generator, `lnk_rules_build()`); `parameters_fresh` column schema flows fresh → link. Cross-referenced to #129. + +### Consumers to encode + +- `access_gradient_max` → `R/lnk_barriers_unify.R`, `R/lnk_pipeline_prepare.R` +- `cluster_*` (9 columns) → `R/lnk_pipeline_connect.R` +- `observation_*` (5 columns) → `R/lnk_barrier_overrides.R` +- `spawn_gradient_min` → fresh: `R/frs_habitat_classify.R:171`, `R/frs_habitat_predicates.R:87` +- `rear_gradient_min` → **no consumer in either package** — header-only in both `fresh/inst/extdata/parameters_fresh.csv` and every link bundle. It is fresh-owned, so whether it gets dropped is a fresh-side question. Record it here as unused; do not drop it as part of this issue. + +## Make the dictionary load-bearing + +`audit_configs.R` §3b currently hardcodes ownership as `grepl("^observation_", extra_link)`. Once the dictionary carries `owner`, §3b should read the dictionary instead of prefix-matching. That makes the dictionary checked on every audit run rather than drift-prone — the same ambition #75 has for the dimensions side. + +## Stop re-deriving this + +Add an ownership row to `RUNBOOK.md` §7 "Where every rule lives", pointing at NewGraphEnvironment/fresh#129 and `audit_configs.R` §3b. The fresh#69 → fresh#129 boundary has been worked out at length before and is currently only findable by archaeology through two repos' planning archives. + +## Scope + +- [ ] `git mv` the dimensions dictionary; repair `CLAUDE.md:238` +- [ ] Author `dictionary_parameters_fresh.csv` (19 rows), tracing each column to its consumer rather than inferring +- [ ] Rewire `audit_configs.R` §3b to read `owner` from the dictionary +- [ ] `RUNBOOK.md` §7 ownership row + +## Out of scope + +- Auto-generated README + `lnk_rules_build()` validation against the dictionary — stays in #75, which this unblocks by giving it two files in a consistent shape. +- Dropping or relocating `rear_gradient_min`. +- Any change to fresh's own `parameters_fresh.csv`. + +## Verification + +- `grep -rn "dimensions_columns" . --exclude-dir=.git` returns nothing outside `NEWS.md` and `planning/archive/` +- `Rscript data-raw/audit_configs.R` runs clean, with §3b sourcing ownership from the dictionary +- Every `consumed_by` entry in the new dictionary resolves to a real file:line +- `devtools::test()` passes (no test currently touches either file — confirms the rename is inert) + + +--- + +## Plan-mode exploration (2026-07-29) + +### Reference sweep — the rename is inert + +`grep -rn "dimensions_columns" . --exclude-dir=.git` returns three hits, all prose. No `system.file()` call, no `.Rd`, no test, no workflow reads the file. + +| Hit | Action | +|---|---| +| `CLAUDE.md:256` (#75 entry) | repair — the only live reference | +| `NEWS.md:421` (v0.17-era changelog) | leave — historical, accurate at the time | +| `planning/archive/2026-05-issue-45-gradient-classes/findings.md:108` | leave — archive | + +`lnk_config()` resolves bundles via `dir.exists()` (`R/lnk_config.R:280`), so loose CSVs at the `configs/` root are ignored. Adding a second one is safe. + +### Ownership — already settled, do not re-derive + +The fresh↔link `parameters_fresh` column boundary was worked out at length and is recorded in two places: + +1. **The decision** — `fresh/planning/archive/2026-04-issue-129-barrier-overrides/findings.md:3`: + > The observation override from #69 put fish passage interpretation (counting, thresholds, species grouping, date filters) in fresh. That belongs in link. + + Shipped fresh 0.12.7 — `barrier_overrides` replaced `observations`, `observation_*` columns deleted from fresh's CSV. fresh NEWS: *"fish passage interpretation belongs in link, not the network engine."* Arc is fresh#69 (added them) → fresh#129 (removed them). + +2. **The enforcement** — `data-raw/audit_configs.R:140-186` §3b: + ```r + # fresh owns the access/cluster parameter SCHEMA; link hand-authors per-bundle + # copies seeded from it plus link-only `observation_*` extensions. Values + # legitimately diverge (link tunes them) — only the COLUMN SET matters here. + ``` + | condition | verdict | + |---|---| + | `fresh \ link` | FLAG — link missing an engine param | + | `link \ fresh`, not `observation_*` | FLAG — unexpected column | + | `link \ fresh`, matches `^observation_` | expected extension | + | values differ | fine by design | + +Same comment records the two-way directionality: `rules.yaml` flows link → fresh (link owns `lnk_rules_build()`); `parameters_fresh` column schema flows fresh → link. Cross-referenced to link#129. + +### Consumer traces (verified on HEAD b9f6285) + +- `access_gradient_max` → `R/lnk_barriers_unify.R:139`, `R/lnk_pipeline_prepare.R:553` — **plus** fresh. Dual-consumed. +- `spawn_gradient_min` → `fresh/R/frs_habitat_classify.R:171`, `fresh/R/frs_habitat_predicates.R:87` +- `cluster_*` (9) → read in **fresh** at `fresh/R/frs_habitat.R:1164-1196`. link only passes the frame through (`R/lnk_pipeline_connect.R:107` → `.frs_run_connectivity()`). The issue body's "→ `lnk_pipeline_connect.R`" is imprecise. +- `observation_*` (5) → `R/lnk_barrier_overrides.R` +- `rear_gradient_min` → **no reader in either package.** Header-only in `fresh/inst/extdata/parameters_fresh.csv` and every link bundle. fresh-owned, so dropping it is a fresh-side call. + +Cluster semantics prose source: `fresh/R/frs_cluster.R` roxygen documents `direction`, `bridge_gradient`, `bridge_distance`, `confluence_m` in full. + +### Environment notes + +- `data-raw/` is `.Rbuildignore`d — `audit_configs.R` is dev-only. `inst/` ships, so testthat is the durable guard for the dictionary contract; the audit is the pre-trifecta gate. +- `audit_configs.R` hardcodes `setwd("/Users/airvine/Projects/repo/link")` (line 14). Portability defect, out of scope for #233, worth its own issue. +- fresh is Suggests (>= 0.32.0). fresh repo pulled to v0.32.0 during this session; installed R package may still be 0.31.0 — reinstall before relying on cross-package tests. + +### Repo state at branch time + +Local `main` was 38 commits behind `origin/main`; the pull was blocked by 96 untracked `data-raw/logs/` files that upstream had since committed (65 in the first collision set, 31 more that git's truncated error had hidden). All 96 were copied to `data-raw/logs/_local_pre_pull_20260729/` (byte-identity verified, 384K) before removal, then `main` fast-forwarded to `b9f6285` / v0.44.2. Of the originals, 40 were byte-identical to upstream; the rest were divergent local run outputs from 2026-05-13/14/15 preserved in that backup. + +Plan targets were checked against the 38 commits: `dimensions_columns.csv`, `data-raw/audit_configs.R`, and `RUNBOOK.md` are all untouched upstream. Adjustments carried into the plan: `CLAUDE.md` ref moved `:238` → `:256`; version bump target `0.43.1` → `0.44.3`. diff --git a/planning/archive/2026-07-issue-233-config-dictionaries/progress.md b/planning/archive/2026-07-issue-233-config-dictionaries/progress.md new file mode 100644 index 00000000..c95b2afe --- /dev/null +++ b/planning/archive/2026-07-issue-233-config-dictionaries/progress.md @@ -0,0 +1,39 @@ +# Progress — Rename dimensions_columns.csv → dictionary_dimensions.csv; add dictionary_parameters_fresh.csv (#233) + +## Session 2026-07-29 + +- Filed #233 after tracing the `observation_control_apply` question back to the fresh↔link column-ownership boundary +- Plan-mode exploration — phases approved by user +- Recovered stale local `main` (38 commits behind): backed up 96 colliding untracked `data-raw/logs/` files to `data-raw/logs/_local_pre_pull_20260729/` (byte-identity verified) before fast-forwarding to `b9f6285` / v0.44.2 +- Re-verified all plan targets against the 38 upstream commits — `dimensions_columns.csv`, `audit_configs.R`, `RUNBOOK.md` untouched. Adjusted `CLAUDE.md:238` → `:256` and bump target `0.43.1` → `0.44.3` +- Created branch `233-rename-dimensions-columns-csv-dictionary` off updated main +- Scaffolded PWF baseline from issue #233 with approved phases +- Next: Phase 1 — `git mv` the dictionary + repair `CLAUDE.md:256` + +## Session 2026-07-30 + +- **Phase 1 complete** — `git mv` to `dictionary_dimensions.csv`, `CLAUDE.md:256` repaired +- Sweep verified: only `NEWS.md:421` + archived findings retain the old name (both historical by design). No indirect/glob reference — `audit_configs.R:205` globs only `overrides/`, never the configs root +- Installed `fresh` v0.32.0 from local repo (was 0.31.0, below link's `fresh (>= 0.32.0)` pin) — cleared a spurious `frs_wsg_drainage` export error +- Test suite: 1294 PASS, 1 FAIL. The failure (`test-lnk_wsg_resolve.R:138`) is missing DB table `public.wsg_outlet`, whose builder is open follow-up #227 — pre-existing, unrelated to the rename +- Next: Phase 2 — write the failing dictionary/schema contract test +- **Phase 2 complete** — `tests/testthat/test-dictionaries.R` written as the failing contract: 6 FAIL / 9 PASS. All 6 failures are `dictionary_parameters_fresh.csv` assertions (file lands in Phase 3); the dimensions side already passes +- Discovered bundles carry different column subsets — bcfishpass `dimensions.csv` has 30 columns, the three `default*` bundles have 32. Coverage is therefore asserted against the union across bundles, with a per-bundle subset check. Existing `dictionary_dimensions.csv` (32 rows) already covers the union exactly, no orphans +- Confirmed the ownership partition is exact: fresh canonical = 14 columns, link bundles = those 14 + exactly the 5 `observation_*`, zero fresh-not-link gap +- Self-review caught two precision defects in the new test before commit: `nzchar(NA)` is `TRUE` so bare non-empty checks would wave through an NA cell (added `filled()`), and the owner assertion used `expect_setequal` where a domain check was meant (conflated two failure modes) +- `lintr::lint()` clean on the new file +- Next: Phase 3 — author `dictionary_parameters_fresh.csv` (19 rows) and turn the 6 red assertions green +- **Phase 3 complete** — `inst/extdata/configs/dictionary_parameters_fresh.csv` authored, 19 rows. Contract tests 23 PASS / 0 FAIL; full suite 1317 PASS (up 23), same lone pre-existing #227 failure +- Every `consumed_by` reference machine-verified against the actual source line via a throwaway resolver script — 24/24 resolve. Two were off by 1-2 lines in the first draft (`frs_habitat.R:1159` was blank; `frs_habitat_classify.R:168` pointed at `species_code` not `access_gradient_max`) and were corrected rather than left approximate +- Ownership recorded as fact, not inference: 14 fresh-owned, 5 link-owned (`observation_*`) +- Notable content captured: `cluster_spawn_bridge_distance` is overridden by `connected_distance_max` in rules.yaml when present (the YAML wins, the column is only a fallback); `rear_gradient_min` is the sole orphan of the 19 +- Next: Phase 4 — rewire `audit_configs.R` section 3b to read `owner` from the dictionary +- **Phase 4 complete** — `audit_configs.R` section 3b now reads ownership from `dictionary_parameters_fresh.csv` instead of prefix-matching `^observation_`. Adding a link-owned column is now a dictionary edit, not a regex edit +- Three guards where there was one: undocumented-column coverage, a reverse check (dictionary claims link-owned but fresh ships it = stale dictionary), and a missing/malformed-dictionary guard that flags instead of erroring mid-run +- Negative-tested: dropping one dictionary row makes the audit exit 1 and fire both flags; restored byte-identical to HEAD. A guard that has never failed is not a verified guard +- Lint parity held (13 before, 13 after) after restructuring an alignment check that had added one `indentation_linter` +- Next: Phase 5 — RUNBOOK section 7 ownership row, NEWS, version bump +- **Phase 5 complete** — `RUNBOOK.md` section 7 gains a dictionary row plus a "Who owns which parameters_fresh column" subsection: the 14/5 split, fresh#129 as the deciding issue, both enforcement points, and the opposite directions of travel (`rules.yaml` link->fresh, column schema fresh->link). `rear_gradient_min` added to the "gaps worth knowing" list (now three) +- `NEWS.md` entry written; version bumped 0.44.2 -> 0.44.3 as the final commit +- Final validation: 1317 PASS / 1 pre-existing FAIL (#227), audit exits 0 with no findings, both touched files lint-clean, old-name sweep clean +- Next: `/planning-archive`, then `/gh-pr-push` diff --git a/planning/archive/2026-07-issue-233-config-dictionaries/task_plan.md b/planning/archive/2026-07-issue-233-config-dictionaries/task_plan.md new file mode 100644 index 00000000..32740693 --- /dev/null +++ b/planning/archive/2026-07-issue-233-config-dictionaries/task_plan.md @@ -0,0 +1,77 @@ +# Task: Rename dimensions_columns.csv → dictionary_dimensions.csv; add dictionary_parameters_fresh.csv (#233) + +Two config CSVs carry the pipeline's methodology. `dimensions.csv` has a data dictionary; `parameters_fresh.csv` has none — its 19 columns are documented only in scattered prose (`RUNBOOK.md` §7 covers two families, roxygen on `lnk_barrier_overrides()` covers the observation block, the 9 `cluster_*` columns are undocumented). The existing dictionary is also named for its shape (`dimensions_columns.csv`) rather than what it is. + +The deeper problem this fixes: **the fresh↔link column-ownership boundary keeps getting re-derived.** It was settled by NewGraphEnvironment/fresh#129 (shipped fresh 0.12.7 — `observation_*` removed from fresh because "fish passage interpretation belongs in link, not the network engine") and is enforced today by `data-raw/audit_configs.R` §3b. That decision is currently findable only by archaeology through two repos' planning archives. Encoding it as an `owner` column, and making the audit read it, ends the re-derivation. + +## Correction to the issue body + +The issue says `cluster_*` (9 columns) → `R/lnk_pipeline_connect.R`. That is imprecise and the implementation must not copy it. link only **passes** `loaded$parameters_fresh` through (`R/lnk_pipeline_connect.R:107` → `.frs_run_connectivity()`); the columns are actually **read in fresh** at `fresh/R/frs_habitat.R:1164-1196`. `access_gradient_max` is genuinely dual-consumed (link `R/lnk_barriers_unify.R:139` + `R/lnk_pipeline_prepare.R:553`, and fresh). So `consumed_by` must distinguish reader from pass-through, traced per column rather than assumed. + +## Phase 1: Rename + reference repair + +- [x] `git mv inst/extdata/configs/dimensions_columns.csv inst/extdata/configs/dictionary_dimensions.csv` +- [x] Update `CLAUDE.md:256` (#75 entry) to the new filename +- [x] Confirm sweep clean: `grep -rn "dimensions_columns" . --exclude-dir=.git` returns only `NEWS.md:421` + `planning/archive/2026-05-issue-45-gradient-classes/findings.md:108` (both historical, intentionally untouched) +- [x] `devtools::test()` — confirms the rename is inert (`lnk_config()` resolves bundles via `dir.exists()`, `R/lnk_config.R:280`; nothing reads the dictionary). 1294 PASS; 1 pre-existing FAIL (`test-lnk_wsg_resolve.R:138`) from missing `public.wsg_outlet` DB table — builder is open follow-up #227, unrelated to this rename +- [x] Verified no indirect reference: the only `list.files()` over a bundle dir is `data-raw/audit_configs.R:205`, scoped to `overrides/`, never the configs root. No hits in `_pkgdown.yml`, `.github/`, `vignettes/`, `man/`, `NAMESPACE` + +## Phase 2: Test first — dictionary/schema contract + +- [x] `tests/testthat/test-dictionaries.R`, using `system.file("extdata", "configs", ...)` (`inst/` ships; `data-raw/` is `.Rbuildignore`d, so testthat is the durable guard — the audit script is dev-only) +- [x] Assert: every column in each bundle's `parameters_fresh.csv` has exactly one dictionary row, and every dictionary row names a real column +- [x] Assert: `owner` ∈ {`fresh`, `link`}; the `link`-owned set is exactly the 5 `observation_*` columns +- [x] Assert: `dictionary_dimensions.csv` covers every column of each bundle's `dimensions.csv` +- [x] `skip_if_not_installed("fresh")` on the cross-package assertion — fresh is Suggests (>= 0.32.0) +- [x] Tests fail at this point (no dictionary yet). That is the contract. **6 FAIL / 9 PASS** — every `parameters_fresh` assertion red, dimensions side already green +- [x] Bundles carry *different* column subsets (bcfishpass `dimensions.csv` = 30 cols, defaults = 32), so coverage is asserted against the **union** across bundles, not any single bundle + +## Phase 3: Author dictionary_parameters_fresh.csv + +- [x] `inst/extdata/configs/dictionary_parameters_fresh.csv`, 19 rows, schema `column,type,group,owner,consumed_by,default_when_absent,description,related` (mirrors `dictionary_dimensions.csv` with `emits` → `consumed_by`, plus `owner`) +- [x] Groups: `key` / `access` / `gradient` / `cluster` / `observation` +- [x] Trace every `consumed_by` to a real `file:line`, distinguishing reader from pass-through. **All 24 refs machine-verified to resolve to the exact intended line** (two were off by 1–2 lines on first draft and were corrected, not left approximate). Verified refs on current HEAD: + - `access_gradient_max` → `R/lnk_barriers_unify.R:139`, `R/lnk_pipeline_prepare.R:553` (+ fresh) + - `spawn_gradient_min` → `fresh/R/frs_habitat_classify.R:171`, `fresh/R/frs_habitat_predicates.R:87` + - `cluster_*` → `fresh/R/frs_habitat.R:1164-1196`; link pass-through at `R/lnk_pipeline_connect.R:107` + - cluster semantics prose → `fresh/R/frs_cluster.R` roxygen (documents `direction` / `bridge_gradient` / `bridge_distance` / `confluence_m` in full) + - `observation_*` → `R/lnk_barrier_overrides.R` +- [x] Record `rear_gradient_min` as unused — header-only in both packages, zero readers. fresh-owned, so its fate is a fresh-side call; do not drop it here +- [x] Confirmed the spawn-side cluster columns ARE consumed (`cluster_spawn_direction` at `fresh/R/frs_habitat.R:1228`, `cluster_spawn_confluence_m` at `:1230`) — `rear_gradient_min` is the only genuine orphan of the 19 +- [x] Phase 2 tests now pass — 23 PASS / 0 FAIL on the dictionary contract; full suite 1317 PASS + +## Phase 4: Make the dictionary load-bearing in the audit + +- [x] `data-raw/audit_configs.R` §3b: replaced the hardcoded `grepl("^observation_", extra_link)` ownership rule with a lookup against the dictionary's `owner` column +- [x] Add a dictionary-coverage check so an undocumented new column flags via `flag()` rather than passing silently +- [x] Added a reverse consistency check: a column the dictionary calls link-owned that fresh actually ships is a stale dictionary, and flags +- [x] Guard against the dictionary itself being missing/malformed (flags rather than erroring out mid-audit) +- [x] Preserve existing semantics: `flag()` accumulator, end-of-run rollup, non-zero exit +- [x] `Rscript data-raw/audit_configs.R` reports "No findings — config layers aligned." and exits 0 +- [x] **Negative-tested the guard** — removing one dictionary row makes the audit exit 1 and fire both `undocumented` and `not declared link-owned` flags; file restored byte-identical to HEAD afterwards +- [x] No new lints introduced (13 before, 13 after — the first draft added one `indentation_linter` at the alignment check, since restructured) + +## Phase 5: Documentation + +- [x] `RUNBOOK.md` §7 "Where every rule lives": new dictionary row + a full **"Who owns which `parameters_fresh` column"** subsection naming fresh#129, the 14/5 split, both enforcement points, and the opposite directions of travel for `rules.yaml` vs the column schema +- [x] Recorded `rear_gradient_min` in the RUNBOOK's "gaps worth knowing" (now three, was two) +- [x] `NEWS.md` entry +- [x] Version bump to 0.44.3 as the **final** commit of the branch (per CLAUDE.md release convention) + +## Validation + +- [x] `devtools::test()` — 1317 PASS, 1 pre-existing FAIL (#227 `public.wsg_outlet`, DB state, unrelated) +- [x] `lintr` clean on both touched files (`test-dictionaries.R` 0 lints; `audit_configs.R` 13 = unchanged baseline) +- [x] `Rscript data-raw/audit_configs.R` → 0 findings, exit 0 +- [x] Every `consumed_by` entry resolves to a real file:line — machine-verified, 24/24 +- [x] Tests pass +- [x] `/code-check` clean on each commit (run inline; see progress.md) +- [x] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion + +## Out of scope + +- #75's auto-generated README + `lnk_rules_build()` validation — this unblocks it by giving both files a consistent shape +- Dropping or relocating `rear_gradient_min` +- Any change to `fresh` +- `audit_configs.R`'s hardcoded `setwd("/Users/airvine/Projects/repo/link")` (line 14) — a real portability defect that `/code-check` will flag when §3b is touched, but not this issue's scope. Worth its own issue. diff --git a/tests/testthat/test-dictionaries.R b/tests/testthat/test-dictionaries.R new file mode 100644 index 00000000..aaab2954 --- /dev/null +++ b/tests/testthat/test-dictionaries.R @@ -0,0 +1,137 @@ +# Contract tests for the config data dictionaries. +# +# `data-raw/audit_configs.R` is the pre-trifecta gate, but `data-raw/` is +# .Rbuildignore'd — it never runs for an installed package or in CI. These +# tests are the durable guard: the dictionaries must stay in lockstep with +# the CSVs they describe, or a silently-undocumented column ships. +# +# The `owner` partition encoded here is not a local judgement call. It was +# settled by NewGraphEnvironment/fresh#129 (shipped fresh 0.12.7): fresh owns +# the network-engine params (access + cluster geometry); link owns the +# fish-passage interpretation params (`observation_*`), which were deleted +# from fresh's own parameters_fresh.csv because "fish passage interpretation +# belongs in link, not the network engine". + +cfg_dir <- function() system.file("extdata", "configs", package = "link") + +bundle_names <- function() basename(list.dirs(cfg_dir(), recursive = FALSE)) + +read_csv_plain <- function(path) { + utils::read.csv(path, stringsAsFactors = FALSE, check.names = FALSE) +} + +# `nzchar(NA)` is TRUE, so a bare nzchar() check would wave through an NA +# cell — exactly the half-authored row these tests exist to catch. +filled <- function(x) !is.na(x) & nzchar(trimws(x)) + +dict_read <- function(stem) { + read_csv_plain(file.path(cfg_dir(), paste0("dictionary_", stem, ".csv"))) +} + +bundle_cols <- function(bundle, stem) { + names(read_csv_plain(file.path(cfg_dir(), bundle, paste0(stem, ".csv")))) +} + +# Union of a stem's columns across every bundle. Bundles legitimately carry +# different column subsets (bcfishpass's dimensions.csv omits two columns the +# default bundles use), so the dictionary is checked against the union, not +# against any single bundle. +bundle_cols_union <- function(stem) { + unique(unlist(lapply(bundle_names(), bundle_cols, stem = stem))) +} + +# -- dictionary_dimensions --------------------------------------------------- + +test_that("dictionary_dimensions has the expected shape", { + d <- dict_read("dimensions") + + expect_true(all(c("column", "type", "group", "applies_to", + "default_when_absent", "description", "emits", + "related") %in% names(d))) + expect_false(any(duplicated(d$column))) + expect_true(all(filled(d$column))) + expect_true(all(filled(d$description))) +}) + +test_that("dictionary_dimensions covers every bundle's dimensions.csv", { + d <- dict_read("dimensions") + + for (b in bundle_names()) { + undocumented <- setdiff(bundle_cols(b, "dimensions"), d$column) + expect_identical( + undocumented, character(0), + info = sprintf("bundle %s has undocumented dimensions columns: %s", + b, paste(undocumented, collapse = ", ")) + ) + } +}) + +test_that("dictionary_dimensions has no rows for columns that do not exist", { + d <- dict_read("dimensions") + expect_setequal(d$column, bundle_cols_union("dimensions")) +}) + +# -- dictionary_parameters_fresh --------------------------------------------- + +test_that("dictionary_parameters_fresh has the expected shape", { + d <- dict_read("parameters_fresh") + + expect_true(all(c("column", "type", "group", "owner", "consumed_by", + "default_when_absent", "description", + "related") %in% names(d))) + expect_false(any(duplicated(d$column))) + expect_true(all(filled(d$column))) + expect_true(all(filled(d$description))) + expect_true(all(filled(d$consumed_by))) +}) + +test_that("dictionary_parameters_fresh covers every bundle's parameters_fresh.csv", { + d <- dict_read("parameters_fresh") + + for (b in bundle_names()) { + undocumented <- setdiff(bundle_cols(b, "parameters_fresh"), d$column) + expect_identical( + undocumented, character(0), + info = sprintf("bundle %s has undocumented parameters_fresh columns: %s", + b, paste(undocumented, collapse = ", ")) + ) + } +}) + +test_that("dictionary_parameters_fresh has no rows for columns that do not exist", { + d <- dict_read("parameters_fresh") + expect_setequal(d$column, bundle_cols_union("parameters_fresh")) +}) + +# -- the fresh <-> link ownership partition (fresh#129) ---------------------- + +test_that("every parameters_fresh column declares a valid owner", { + # Domain check only. Asserting set-equality here would conflate "no bogus + # owner value" with "both owners are in use" — the link side is pinned by + # the next test, and a failure there should not also fire here. + d <- dict_read("parameters_fresh") + expect_true(all(filled(d$owner))) + expect_true(all(d$owner %in% c("fresh", "link"))) +}) + +test_that("link owns exactly the observation_* interpretation columns", { + d <- dict_read("parameters_fresh") + + link_owned <- d$column[d$owner == "link"] + expect_setequal(link_owned, grep("^observation_", d$column, value = TRUE)) +}) + +test_that("fresh-owned columns match fresh's canonical parameters_fresh.csv", { + # Cross-package: fresh is Suggests (>= 0.32.0). The contract is that link's + # bundles are fresh's column set plus link-owned extensions — a column fresh + # adds that link is missing means link's copy may no longer load through + # frs_habitat(). Mirrors data-raw/audit_configs.R section 3b. + skip_if_not_installed("fresh") + + canonical <- system.file("extdata", "parameters_fresh.csv", package = "fresh") + skip_if(!nzchar(canonical), "fresh's bundled parameters_fresh.csv not found") + + d <- dict_read("parameters_fresh") + expect_setequal(d$column[d$owner == "fresh"], + names(read_csv_plain(canonical))) +})