From 83cdea45f4b15be97d9e8184cd37e567e60dc854 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:53:49 -0700 Subject: [PATCH 01/11] Initialize PWF baseline for #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- planning/active/findings.md | 43 ++++++++++++++++++++++++++++++ planning/active/progress.md | 8 ++++++ planning/active/task_plan.md | 51 ++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 planning/active/findings.md create mode 100644 planning/active/progress.md create mode 100644 planning/active/task_plan.md diff --git a/planning/active/findings.md b/planning/active/findings.md new file mode 100644 index 0000000..f0e9153 --- /dev/null +++ b/planning/active/findings.md @@ -0,0 +1,43 @@ +# Findings — cd_summary(): stations sharing a long_name give indistinguishable rows (#98) + +## Issue context + +**If we do it:** a `cd_summary()` table over several stations that share one `long_name` says which row is which station. **If we never do:** two rows read `Mean discharge / Annual` with different slopes and nothing to tell them apart — while `cd_plot_comparison()` beside the table labels the same series `Mean discharge (q_site1)`. + +## Problem + +`series_check()` tells callers that series from several sites need distinct `variable` names. The natural next step for streamflow is one `long_name` ("Mean discharge") on every station. `cd_summary()` (`R/cd_summary.R`) replaces `variable` with `Parameter = long_name` and drops `variable`, so those rows become indistinguishable. Measured in #93's review with `q_site1` / `q_site2`. + +#93 handled the same case in `cd_plot_comparison()`: a label shared by several variables gets ` (variable)` appended, and facets key on variable + label so nothing can merge. + +## Proposed Solution + +- In `cd_summary()`, append ` (variable)` to a `Parameter` shared by more than one variable — the same rule as `cd_plot_comparison()`, ideally lifted into one internal helper both call. +- Test: two stations, one `long_name` → two distinct `Parameter` values; registered ERA5 variables unchanged (registry long_names are unique). + +A second way to get indistinguishable rows: since #97 a trend table can hold both a raw-value and an anomaly trend of one series (`bind_rows(cd_trend(x), cd_trend(ano))`, told apart by `trend_on`). `cd_summary()` drops `trend_on`, so those rows differ only by `Unit`, and for an `absolute` variable such as tmean not even by that. Whatever disambiguates stations here should also cover `trend_on`, or `cd_summary()` should carry it. + +Found by `/code-check` on #93; the `trend_on` case by the plan review for #97. + +## Plan-gate decision (2026-09-29) + +`trend_on` disambiguation: a conditional `Trend on` column ("Value"/"Anomaly"), added only +when the table holds both scales — the `Region` precedent. Chosen over always adding the +column (changes every existing table, both vignettes' kables) and over suffixing `Parameter` +(scale ends up in a text label rather than a filterable field). + +## Exploration + +- `cd_plot_comparison()` (`R/cd_plot_comparison.R:34-39`) has the #93 rule inline: a label + shared by several variables gets ` (variable)` appended, one pass. Its residual collision + (`long_name = c("Q", "Q", "Q (a)")`) is harmless there because facets key on + variable + label. A table has no hidden key, so the lifted helper repeats until labels are + distinct per variable. +- Shared helpers live in `R/cd_anomaly.R` (`col_or_na`, `series_check`, `meta_resolve`, + `meta_check`); CLAUDE.md tells new consumers to call them. +- `cd_summary()` already reads a missing/`NA` `trend_on` as anomaly (`meta_resolve(raw = col_or_na(...) %in% "value")`). + +## Errors Encountered + +| Error | Resolution | +|-------|------------| diff --git a/planning/active/progress.md b/planning/active/progress.md new file mode 100644 index 0000000..440fe33 --- /dev/null +++ b/planning/active/progress.md @@ -0,0 +1,8 @@ +# Progress — cd_summary(): stations sharing a long_name give indistinguishable rows (#98) + +## Session 2026-09-30 + +- Plan-mode exploration — phases approved by user (conditional `Trend on` column chosen at the gate) +- Created branch `98-cd-summary-stations-sharing-a-long-name` off main +- Scaffolded PWF baseline from issue #98 with approved phases +- Next: start Phase 1 diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md new file mode 100644 index 0000000..f3e6b31 --- /dev/null +++ b/planning/active/task_plan.md @@ -0,0 +1,51 @@ +# Task: cd_summary(): stations sharing a long_name give indistinguishable rows (#98) + + +`series_check()` tells callers that series from several sites need distinct `variable` names. The natural next step for streamflow is one `long_name` ("Mean discharge") on every station. `cd_summary()` (`R/cd_summary.R`) replaces `variable` with `Parameter = long_name` and drops `variable`, so those rows become indistinguishable. Measured in #93's review with `q_site1` / `q_site2`. + +#93 handled the same case in `cd_plot_comparison()`: a label shared by several variables gets ` (variable)` appended, and facets key on variable + label so nothing can merge. + +## Phase 1: Lift the shared-label rule into one helper +- [ ] Add `label_disambiguate(variable, label)` to `R/cd_anomaly.R` beside the other series + helpers (`@noRd`): a label shared by more than one distinct variable gets + ` (variable)` appended. Repeat (bounded by the number of distinct variables) until no + label is shared by two variables, so a contrived collision (`long_name = c("Q", "Q", + "Q (a)")`) still ends distinct — a table has no hidden facet key to fall back on. +- [ ] `cd_plot_comparison()` calls the helper in place of its inline three lines; facet key + logic unchanged. Existing plot tests stay green (the collision test asserts facets only). +- [ ] Unit tests for the helper in `tests/testthat/test-cd_anomaly.R`: shared → suffixed, + unique → untouched, one variable over several periods → no suffix, the collision case → + one distinct label per variable, `NA` handling not reachable (callers coalesce first). + +## Phase 2: cd_summary() disambiguates stations +- [ ] `cd_summary()` passes `labels_param` through `label_disambiguate()`. +- [ ] Tests in `test-cd_summary.R`: two stations, one `long_name` → `"Mean discharge (q_site1)"`, + `"Mean discharge (q_site2)"`; registered ERA5 variables unchanged (registry long_names are + unique — assert `anyDuplicated(cd_variables()$long_name) == 0` so the premise is pinned); + one variable over several periods gets no suffix; labels match `cd_plot_comparison()` + for the same variables. + +## Phase 3: cd_summary() carries the scale when a table mixes them +- [ ] Scale = `coalesce(col_or_na(trend, "trend_on"), "anomaly")` (missing/NA read as anomaly, + as `cd_summary()` already does for units). When it has more than one distinct value, add + `Trend on` (`"Value"`/`"Anomaly"`) after `Period`; otherwise the output shape is unchanged. +- [ ] Tests: mixed table (`bind_rows(cd_trend(x), cd_trend(ano))` for tmean — identical Unit, + the case the issue names) → `Trend on` present with `c("Value", "Anomaly")`; single-scale + and no-`trend_on` tables → `expect_named()` unchanged; value + `NA` trend_on → column + present, NA row reads `"Anomaly"`; `region_name` still last. +- [ ] Roxygen: `@return` and description name both the ` (variable)` suffix and the conditional + `Trend on` column; `devtools::document()`. + +## Phase 4: Docs +- [ ] `CLAUDE.md` consumer-chain paragraph: the helpers in `R/cd_anomaly.R` are now four + (add `label_disambiguate`) — any new consumer that prints labels calls it. +- [ ] Full `devtools::test()`, `lintr::lint_package()`, `pkgdown::check_pkgdown()` (no new export). + +NEWS + version bump are left to `/gh-pr-merge`, per the repo workflow (0.5.4, patch). + +## Validation + +- [ ] Tests pass +- [ ] `/code-check` clean on each commit +- [ ] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion From d3cb7a90e6145ac4b931dec47a798989e867074b Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:54:54 -0700 Subject: [PATCH 02/11] Lift the shared-label rule into label_disambiguate() cd_plot_comparison()'s inline #93 rule moves to one helper in R/cd_anomaly.R so cd_summary() can call the same rule. The helper repeats until no label is shared by two variables, since a table has no hidden facet key to fall back on. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_anomaly.R | 18 +++++++++++++++++ R/cd_plot_comparison.R | 8 ++++---- planning/active/task_plan.md | 6 +++--- tests/testthat/test-cd_anomaly.R | 33 ++++++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/R/cd_anomaly.R b/R/cd_anomaly.R index 8ecd8cd..4f74684 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -185,6 +185,24 @@ meta_resolve <- function(x, raw = FALSE) { ) } +#' Make labels tell variables apart: a label shared by several variables (one +#' long_name on many stations) gets ` (variable)` appended. Repeated because a +#' suffixed label can meet another variable's own label (`c("Q", "Q", "Q (a)")`); +#' each pass lengthens only the labels still shared, and the loop is bounded by +#' the number of variables. The one place the rule lives; every consumer that +#' prints labels calls it. +#' @noRd +label_disambiguate <- function(variable, label) { + variable <- as.character(variable) + for (i in seq_along(unique(variable))) { + lab <- unique(data.frame(variable = variable, label = label)) + shared <- label %in% lab$label[duplicated(lab$label)] + if (!any(shared)) break + label[shared] <- paste0(label[shared], " (", variable[shared], ")") + } + label +} + #' Abort unless each variable/period carries one value of each metadata #' column present in `x` (NA counts as a value). `x` must be ungrouped. #' @noRd diff --git a/R/cd_plot_comparison.R b/R/cd_plot_comparison.R index 3eb9c35..2ae444d 100644 --- a/R/cd_plot_comparison.R +++ b/R/cd_plot_comparison.R @@ -34,10 +34,10 @@ cd_plot_comparison <- function(x, # Facet labels: carried long_name, then cd_variables(), then the name. # A label shared by several variables (one long_name, many stations) # carries the variable name too, so their facets read differently. - x$param <- dplyr::coalesce(meta_resolve(x)$long_name, as.character(x$variable)) - lab <- unique(data.frame(variable = as.character(x$variable), param = x$param)) - shared <- x$param %in% lab$param[duplicated(lab$param)] - x$param[shared] <- paste0(x$param[shared], " (", x$variable[shared], ")") + x$param <- label_disambiguate( + x$variable, + dplyr::coalesce(meta_resolve(x)$long_name, as.character(x$variable)) + ) # Facet on variable and label together, never the label alone, so no # label, however it collides, can put two variables in one facet. # Levels in label order, as faceting by label gave. diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index f3e6b31..87879f5 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -6,14 +6,14 @@ #93 handled the same case in `cd_plot_comparison()`: a label shared by several variables gets ` (variable)` appended, and facets key on variable + label so nothing can merge. ## Phase 1: Lift the shared-label rule into one helper -- [ ] Add `label_disambiguate(variable, label)` to `R/cd_anomaly.R` beside the other series +- [x] Add `label_disambiguate(variable, label)` to `R/cd_anomaly.R` beside the other series helpers (`@noRd`): a label shared by more than one distinct variable gets ` (variable)` appended. Repeat (bounded by the number of distinct variables) until no label is shared by two variables, so a contrived collision (`long_name = c("Q", "Q", "Q (a)")`) still ends distinct — a table has no hidden facet key to fall back on. -- [ ] `cd_plot_comparison()` calls the helper in place of its inline three lines; facet key +- [x] `cd_plot_comparison()` calls the helper in place of its inline three lines; facet key logic unchanged. Existing plot tests stay green (the collision test asserts facets only). -- [ ] Unit tests for the helper in `tests/testthat/test-cd_anomaly.R`: shared → suffixed, +- [x] Unit tests for the helper in `tests/testthat/test-cd_anomaly.R`: shared → suffixed, unique → untouched, one variable over several periods → no suffix, the collision case → one distinct label per variable, `NA` handling not reachable (callers coalesce first). diff --git a/tests/testthat/test-cd_anomaly.R b/tests/testthat/test-cd_anomaly.R index f5f0394..2f70c2f 100644 --- a/tests/testthat/test-cd_anomaly.R +++ b/tests/testthat/test-cd_anomaly.R @@ -273,3 +273,36 @@ test_that("stacked sites under one variable are refused, not pooled (#92)", { expect_error(cd_trend(x, trend_start = 2000), "More than one row") } }) + +# label_disambiguate() (#98) ------------------------------------------------ + +test_that("label_disambiguate appends the variable to a label several variables share", { + expect_identical( + label_disambiguate(c("q_site1", "q_site2", "q_site1"), rep("Mean discharge", 3)), + c("Mean discharge (q_site1)", "Mean discharge (q_site2)", "Mean discharge (q_site1)") + ) +}) + +test_that("label_disambiguate leaves unique labels and single variables alone", { + expect_identical( + label_disambiguate(c("tmean", "prcp"), c("Mean temperature", "Precipitation")), + c("Mean temperature", "Precipitation") + ) + # one variable over several periods is not a collision + expect_identical(label_disambiguate(c("tmean", "tmean"), c("T", "T")), c("T", "T")) + expect_identical(label_disambiguate(character(0), character(0)), character(0)) +}) + +test_that("label_disambiguate ends with one distinct label per variable when a suffix collides", { + out <- label_disambiguate(c("a", "b", "c", "a"), c("Q", "Q", "Q (a)", "Q")) + expect_identical(out, c("Q (a) (a)", "Q (b)", "Q (a) (c)", "Q (a) (a)")) + pairs <- unique(data.frame(v = c("a", "b", "c", "a"), l = out)) + expect_false(anyDuplicated(pairs$l) > 0) +}) + +test_that("label_disambiguate reads a factor variable by name", { + expect_identical( + label_disambiguate(factor(c("q2", "q1")), c("Q", "Q")), + c("Q (q2)", "Q (q1)") + ) +}) From 1b5d5483529bc3e44e0cb2f0e201563943cea4a2 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:55:25 -0700 Subject: [PATCH 03/11] cd_summary(): append the variable to a long_name several stations share Two stations carrying one long_name printed as identical rows once cd_summary() dropped variable. Parameter now goes through label_disambiguate(), matching cd_plot_comparison()'s labels. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_summary.R | 5 +++- planning/active/task_plan.md | 4 ++-- tests/testthat/test-cd_summary.R | 41 ++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/R/cd_summary.R b/R/cd_summary.R index a019df8..6bf9606 100644 --- a/R/cd_summary.R +++ b/R/cd_summary.R @@ -43,7 +43,10 @@ cd_summary <- function(trend, region_name = NULL) { trend <- dplyr::ungroup(trend) meta <- meta_resolve(trend, raw = col_or_na(trend, "trend_on") %in% "value") - labels_param <- dplyr::coalesce(meta$long_name, as.character(trend$variable)) + labels_param <- label_disambiguate( + trend$variable, + dplyr::coalesce(meta$long_name, as.character(trend$variable)) + ) labels_unit <- meta$unit out <- trend |> diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index 87879f5..bd45129 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -18,8 +18,8 @@ one distinct label per variable, `NA` handling not reachable (callers coalesce first). ## Phase 2: cd_summary() disambiguates stations -- [ ] `cd_summary()` passes `labels_param` through `label_disambiguate()`. -- [ ] Tests in `test-cd_summary.R`: two stations, one `long_name` → `"Mean discharge (q_site1)"`, +- [x] `cd_summary()` passes `labels_param` through `label_disambiguate()`. +- [x] Tests in `test-cd_summary.R`: two stations, one `long_name` → `"Mean discharge (q_site1)"`, `"Mean discharge (q_site2)"`; registered ERA5 variables unchanged (registry long_names are unique — assert `anyDuplicated(cd_variables()$long_name) == 0` so the premise is pinned); one variable over several periods gets no suffix; labels match `cd_plot_comparison()` diff --git a/tests/testthat/test-cd_summary.R b/tests/testthat/test-cd_summary.R index f49e81c..cc73091 100644 --- a/tests/testthat/test-cd_summary.R +++ b/tests/testthat/test-cd_summary.R @@ -214,3 +214,44 @@ test_that("cd_summary reads a trend without trend_on as an anomaly trend (#97)", ) expect_equal(cd_summary(trend)$Unit, "%") }) + +# Stations sharing a long_name (#98) ---------------------------------------- + +station_trend <- function(...) { + tibble::tibble( + variable = c("q_site1", "q_site2", "q_site1"), period = c("annual", "annual", "spawn"), + trend_start = 2000, slope = c(0.1, -0.2, 0.3), intercept = 0, mk_pvalue = 0.05, + n_years = 20, ... + ) +} + +test_that("cd_summary tells apart stations that share a long_name (#98)", { + smry <- cd_summary(station_trend(long_name = "Mean discharge", unit = "m3/s")) + expect_equal( + smry$Parameter, + c("Mean discharge (q_site1)", "Mean discharge (q_site2)", "Mean discharge (q_site1)") + ) +}) + +test_that("cd_summary labels shared long_names as cd_plot_comparison() does (#98)", { + skip_if_not_installed("ggplot2") + cmp <- tibble::tibble( + variable = c("q_site1", "q_site2"), period = "annual", + mean_a = 1:2, mean_b = 2:3, difference = -1, method = "mean_diff", + long_name = "Mean discharge" + ) + p <- suppressWarnings(cd_plot_comparison(cmp)) + smry <- cd_summary(station_trend(long_name = "Mean discharge")[1:2, ]) + expect_setequal(smry$Parameter, unique(p$data$param)) +}) + +test_that("cd_summary adds no suffix to registered variables (#98)", { + # registry long_names are unique, so no ERA5 row is ever suffixed + expect_equal(anyDuplicated(cd_variables()$long_name), 0) + vars <- cd_variables()$variable + trend <- tibble::tibble( + variable = rep(vars, 2), period = rep(c("annual", "winter"), each = length(vars)), + trend_start = 1951, slope = 0.1, intercept = 0, mk_pvalue = 0.1, n_years = 70 + ) + expect_false(any(grepl("\\(", cd_summary(trend)$Parameter))) +}) From 2e670e09d799c0d99a9f6d5ffc25d83c511fc956 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:57:09 -0700 Subject: [PATCH 04/11] label_disambiguate(): abort rather than return a label two variables share Plan review: variable names containing parentheses can make the bounded loop end with a label still shared, silently. A post-loop check now aborts. The plot collision test pins the new facet labels, and a variable carrying different long_names by period is covered. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_anomaly.R | 25 ++++++++++++++++++------ R/cd_plot_comparison.R | 5 +++-- man/cd_plot_comparison.Rd | 5 +++-- planning/active/review-plan.md | 15 ++++++++++++++ tests/testthat/test-cd_anomaly.R | 14 +++++++++++++ tests/testthat/test-cd_plot_comparison.R | 2 ++ 6 files changed, 56 insertions(+), 10 deletions(-) create mode 100644 planning/active/review-plan.md diff --git a/R/cd_anomaly.R b/R/cd_anomaly.R index 4f74684..31a2107 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -188,18 +188,31 @@ meta_resolve <- function(x, raw = FALSE) { #' Make labels tell variables apart: a label shared by several variables (one #' long_name on many stations) gets ` (variable)` appended. Repeated because a #' suffixed label can meet another variable's own label (`c("Q", "Q", "Q (a)")`); -#' each pass lengthens only the labels still shared, and the loop is bounded by -#' the number of variables. The one place the rule lives; every consumer that +#' each pass lengthens only the labels still shared. Ordinary names settle within +#' one pass per variable; names built to collide (parentheses in `variable`) might +#' not, so a label still shared after `max_passes` aborts rather than printing two +#' variables under one name. The one place the rule lives; every consumer that #' prints labels calls it. #' @noRd -label_disambiguate <- function(variable, label) { +label_disambiguate <- function(variable, label, max_passes = length(unique(variable))) { variable <- as.character(variable) - for (i in seq_along(unique(variable))) { + shared_find <- function(label) { lab <- unique(data.frame(variable = variable, label = label)) - shared <- label %in% lab$label[duplicated(lab$label)] - if (!any(shared)) break + label %in% lab$label[duplicated(lab$label)] + } + for (i in seq_len(max_passes)) { + shared <- shared_find(label) + if (!any(shared)) return(label) label[shared] <- paste0(label[shared], " (", variable[shared], ")") } + shared <- shared_find(label) + if (any(shared)) { + rlang::abort(paste0( + "Could not give each variable its own label; still shared: ", + paste(utils::head(unique(label[shared]), 5), collapse = ", "), + ". Rename the variables or give them distinct `long_name`s." + )) + } label } diff --git a/R/cd_plot_comparison.R b/R/cd_plot_comparison.R index 2ae444d..f648314 100644 --- a/R/cd_plot_comparison.R +++ b/R/cd_plot_comparison.R @@ -8,8 +8,9 @@ #' `period`, `mean_a`, `mean_b`, `difference`, optionally #' `long_name`. Facets are labelled by `long_name` where present and #' not `NA`, otherwise by [cd_variables()], otherwise by `variable`; -#' a label shared by several variables gets the variable name appended. -#' Each variable gets its own facet whatever the labels. +#' a label shared by several variables gets the variable name appended, +#' as in [cd_summary()], until each variable's label is its own. Each +#' variable gets its own facet whatever the labels. #' @param title Optional plot title. #' @param labels Named character vector of length 2 for window labels. #' Default `c(a = "Recent", b = "Historical")`. diff --git a/man/cd_plot_comparison.Rd b/man/cd_plot_comparison.Rd index 3d0406b..dd4a957 100644 --- a/man/cd_plot_comparison.Rd +++ b/man/cd_plot_comparison.Rd @@ -11,8 +11,9 @@ cd_plot_comparison(x, title = NULL, labels = c(a = "Recent", b = "Historical")) \code{period}, \code{mean_a}, \code{mean_b}, \code{difference}, optionally \code{long_name}. Facets are labelled by \code{long_name} where present and not \code{NA}, otherwise by \code{\link[=cd_variables]{cd_variables()}}, otherwise by \code{variable}; -a label shared by several variables gets the variable name appended. -Each variable gets its own facet whatever the labels.} +a label shared by several variables gets the variable name appended, +as in \code{\link[=cd_summary]{cd_summary()}}, until each variable's label is its own. Each +variable gets its own facet whatever the labels.} \item{title}{Optional plot title.} diff --git a/planning/active/review-plan.md b/planning/active/review-plan.md new file mode 100644 index 0000000..ca3fe88 --- /dev/null +++ b/planning/active/review-plan.md @@ -0,0 +1,15 @@ +# Plan review — #98 (Plan agent, 2026-09-30) + +Verdict: sound, no blockers. Findings and disposition: + +| # | Category | Finding | Disposition | +|---|----------|---------|-------------| +| 1 | Gap | Repeat loop bounded by n variables can end with labels still shared when variable names contain parentheses (`"a) (b"` + `"b"`) — silently | Fixed: post-loop check aborts; `max_passes` arg lets a test reach it | +| 2 | Gap | Plot collision facets change from `Q (a),Q (b),Q (a)` to `Q (a) (a),Q (b),Q (a) (c)`; test only counted facets | Fixed: test asserts labels; goes in NEWS | +| 3 | Gap | `Trend on` should use `%in% "value"` like `Unit`, not coalesce + title-case | Fixed | +| 4 | Gap | One variable, different long_names by period — untested | Test added | +| 5 | Gap | NA `variable` not refused by `series_check()`; shared label gets `" (NA)"` | Accepted: honest output; noted here | +| 6 | Gap | Suffixes/columns decided per call; separately summarised tables bound together differ | Documented in `cd_summary()` details | +| 7 | — | Grouped input fine | No change | +| 8 | Note | Several `trend_start` values → rows differ only by `Years` | Filed as follow-up issue | +| — | Acceptance | Combined stations × scales × region test | Added | diff --git a/tests/testthat/test-cd_anomaly.R b/tests/testthat/test-cd_anomaly.R index 2f70c2f..e149fc8 100644 --- a/tests/testthat/test-cd_anomaly.R +++ b/tests/testthat/test-cd_anomaly.R @@ -306,3 +306,17 @@ test_that("label_disambiguate reads a factor variable by name", { c("Q (q2)", "Q (q1)") ) }) + +test_that("label_disambiguate handles one variable carrying different labels by period", { + expect_identical( + label_disambiguate(c("a", "a", "b"), c("X", "Y", "X")), + c("X (a)", "Y", "X (b)") + ) +}) + +test_that("label_disambiguate aborts rather than return a label two variables share", { + expect_error( + label_disambiguate(c("a", "b", "c"), c("Q", "Q", "Q (a)"), max_passes = 1), + "still shared: Q \\(a\\)" + ) +}) diff --git a/tests/testthat/test-cd_plot_comparison.R b/tests/testthat/test-cd_plot_comparison.R index e85a345..42275f0 100644 --- a/tests/testthat/test-cd_plot_comparison.R +++ b/tests/testthat/test-cd_plot_comparison.R @@ -79,6 +79,8 @@ test_that("cd_plot_comparison never puts two variables in one facet, even when l expect_false(anyDuplicated(facet_vars$facet) > 0) b <- suppressWarnings(ggplot2::ggplot_build(p)) expect_equal(nrow(b$layout$layout), 3) + # labels, not just facets, are distinct per variable (#98) + expect_setequal(unique(p$data$param), c("Q (a) (a)", "Q (b)", "Q (a) (c)")) }) test_that("cd_plot_comparison adds no suffix to one variable compared over several periods", { From 86a6319ad0978a706d6fa855e60749f94372f1ed Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:57:09 -0700 Subject: [PATCH 05/11] cd_summary(): add Trend on when a table mixes raw-value and anomaly trends bind_rows(cd_trend(x), cd_trend(ano)) gave rows differing by Unit at most - for tmean not even that. A Trend on column (Value/Anomaly) now follows Period when the table holds both scales; a single-scale table keeps its shape. Same rule as Unit: only trend_on == "value" is a raw-value trend. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_summary.R | 24 +++++++++++++- man/cd_summary.Rd | 14 +++++++- planning/active/task_plan.md | 8 ++--- tests/testthat/test-cd_summary.R | 55 ++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 6 deletions(-) diff --git a/R/cd_summary.R b/R/cd_summary.R index 6bf9606..380a556 100644 --- a/R/cd_summary.R +++ b/R/cd_summary.R @@ -15,12 +15,24 @@ #' series is `"%"`, which does not describe a slope in mm — so it agrees #' with the axis label of [cd_plot_timeseries()] on the same series. #' +#' Rows are kept distinguishable. A `long_name` shared by several variables +#' (one label on many stations) gets the variable name appended, as in +#' [cd_plot_comparison()]: `"Mean discharge (q_site1)"`. A table holding both +#' raw-value and anomaly trends, such as +#' `dplyr::bind_rows(cd_trend(x), cd_trend(ano))`, gains a `Trend on` column +#' (`"Value"` or `"Anomaly"`; a missing or `NA` `trend_on` reads as +#' `"Anomaly"`). A table on one scale has no such column. Both are decided +#' within one call, so tables summarised separately and then bound together +#' (one per region, say) can differ in suffixes and columns; bind the trend +#' tables first and summarise once. +#' #' @param trend A tibble from [cd_trend()]. #' @param region_name Optional character label for the AOI. If provided, #' adds a `Region` column. #' #' @return A tibble with columns `Parameter`, `Period`, `Slope`, `Years`, -#' `Total Change`, `Unit`, `p-value`, and optionally `Region`. +#' `Total Change`, `Unit`, `p-value`, and optionally `Region`. When `trend` +#' mixes raw-value and anomaly trends, a `Trend on` column follows `Period`. #' #' @examples #' catalog <- cd_catalog( @@ -61,6 +73,16 @@ cd_summary <- function(trend, region_name = NULL) { ) |> dplyr::select("Parameter", "Period", "Slope", "Years", "Total Change", "Unit", "p-value") + # A table mixing raw-value and anomaly trends of one series would otherwise + # give rows that differ by Unit at most. Same rule as Unit: only "value" is + # a raw-value trend; anything else, NA included, reads as anomaly. + on_value <- col_or_na(trend, "trend_on") %in% "value" + if (length(unique(on_value)) > 1) { + out <- tibble::add_column( + out, `Trend on` = dplyr::if_else(on_value, "Value", "Anomaly"), .after = "Period" + ) + } + if (!is.null(region_name)) { out$Region <- region_name } diff --git a/man/cd_summary.Rd b/man/cd_summary.Rd index a57ac5c..3b1c397 100644 --- a/man/cd_summary.Rd +++ b/man/cd_summary.Rd @@ -14,7 +14,8 @@ adds a \code{Region} column.} } \value{ A tibble with columns \code{Parameter}, \code{Period}, \code{Slope}, \code{Years}, -\verb{Total Change}, \code{Unit}, \code{p-value}, and optionally \code{Region}. +\verb{Total Change}, \code{Unit}, \code{p-value}, and optionally \code{Region}. When \code{trend} +mixes raw-value and anomaly trends, a \verb{Trend on} column follows \code{Period}. } \description{ Joins trend statistics with variable metadata and computes Total @@ -32,6 +33,17 @@ raw values (\code{trend_on == "value"}) \code{Unit} is shown only for \code{abso and \code{pct_point_diff} series — the anomaly unit of a \code{pct_normal} series is \code{"\%"}, which does not describe a slope in mm — so it agrees with the axis label of \code{\link[=cd_plot_timeseries]{cd_plot_timeseries()}} on the same series. + +Rows are kept distinguishable. A \code{long_name} shared by several variables +(one label on many stations) gets the variable name appended, as in +\code{\link[=cd_plot_comparison]{cd_plot_comparison()}}: \code{"Mean discharge (q_site1)"}. A table holding both +raw-value and anomaly trends, such as +\code{dplyr::bind_rows(cd_trend(x), cd_trend(ano))}, gains a \verb{Trend on} column +(\code{"Value"} or \code{"Anomaly"}; a missing or \code{NA} \code{trend_on} reads as +\code{"Anomaly"}). A table on one scale has no such column. Both are decided +within one call, so tables summarised separately and then bound together +(one per region, say) can differ in suffixes and columns; bind the trend +tables first and summarise once. } \examples{ catalog <- cd_catalog( diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index bd45129..32f020c 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -26,14 +26,14 @@ for the same variables. ## Phase 3: cd_summary() carries the scale when a table mixes them -- [ ] Scale = `coalesce(col_or_na(trend, "trend_on"), "anomaly")` (missing/NA read as anomaly, - as `cd_summary()` already does for units). When it has more than one distinct value, add +- [x] Scale = `col_or_na(trend, "trend_on") %in% "value"` — the `Unit` rule; anything else, + NA included, reads as anomaly (plan review #3). When it has more than one distinct value, add `Trend on` (`"Value"`/`"Anomaly"`) after `Period`; otherwise the output shape is unchanged. -- [ ] Tests: mixed table (`bind_rows(cd_trend(x), cd_trend(ano))` for tmean — identical Unit, +- [x] Tests: mixed table (`bind_rows(cd_trend(x), cd_trend(ano))` for tmean — identical Unit, the case the issue names) → `Trend on` present with `c("Value", "Anomaly")`; single-scale and no-`trend_on` tables → `expect_named()` unchanged; value + `NA` trend_on → column present, NA row reads `"Anomaly"`; `region_name` still last. -- [ ] Roxygen: `@return` and description name both the ` (variable)` suffix and the conditional +- [x] Roxygen: `@return` and description name both the ` (variable)` suffix and the conditional `Trend on` column; `devtools::document()`. ## Phase 4: Docs diff --git a/tests/testthat/test-cd_summary.R b/tests/testthat/test-cd_summary.R index cc73091..639976b 100644 --- a/tests/testthat/test-cd_summary.R +++ b/tests/testthat/test-cd_summary.R @@ -255,3 +255,58 @@ test_that("cd_summary adds no suffix to registered variables (#98)", { ) expect_false(any(grepl("\\(", cd_summary(trend)$Parameter))) }) + +# Tables mixing raw-value and anomaly trends (#98) ------------------------- + +cols_summary <- c("Parameter", "Period", "Slope", "Years", "Total Change", "Unit", "p-value") + +test_that("cd_summary adds Trend on when a table mixes scales (#98)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + # tmean is absolute: raw and anomaly rows share Parameter, Period and Unit + x <- raw_series("tmean") + ano <- cd_anomaly(x, cd_baseline(x, 2000:2004)) + smry <- cd_summary(dplyr::bind_rows(cd_trend(x, 2000), cd_trend(ano, 2000)), + region_name = "AOI") + expect_named(smry, c("Parameter", "Period", "Trend on", cols_summary[-(1:2)], "Region")) + expect_equal(smry$`Trend on`, c("Value", "Anomaly")) + expect_equal(smry$Unit, c("°C", "°C")) + expect_equal(anyDuplicated(smry[c("Parameter", "Period", "Trend on")]), 0) +}) + +test_that("cd_summary adds no Trend on column to a table on one scale (#98)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- raw_series("tmean") + ano <- cd_anomaly(x, cd_baseline(x, 2000:2004)) + expect_named(cd_summary(cd_trend(x, 2000)), cols_summary) + expect_named(cd_summary(cd_trend(ano, 2000)), cols_summary) + # no trend_on at all, as in tables saved before 0.5.2 + expect_named(cd_summary(station_trend()), cols_summary) + expect_named(cd_summary(station_trend()[0, ]), cols_summary) +}) + +test_that("cd_summary reads an NA trend_on as Anomaly in Trend on (#98)", { + smry <- cd_summary(station_trend(trend_on = c("value", NA, "anomaly"))) + expect_equal(smry$`Trend on`, c("Value", "Anomaly", "Anomaly")) +}) + +test_that("cd_summary keeps stations x scales x region distinct (#98)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- dplyr::bind_rows( + raw_series("q_site1", anomaly_type = "absolute", unit = "m3/s", long_name = "Mean discharge"), + raw_series("q_site2", anomaly_type = "absolute", unit = "m3/s", long_name = "Mean discharge") + ) + ano <- cd_anomaly(x, cd_baseline(x, 2000:2004)) + smry <- cd_summary(dplyr::bind_rows(cd_trend(x, 2000), cd_trend(ano, 2000)), region_name = "AOI") + expect_equal(nrow(smry), 4) + expect_equal(anyDuplicated(smry[c("Parameter", "Period", "Trend on")]), 0) + expect_setequal(smry$Parameter, c("Mean discharge (q_site1)", "Mean discharge (q_site2)")) + expect_identical(names(smry)[ncol(smry)], "Region") +}) + +test_that("cd_summary reads any trend_on other than value as Anomaly (#98)", { + smry <- cd_summary(station_trend(trend_on = c("value", "anomalies", "anomaly"))) + expect_equal(smry$`Trend on`, c("Value", "Anomaly", "Anomaly")) +}) From 1cf3c37275d7553fbee03ada4c6740738cbd4d8a Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 07:57:55 -0700 Subject: [PATCH 06/11] CLAUDE.md: label_disambiguate() joins the shared series helpers Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- CLAUDE.md | 2 +- planning/active/progress.md | 9 +++++++++ planning/active/task_plan.md | 6 +++--- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c74785c..3133d89 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,7 @@ The historical `cd_fetch()` / `cd_derive()` R-side producer functions still ship **Key design decision:** Raw climate values on STAC (not pre-computed anomalies). All baseline/anomaly/trend computation consumer-side for maximum flexibility over reference periods and comparison windows. -**The consumer chain is not ERA5-only.** `cd_baseline()` → `cd_anomaly()` → `cd_trend()` → `cd_summary()` / `cd_compare()` take any series in the long format (`variable`, `period`, `year`, `value`, optional `anomaly_type`/`unit`/`long_name`) — other packages (wet's streamflow) target it. The contract lives in `?cd_anomaly`; the rules live in three helpers in `R/cd_anomaly.R` (`series_check`, `meta_resolve`, `meta_check`). Call them from any new consumer function rather than reading `cd_variables()` directly — per-function copies are what three review rounds kept finding broken (#92). +**The consumer chain is not ERA5-only.** `cd_baseline()` → `cd_anomaly()` → `cd_trend()` → `cd_summary()` / `cd_compare()` take any series in the long format (`variable`, `period`, `year`, `value`, optional `anomaly_type`/`unit`/`long_name`) — other packages (wet's streamflow) target it. The contract lives in `?cd_anomaly`; the rules live in four helpers in `R/cd_anomaly.R` (`series_check`, `meta_resolve`, `meta_check`, `label_disambiguate`). Call them from any new consumer function rather than reading `cd_variables()` directly — per-function copies are what three review rounds kept finding broken (#92). Anything that prints a label per variable passes it through `label_disambiguate()`, which appends ` (variable)` to a `long_name` several stations share; `cd_summary()` and `cd_plot_comparison()` both do (#98). **A trend table can mix scales.** Since #97 every `cd_trend()` row carries `trend_on` (`"value"` / `"anomaly"`), and `bind_rows(cd_trend(x), cd_trend(ano))` is a legal input. Any consumer of a trend table reads it with `col_or_na(trend, "trend_on")`: a missing or `NA` `trend_on` (hand-built tables, tables saved before 0.5.2) must not error. `cd_summary()` reads a missing `trend_on` as anomaly; `cd_plot_timeseries()` draws it on either scale (#103). diff --git a/planning/active/progress.md b/planning/active/progress.md index 440fe33..8e9beeb 100644 --- a/planning/active/progress.md +++ b/planning/active/progress.md @@ -6,3 +6,12 @@ - Created branch `98-cd-summary-stations-sharing-a-long-name` off main - Scaffolded PWF baseline from issue #98 with approved phases - Next: start Phase 1 + +- Plan review (Plan agent) returned no blockers; findings and dispositions in `review-plan.md` +- Phase 1 `d3cb7a9`, Phase 2 `1b5d548`, review hardening `2e670e0`, Phase 3 `86a6319` +- Mutation checks in a scratch copy: removing the station fix, the `Trend on` block, or the + helper's abort each turns the new tests red +- Full suite: `[ FAIL 0 | WARN 6 | SKIP 0 | PASS 393 ]` (the 6 warnings are the existing + row-names warning from `labels["a"]` in `cd_plot_comparison()`) +- `pkgdown::check_pkgdown()` aborts on an existing DESCRIPTION URL check (github.io url missing), + unrelated to this branch; no new export diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index 32f020c..1d795da 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -37,15 +37,15 @@ `Trend on` column; `devtools::document()`. ## Phase 4: Docs -- [ ] `CLAUDE.md` consumer-chain paragraph: the helpers in `R/cd_anomaly.R` are now four +- [x] `CLAUDE.md` consumer-chain paragraph: the helpers in `R/cd_anomaly.R` are now four (add `label_disambiguate`) — any new consumer that prints labels calls it. -- [ ] Full `devtools::test()`, `lintr::lint_package()`, `pkgdown::check_pkgdown()` (no new export). +- [x] Full `devtools::test()`, `lintr::lint_package()`, `pkgdown::check_pkgdown()` (no new export). NEWS + version bump are left to `/gh-pr-merge`, per the repo workflow (0.5.4, patch). ## Validation -- [ ] Tests pass +- [x] Tests pass - [ ] `/code-check` clean on each commit - [ ] PWF checkboxes match landed work - [ ] `/planning-archive` on completion From bb6718d0b97bf07869d4e1dce0e269b35555fdca Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 08:04:16 -0700 Subject: [PATCH 07/11] label_disambiguate(): bound passes by distinct (variable, label) pairs Code-check round 1: one variable can carry a different label per period, so a collision chain can outrun the variable count and abort on valid input. Enumerated 66,711 small inputs at the new bound: no aborts, no label shared by two variables. Also drops cd_summary() advice to bind per-region trend tables before summarising, which would merge regions. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_anomaly.R | 10 +++++---- R/cd_plot_comparison.R | 4 ++-- R/cd_summary.R | 6 +++--- man/cd_plot_comparison.Rd | 4 ++-- man/cd_summary.Rd | 6 +++--- planning/active/findings.md | 13 ++++++++++++ planning/active/label_disambiguate_enum.R | 20 +++++++++++++++++ planning/active/review-plan.md | 2 +- planning/active/review-round1.md | 26 +++++++++++++++++++++++ tests/testthat/test-cd_anomaly.R | 6 ++++++ 10 files changed, 82 insertions(+), 15 deletions(-) create mode 100644 planning/active/label_disambiguate_enum.R create mode 100644 planning/active/review-round1.md diff --git a/R/cd_anomaly.R b/R/cd_anomaly.R index 31a2107..f68bf14 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -188,13 +188,15 @@ meta_resolve <- function(x, raw = FALSE) { #' Make labels tell variables apart: a label shared by several variables (one #' long_name on many stations) gets ` (variable)` appended. Repeated because a #' suffixed label can meet another variable's own label (`c("Q", "Q", "Q (a)")`); -#' each pass lengthens only the labels still shared. Ordinary names settle within -#' one pass per variable; names built to collide (parentheses in `variable`) might -#' not, so a label still shared after `max_passes` aborts rather than printing two +#' each pass lengthens only the labels still shared. A chain of such collisions +#' can be as long as the distinct (variable, label) pairs, not the variables — +#' one variable may carry a different label per period — so that is the bound. +#' A label still shared after `max_passes` aborts rather than printing two #' variables under one name. The one place the rule lives; every consumer that #' prints labels calls it. #' @noRd -label_disambiguate <- function(variable, label, max_passes = length(unique(variable))) { +label_disambiguate <- function(variable, label, + max_passes = nrow(unique(data.frame(v = as.character(variable), l = label)))) { variable <- as.character(variable) shared_find <- function(label) { lab <- unique(data.frame(variable = variable, label = label)) diff --git a/R/cd_plot_comparison.R b/R/cd_plot_comparison.R index f648314..097c04e 100644 --- a/R/cd_plot_comparison.R +++ b/R/cd_plot_comparison.R @@ -9,8 +9,8 @@ #' `long_name`. Facets are labelled by `long_name` where present and #' not `NA`, otherwise by [cd_variables()], otherwise by `variable`; #' a label shared by several variables gets the variable name appended, -#' as in [cd_summary()], until each variable's label is its own. Each -#' variable gets its own facet whatever the labels. +#' as in [cd_summary()]. Each variable gets its own facet whatever the +#' labels. #' @param title Optional plot title. #' @param labels Named character vector of length 2 for window labels. #' Default `c(a = "Recent", b = "Historical")`. diff --git a/R/cd_summary.R b/R/cd_summary.R index 380a556..259e055 100644 --- a/R/cd_summary.R +++ b/R/cd_summary.R @@ -22,9 +22,9 @@ #' `dplyr::bind_rows(cd_trend(x), cd_trend(ano))`, gains a `Trend on` column #' (`"Value"` or `"Anomaly"`; a missing or `NA` `trend_on` reads as #' `"Anomaly"`). A table on one scale has no such column. Both are decided -#' within one call, so tables summarised separately and then bound together -#' (one per region, say) can differ in suffixes and columns; bind the trend -#' tables first and summarise once. +#' within one call, so summaries bound together (one per region, each with +#' its `region_name`) can differ in suffixes, and a `Trend on` column present +#' in only some of them is `NA` for the rest. #' #' @param trend A tibble from [cd_trend()]. #' @param region_name Optional character label for the AOI. If provided, diff --git a/man/cd_plot_comparison.Rd b/man/cd_plot_comparison.Rd index dd4a957..9d23e44 100644 --- a/man/cd_plot_comparison.Rd +++ b/man/cd_plot_comparison.Rd @@ -12,8 +12,8 @@ cd_plot_comparison(x, title = NULL, labels = c(a = "Recent", b = "Historical")) \code{long_name}. Facets are labelled by \code{long_name} where present and not \code{NA}, otherwise by \code{\link[=cd_variables]{cd_variables()}}, otherwise by \code{variable}; a label shared by several variables gets the variable name appended, -as in \code{\link[=cd_summary]{cd_summary()}}, until each variable's label is its own. Each -variable gets its own facet whatever the labels.} +as in \code{\link[=cd_summary]{cd_summary()}}. Each variable gets its own facet whatever the +labels.} \item{title}{Optional plot title.} diff --git a/man/cd_summary.Rd b/man/cd_summary.Rd index 3b1c397..1b3c0e2 100644 --- a/man/cd_summary.Rd +++ b/man/cd_summary.Rd @@ -41,9 +41,9 @@ raw-value and anomaly trends, such as \code{dplyr::bind_rows(cd_trend(x), cd_trend(ano))}, gains a \verb{Trend on} column (\code{"Value"} or \code{"Anomaly"}; a missing or \code{NA} \code{trend_on} reads as \code{"Anomaly"}). A table on one scale has no such column. Both are decided -within one call, so tables summarised separately and then bound together -(one per region, say) can differ in suffixes and columns; bind the trend -tables first and summarise once. +within one call, so summaries bound together (one per region, each with +its \code{region_name}) can differ in suffixes, and a \verb{Trend on} column present +in only some of them is \code{NA} for the rest. } \examples{ catalog <- cd_catalog( diff --git a/planning/active/findings.md b/planning/active/findings.md index f0e9153..4d512cd 100644 --- a/planning/active/findings.md +++ b/planning/active/findings.md @@ -41,3 +41,16 @@ column (changes every existing table, both vignettes' kables) and over suffixing | Error | Resolution | |-------|------------| + +## label_disambiguate() pass bound — enumeration (2026-09-30) + +Code-check round 1 showed the first bound (`n_distinct(variable)`) was reasoned, not measured: +`label_disambiguate(c("a","e","e","e"), c("Q","Q","Q (a)","Q (a) (a)"))` aborted after two +passes and settles at three, because one variable can carry a different label per period. The +bound is now the number of distinct (variable, label) pairs. + +Enumeration (`label_disambiguate_enum.R`): every set of 1–4 distinct pairs over variables +`a`, `b`, `a) (b` and 12 labels built by chaining their suffixes to depth 2 — 66,711 sets. +**0 aborts, 0 labels shared by two variables.** 969 sets merge two labels of *one* variable +(`a` carrying `Q` and `Q (a)` in different periods both print `Q (a)`); those rows still differ +by `Period`, and the one-pass rule on main does the same. Accepted. diff --git a/planning/active/label_disambiguate_enum.R b/planning/active/label_disambiguate_enum.R new file mode 100644 index 0000000..380cfe1 --- /dev/null +++ b/planning/active/label_disambiguate_enum.R @@ -0,0 +1,20 @@ +devtools::load_all(quiet = TRUE) +vs <- c("a", "b", "a) (b") +sfx <- function(l) as.vector(outer(l, vs, function(x, v) paste0(x, " (", v, ")"))) +labs <- unique(c("Q", sfx("Q"), sfx(sfx("Q")))) +pairs <- expand.grid(v = vs, l = labs, stringsAsFactors = FALSE) +n_abort <- 0; n_bad <- 0; n <- 0; max_used <- 0 +for (k in 1:4) { + cmb <- utils::combn(nrow(pairs), k) + for (j in seq_len(ncol(cmb))) { + p <- pairs[cmb[, j], ] + n <- n + 1 + out <- tryCatch(label_disambiguate(p$v, p$l), error = function(e) NULL) + if (is.null(out)) { n_abort <- n_abort + 1; next } + u <- unique(data.frame(v = p$v, l = out)) + if (anyDuplicated(u$l) && any(duplicated(u$l) & !duplicated(u[c("l")]) | TRUE) && any(tapply(u$v, u$l, function(z) length(unique(z))) > 1)) { n_bad <- n_bad + 1; cat("SHARED\n") } + # rows of one variable with one label keep one label (no split) + if (nrow(u) != nrow(unique(p))) { n_merge <- get0("n_merge", ifnotfound = 0) + 1; if (n_merge == 1) print(cbind(p, out)) } + } +} +cat("merged-within-variable:", get0("n_merge", ifnotfound = 0), "\n"); cat("sets:", n, " labels:", length(labs), " aborts:", n_abort, " violations:", n_bad, "\n") diff --git a/planning/active/review-plan.md b/planning/active/review-plan.md index ca3fe88..843c1a5 100644 --- a/planning/active/review-plan.md +++ b/planning/active/review-plan.md @@ -11,5 +11,5 @@ Verdict: sound, no blockers. Findings and disposition: | 5 | Gap | NA `variable` not refused by `series_check()`; shared label gets `" (NA)"` | Accepted: honest output; noted here | | 6 | Gap | Suffixes/columns decided per call; separately summarised tables bound together differ | Documented in `cd_summary()` details | | 7 | — | Grouped input fine | No change | -| 8 | Note | Several `trend_start` values → rows differ only by `Years` | Filed as follow-up issue | +| 8 | Note | Several `trend_start` values → rows differ only by `Years` | Filed as #106 | | — | Acceptance | Combined stations × scales × region test | Added | diff --git a/planning/active/review-round1.md b/planning/active/review-round1.md new file mode 100644 index 0000000..1d79dff --- /dev/null +++ b/planning/active/review-round1.md @@ -0,0 +1,26 @@ +# Code-check review, round 1 (#98) + +Reviewer: subagent, 2026-09-30. Diff: `review.diff` (planning/ excluded). Tests for +test-cd_summary.R, test-cd_anomaly.R and test-cd_plot_comparison.R pass in a copy of the repo +(0 failed, 0 error, 0 skipped). + +## Findings + +- **[severity: fragile]** R/cd_anomaly.R:191-197 — the pass bound `max_passes = length(unique(variable))` is not an upper bound on the passes the rule needs, and the roxygen claim "Ordinary names settle within one pass per variable; names built to collide (parentheses in `variable`) might not" is false. The number of passes depends on how long the chain of colliding labels is, and one variable can carry a different `long_name` in each period (meta_check allows that), so the chain can be longer than the number of variables even when no variable name has a parenthesis. Minimal repro, reproduced in a copy: + + ```r + label_disambiguate(c("a", "e", "e", "e"), c("Q", "Q", "Q (a)", "Q (a) (a)")) + #> Error: Could not give each variable its own label; still shared: Q (a) (a). ... + ``` + + With 2 variables the loop stops after 2 passes, but a third pass would settle it: `"Q (a) (a) (a)"` / `"Q (a) (a) (e)"`. A fuzz over 5 plain variable names and 10 labels drawn from `Q`, `Q (a)`, `Q (a) (a)`, … aborted in 11 of 40,000 draws at the current bound. With `max_passes` set to the number of distinct (variable, label) pairs it aborted in 0. The effect is that `cd_summary()`, and `cd_plot_comparison()`, which plotted such input before this branch because its facets are keyed on variable, now error on valid input. The input is contrived, so the practical risk is low. The error is loud, not silent. However, the comment's guarantee is wrong, and so is the `cd_plot_comparison` roxygen "until each variable's label is its own" (R/cd_plot_comparison.R:11-12, man/cd_plot_comparison.Rd). A bound of `nrow(unique(data.frame(variable, label)))` fixes it. So does the count of distinct labels plus one. Either way, the comment should stop saying that only parenthesised variable names can fail. + +- **[severity: fragile]** R/cd_summary.R:24-27 (and man/cd_summary.Rd) — the new advice "tables summarised separately and then bound together (one per region, say) can differ in suffixes and columns; bind the trend tables first and summarise once" leads to the defect this issue fixes when it is applied to the example it names. Per-region trend tables from `cd_extract()` on different AOIs carry the same variable names (`tmean` in each). `bind_rows(trend_A, trend_B) |> cd_summary()` therefore gives pairs of rows with the same Parameter/Period and nothing to tell them apart. `label_disambiguate()` sees one variable and adds no suffix. `region_name` is a single scalar, so it cannot label both regions. The reader loses the region identity that summarising per region with `region_name` and then binding keeps. The only cost of that per-region approach is a `Trend on` column that is `NA` for regions on one scale, plus possibly different suffixes. The advice holds only when variable names are already distinct across the tables being bound, for example wet's per-station names. It should say that, or recommend summarising per region with `region_name` and binding the summaries. + +No other issues found. Checked: +- The `Trend on` logic: NA/missing/other values read as Anomaly, and this matches `meta_resolve(raw=)`. +- Zero-row input and factor `variable`. +- `tibble` is in Imports. +- `label_disambiguate` gives the post-condition of distinct labels per variable whenever it returns (fuzz: 0 violations). +- The `cd_plot_comparison` facet key is unchanged. +- The CLAUDE.md claim that both consumers call the helper is true. `cd_plot_timeseries()` draws one label per plot, so it is not a sibling that was missed. diff --git a/tests/testthat/test-cd_anomaly.R b/tests/testthat/test-cd_anomaly.R index e149fc8..8df2c7e 100644 --- a/tests/testthat/test-cd_anomaly.R +++ b/tests/testthat/test-cd_anomaly.R @@ -314,6 +314,12 @@ test_that("label_disambiguate handles one variable carrying different labels by ) }) +test_that("label_disambiguate settles a collision chain longer than the variable count", { + # two variables, three passes: a carries three labels across periods + out <- label_disambiguate(c("a", "e", "e", "e"), c("Q", "Q", "Q (a)", "Q (a) (a)")) + expect_identical(out, c("Q (a) (a) (a)", "Q (e)", "Q (a) (e)", "Q (a) (a) (e)")) +}) + test_that("label_disambiguate aborts rather than return a label two variables share", { expect_error( label_disambiguate(c("a", "b", "c"), c("Q", "Q", "Q (a)"), max_passes = 1), From 76a4428d09aef9f6fdb52a800756947d4a18782b Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 08:09:04 -0700 Subject: [PATCH 08/11] label_disambiguate(): state the limit of the pass bound Code-check round 2: a variable name built to collide ("a) (a" beside "a") makes labels that never settle, so the comment's claim that the pair count is the bound held only for the enumerated names. It aborts; now pinned by a test and said in the comment. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_anomaly.R | 8 +++-- planning/active/findings.md | 5 +++ planning/active/review-round2.md | 53 ++++++++++++++++++++++++++++++++ tests/testthat/test-cd_anomaly.R | 5 +++ 4 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 planning/active/review-round2.md diff --git a/R/cd_anomaly.R b/R/cd_anomaly.R index f68bf14..3be4f91 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -190,9 +190,11 @@ meta_resolve <- function(x, raw = FALSE) { #' suffixed label can meet another variable's own label (`c("Q", "Q", "Q (a)")`); #' each pass lengthens only the labels still shared. A chain of such collisions #' can be as long as the distinct (variable, label) pairs, not the variables — -#' one variable may carry a different label per period — so that is the bound. -#' A label still shared after `max_passes` aborts rather than printing two -#' variables under one name. The one place the rule lives; every consumer that +#' one variable may carry a different label per period — so that is the bound; +#' it held for every small input enumerated in #98. A variable name built to +#' collide (`"a) (a"` beside `"a"`) can make labels that never settle, so a label +#' still shared after `max_passes` aborts rather than printing two variables +#' under one name. The one place the rule lives; every consumer that #' prints labels calls it. #' @noRd label_disambiguate <- function(variable, label, diff --git a/planning/active/findings.md b/planning/active/findings.md index 4d512cd..00966b4 100644 --- a/planning/active/findings.md +++ b/planning/active/findings.md @@ -54,3 +54,8 @@ Enumeration (`label_disambiguate_enum.R`): every set of 1–4 distinct pairs ove **0 aborts, 0 labels shared by two variables.** 969 sets merge two labels of *one* variable (`a` carrying `Q` and `Q (a)` in different periods both print `Q (a)`); those rows still differ by `Period`, and the one-pass rule on main does the same. Accepted. + +Bound of the enumeration: its names were `a`, `b`, `a) (b`. Round 2 found a shape outside it — +variables `a` and `a) (a`, with `a) (a` labelled `Q` and `Q (a)` — that never settles at any +bound (tried to 50). It aborts with "Rename the variables…"; `cd_plot_comparison()` plotted it on +main. Needs a name built to collide; accepted, pinned by a test, stated in the helper comment. diff --git a/planning/active/review-round2.md b/planning/active/review-round2.md new file mode 100644 index 0000000..c809d2e --- /dev/null +++ b/planning/active/review-round2.md @@ -0,0 +1,53 @@ +# Code-check review, round 2 (#98) + +Reviewer: subagent, 2026-09-30. Diff: `review.diff` (planning/ excluded), HEAD bb6718d. +All probes were run in a copy of the repo under the session scratchpad. The repo itself was not modified. + +## Clean + +No issues found. + +## What was checked + +- **Test run (copy, `NOT_CRAN=true`).** Results per file: + - test-cd_anomaly: 62 pass + - test-cd_summary: 73 pass + - test-cd_plot_comparison: 13 pass + - test-cd_trend: 28 pass + - test-cd_plot_timeseries: 39 pass + + All five had 0 fail, 0 error and 0 skip. +- **The new `max_passes` default expression.** + - The default is evaluated lazily, at `seq_len(max_passes)`. That happens after `variable <- as.character(variable)` and before `label` changes, so it counts the original pairs. + - A factor `variable` is read by name, and a test covers it. + - Zero-length input gives a 0-row frame, so `max_passes` is 0, the loop is skipped and `character(0)` is returned. A test covers it. + - An NA `variable` or NA label goes through `unique()`/`duplicated()` without error and gives `"Q (NA)"` / `"NA (a)"`. This is accepted by convention. + - Neither caller can pass a factor `label`: `coalesce()` over `col_or_na()` (character) and `as.character(variable)` always yields character. +- **Is the bound sufficient for ordinary names?** + - Fuzz: 30,000 random sets over 3 plain variable names, 2 to 12 rows each, with labels drawn from 121 chained suffixes (`Q`, `Q (a)`, `Q (a) (b)`, ...). + - The most passes any set needed was 3. The largest value of (passes needed − distinct pairs) was −1, and 0 sets exceeded the bound. + - A hand-built 7-pair chain needed 6 passes. + - A second fuzz of 20,000 sets over 4 plain names and over `{a, b, "a) (b", "b) (a"}` gave 0 aborts. +- **Non-convergence, not reported as a finding.** When one variable's name contains `) (` followed by another variable's name, the rule can fail to converge. Minimal set: + - variables `"a"` and `"a) (a"` + - `a` carries `"Q"`; `"a) (a"` carries `"Q"` in one period and `"Q (a)"` in another + + The rule then never settles at any bound: tried up to 50 passes. So `cd_summary()` and `cd_plot_comparison()` abort on it, where main plotted it. The comment's "so that is the bound" reads as a sufficiency claim, and it holds only for names without that shape. The input is deliberately built to collide, it falls under the accepted "contrived label collisions" tradeoff, and the failure is a loud abort whose remedy fits the case ("Rename the variables"). Recorded so nobody rediscovers it; no change recommended. + + Separately, `label_disambiguate_enum.R` uses `"a) (b"`, and its "0 aborts" is true for that space. It does not reach the `"a) (a"` shape. +- **cd_summary `Trend on`.** + - The predicate `%in% "value"` is the same one `meta_resolve(raw=)` uses, so it cannot be NA and `if_else` is safe. + - A zero-row table gives `unique(logical(0))` of length 0, so no column is added. A test covers it. + - Plain `data.frame` input: `tibble::add_column(.after = "Period")` works and returns a data.frame. + - Grouped input is ungrouped first. + - A factor `variable` with a shared long_name across mixed scales gives 4 distinct rows. + - `tibble` is in Imports. +- **Doc text.** + - The rewritten cd_summary paragraph now says per-region summaries (each with its `region_name`) bound together can differ in suffixes and have an NA `Trend on`. That is accurate, and the round-1 advice to bind trend tables first is gone. + - The `@return` addition matches the code. + - The cd_plot_comparison `@param` text matches the code. + - The CLAUDE.md claim that both consumers call the helper is true. `cd_plot_timeseries()` uses one label per plot (`meta$long_name[1]`), so it is not a missed consumer. +- **Regressions.** + - The cd_plot_comparison facet key (variable + label) is unchanged. + - Plain-name output is identical to the old single-pass rule wherever one pass sufficed. + - The one intended change is a chained collision such as `"Q (a) (a)"`, which goes in NEWS. diff --git a/tests/testthat/test-cd_anomaly.R b/tests/testthat/test-cd_anomaly.R index 8df2c7e..ab6f437 100644 --- a/tests/testthat/test-cd_anomaly.R +++ b/tests/testthat/test-cd_anomaly.R @@ -325,4 +325,9 @@ test_that("label_disambiguate aborts rather than return a label two variables sh label_disambiguate(c("a", "b", "c"), c("Q", "Q", "Q (a)"), max_passes = 1), "still shared: Q \\(a\\)" ) + # a variable name built to collide never settles, at any bound + expect_error( + label_disambiguate(c("a", "a) (a", "a) (a"), c("Q", "Q", "Q (a)")), + "still shared" + ) }) From 40c18bded38b8788d13a395a5cd7e2488968d0a5 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 08:14:52 -0700 Subject: [PATCH 09/11] Narrow the label_disambiguate() reach claim to side-by-side labels Code-check round 3: CLAUDE.md and the helper comment said every label printer calls it, but cd_plot_timeseries() plots one caller-chosen variable and does not. Claims narrowed; cd_plot_timeseries() docs say its label is not suffixed and the station belongs in title. Relates to #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- CLAUDE.md | 2 +- R/cd_anomaly.R | 4 +-- R/cd_plot_timeseries.R | 4 ++- man/cd_plot_timeseries.Rd | 4 ++- planning/active/review-round3.md | 52 ++++++++++++++++++++++++++++++++ 5 files changed, 61 insertions(+), 5 deletions(-) create mode 100644 planning/active/review-round3.md diff --git a/CLAUDE.md b/CLAUDE.md index 3133d89..0e8539e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,7 @@ The historical `cd_fetch()` / `cd_derive()` R-side producer functions still ship **Key design decision:** Raw climate values on STAC (not pre-computed anomalies). All baseline/anomaly/trend computation consumer-side for maximum flexibility over reference periods and comparison windows. -**The consumer chain is not ERA5-only.** `cd_baseline()` → `cd_anomaly()` → `cd_trend()` → `cd_summary()` / `cd_compare()` take any series in the long format (`variable`, `period`, `year`, `value`, optional `anomaly_type`/`unit`/`long_name`) — other packages (wet's streamflow) target it. The contract lives in `?cd_anomaly`; the rules live in four helpers in `R/cd_anomaly.R` (`series_check`, `meta_resolve`, `meta_check`, `label_disambiguate`). Call them from any new consumer function rather than reading `cd_variables()` directly — per-function copies are what three review rounds kept finding broken (#92). Anything that prints a label per variable passes it through `label_disambiguate()`, which appends ` (variable)` to a `long_name` several stations share; `cd_summary()` and `cd_plot_comparison()` both do (#98). +**The consumer chain is not ERA5-only.** `cd_baseline()` → `cd_anomaly()` → `cd_trend()` → `cd_summary()` / `cd_compare()` take any series in the long format (`variable`, `period`, `year`, `value`, optional `anomaly_type`/`unit`/`long_name`) — other packages (wet's streamflow) target it. The contract lives in `?cd_anomaly`; the rules live in four helpers in `R/cd_anomaly.R` (`series_check`, `meta_resolve`, `meta_check`, `label_disambiguate`). Call them from any new consumer function rather than reading `cd_variables()` directly — per-function copies are what three review rounds kept finding broken (#92). Anything that labels several variables side by side (a table, facets) passes the labels through `label_disambiguate()`, which appends ` (variable)` to a `long_name` several stations share; `cd_summary()` and `cd_plot_comparison()` both do (#98). `cd_plot_timeseries()` does not — it plots one variable the caller chose, so the station belongs in its `title`. **A trend table can mix scales.** Since #97 every `cd_trend()` row carries `trend_on` (`"value"` / `"anomaly"`), and `bind_rows(cd_trend(x), cd_trend(ano))` is a legal input. Any consumer of a trend table reads it with `col_or_na(trend, "trend_on")`: a missing or `NA` `trend_on` (hand-built tables, tables saved before 0.5.2) must not error. `cd_summary()` reads a missing `trend_on` as anomaly; `cd_plot_timeseries()` draws it on either scale (#103). diff --git a/R/cd_anomaly.R b/R/cd_anomaly.R index 3be4f91..e791986 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -194,8 +194,8 @@ meta_resolve <- function(x, raw = FALSE) { #' it held for every small input enumerated in #98. A variable name built to #' collide (`"a) (a"` beside `"a"`) can make labels that never settle, so a label #' still shared after `max_passes` aborts rather than printing two variables -#' under one name. The one place the rule lives; every consumer that -#' prints labels calls it. +#' under one name. The one place the rule lives; every consumer that labels +#' several variables side by side (a table, facets) calls it. #' @noRd label_disambiguate <- function(variable, label, max_passes = nrow(unique(data.frame(v = as.character(variable), l = label)))) { diff --git a/R/cd_plot_timeseries.R b/R/cd_plot_timeseries.R index eb3f21e..4c836c5 100644 --- a/R/cd_plot_timeseries.R +++ b/R/cd_plot_timeseries.R @@ -9,7 +9,9 @@ #' plotted column's name. `unit` is the #' anomaly's unit, so on raw `value` input it is shown only where the #' anomaly type is `absolute` or `pct_point_diff`, whose anomaly unit is -#' the unit of the values. +#' the unit of the values. Unlike [cd_summary()], a `long_name` shared by +#' several variables is not suffixed with the variable: the plot shows one +#' variable, so name a station in `title`. #' #' @param x A tibble from [cd_anomaly()] with columns `variable`, #' `period`, `year`, `anomaly`, optionally `anomaly_type`, `unit` and diff --git a/man/cd_plot_timeseries.Rd b/man/cd_plot_timeseries.Rd index beba74d..3b73410 100644 --- a/man/cd_plot_timeseries.Rd +++ b/man/cd_plot_timeseries.Rd @@ -51,7 +51,9 @@ as \code{\link[=cd_summary]{cd_summary()}} (see the input contract in \code{\lin plotted column's name. \code{unit} is the anomaly's unit, so on raw \code{value} input it is shown only where the anomaly type is \code{absolute} or \code{pct_point_diff}, whose anomaly unit is -the unit of the values. +the unit of the values. Unlike \code{\link[=cd_summary]{cd_summary()}}, a \code{long_name} shared by +several variables is not suffixed with the variable: the plot shows one +variable, so name a station in \code{title}. } \examples{ \dontrun{ diff --git a/planning/active/review-round3.md b/planning/active/review-round3.md new file mode 100644 index 0000000..04e57e4 --- /dev/null +++ b/planning/active/review-round3.md @@ -0,0 +1,52 @@ +# Review round 3: #98 (label_disambiguate, cd_summary Trend on) + +Reviewer: code-check round 3, 2026-09-30. Worked in a scratchpad copy of the repo. The three changed test files pass there with `NOT_CRAN=true`: anomaly 63, summary 73, plot_comparison 13, with 0 failed, 0 errors and 0 skipped. + +## Mechanism + +Every earlier finding rests on one assumption: that `variable` is what makes a printed thing unique. The code and its prose assumed one variable meant one series, one label and one row, and reasoned about distinctness from that. Nobody enumerated the real map from row key to printed label. The row key is `(variable, period, trend_on, trend_start, region)`, and the map from it to a label is many-to-many. Here is how each finding broke the assumption: + +- **Loop ended with a label still shared.** The code assumed one suffix per variable makes that variable's label unique. +- **`n_distinct(variable)` bound.** It assumed one label per variable, but a variable can carry a different label in each period. +- **Docs advised binding per-region tables before summarising.** They assumed `variable` identifies a series across regions. +- **`"a) (a"` never settles.** It assumed `label + " (v)"` can never equal another variable's label. + +Where the mechanism reaches in this diff, with a verdict for each: + +1. **`label_disambiguate()` "shared" test** (R/cd_anomaly.R `shared_find`). It is keyed on distinct `(variable, label)` pairs, so one variable repeated across periods, scales or starts is correctly not a collision. **OK.** +2. **Pass bound = distinct `(variable, label)` pairs.** I ran a stress test in the copy that the enumeration did not cover: + - 20,000 random inputs, each with 2-4 variables drawn from `a, b, c, q1, q_2, ab`. + - One of those names equals the base label (`"a"`). + - 2-7 rows each, with labels made of 0-4 random `(variable)` suffixes. + + Results: the bound was never exceeded, every input settled, and the worst case needed one pass fewer than the pair count. **OK** for names without parentheses. The contrived case is accepted and pinned. +3. **Abort after the loop.** It fires, and the test pins the rendered label (`"still shared: Q \\(a\\)"`), not only a field name. **OK.** +4. **`max_passes` default.** It is evaluated lazily, before `label` changes. It is 0 on empty input, which returns `character(0)`. **OK.** +5. **Factor `variable`.** It is converted with `as.character()` before any use, and a test covers it. **OK.** +6. **`cd_summary()` row identity.** Stations are told apart by the suffix and scales by `Trend on`. Rows that still match are the `trend_start` rows told apart only by `Years` (#106, out of scope) and bound summaries (documented). **OK.** +7. **`Trend on` rule.** It uses the same expression as the `Unit` rule (`col_or_na(trend, "trend_on") %in% "value"`), so there is no second derivation. It is decided for the whole table, which is a superset of the per-series need and harmless. Zero rows gives no column. **OK.** +8. **`cd_plot_comparison()`.** Facets are keyed on variable plus label, and labels go through the helper. **OK.** +9. **`cd_summary` roxygen and Rd.** They match the code, and the Rd is regenerated. **OK.** +10. **`cd_plot_comparison` roxygen and Rd.** **OK.** +11. **The helper's roxygen ("every consumer that prints labels calls it") and CLAUDE.md ("Anything that prints a label per variable passes it through `label_disambiguate()`").** **FALSE for `cd_plot_timeseries()`.** See the finding below. +12. **`cd_plot_timeseries` roxygen ("resolved by the same rules as `cd_summary()`").** It now differs from `cd_summary()` for a shared `long_name`. This is part of the same finding. +13. **Claims made in test comments.** I checked each one: + - "two variables, three passes": traced by hand; it needs 3 passes over 4 pairs. + - "registry long_names are unique": asserted in the test and true for all 15. + - The crafted `"a) (a"` case aborts as the test states. + + **OK.** + +## Findings + +- **[severity: fragile]** CLAUDE.md (consumer-chain paragraph) and R/cd_anomaly.R (the `label_disambiguate` roxygen: "every consumer that prints labels calls it"). Both state a universal rule that `cd_plot_timeseries()` does not follow. It builds its y-axis label from `meta$long_name[1]` (R/cd_plot_timeseries.R:73) and never calls `label_disambiguate()`, although it receives the full `x` with every variable. I measured this in the copy with two stations sharing `long_name = "Mean discharge"`: + - `cd_summary()` gives `"Mean discharge (q_site1)"` and `"Mean discharge (q_site2)"`. + - `cd_plot_timeseries(x, "q_site1")` and `cd_plot_timeseries(x, "q_site2")` both give the y label `"Mean discharge (m3/s)"`. + + So a station's figure and its table row name it differently, and two station figures are labelled identically. The `cd_plot_timeseries` roxygen also still says its label is "resolved by the same rules as `cd_summary()`". The code is not wrong, because each plot shows one variable the caller chose. The problem is that the new shared-helper rule overclaims its reach, and a later contributor would trust it. There are two fixes: + - Narrow both sentences, for example "anything that labels several variables side by side". + - Or pass the timeseries label through `label_disambiguate(x$variable, …)` over the full `x` and pick the plotted variable's label. + +No bugs or security issues found in the code paths changed by this diff. + +/Users/airvine/Projects/repo/cd/planning/active/review-round3.md From a98fd13d02bbcb15f73e124ae9052a5ecbab5267 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 08:15:24 -0700 Subject: [PATCH 10/11] Archive planning files for issue #98 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- .../README.md | 13 +++++++++++++ .../findings.md | 0 .../label_disambiguate_enum.R | 0 .../progress.md | 13 +++++++++++++ .../review-plan.md | 0 .../review-round1.md | 0 .../review-round2.md | 0 .../review-round3.md | 0 .../task_plan.md | 6 +++--- 9 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 planning/archive/2026-09-issue-98-summary-station-labels/README.md rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/findings.md (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/label_disambiguate_enum.R (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/progress.md (57%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/review-plan.md (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/review-round1.md (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/review-round2.md (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/review-round3.md (100%) rename planning/{active => archive/2026-09-issue-98-summary-station-labels}/task_plan.md (93%) diff --git a/planning/archive/2026-09-issue-98-summary-station-labels/README.md b/planning/archive/2026-09-issue-98-summary-station-labels/README.md new file mode 100644 index 0000000..5dd7940 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/README.md @@ -0,0 +1,13 @@ +## Outcome + +`cd_summary()` keeps rows distinguishable (#98). A `long_name` shared by several variables (one label on many stations) gets ` (variable)` appended, by a rule lifted out of `cd_plot_comparison()` into one internal helper, `label_disambiguate()` in `R/cd_anomaly.R`, which both now call. A table that mixes raw-value and anomaly trends gains a `Trend on` column (`Value`/`Anomaly`, the same `%in% "value"` rule as `Unit`); single-scale tables keep their shape, so the vignettes are unchanged. The user chose the conditional column at the plan gate over always adding it or suffixing `Parameter`. What was learned: the helper's repeat loop was bounded by reasoning three times and wrong twice — the plan review caught a silent residual collision, code-check round 1 a bound (variables) too small for one variable carrying different labels by period, round 2 a variable-name shape (`a) (a`) that never settles and so aborts. The shared mechanism, named in round 3, is assuming `variable` makes a printed thing unique; the loop ended on enumeration, not on a reviewer's say-so. A third source of look-alike rows (several `trend_start` values) is filed as #106. + +## Measurement + +Exhaustive enumeration of `label_disambiguate()` (`label_disambiguate_enum.R`): every set of 1–4 distinct (variable, label) pairs over variables `a`, `b`, `a) (b` and 12 chained-suffix labels — 66,711 sets, 0 aborts, 0 labels shared by two variables at the pair-count bound. At the first bound (`n_distinct(variable)`) the round-1 reviewer's 40,000-draw fuzz aborted 11 times; at the pair count, 0. 969 of the 66,711 sets merge two labels of one variable across periods (accepted: rows differ by `Period`, as on main). Round 2 and 3 stress runs (50,000 random inputs) never exceeded the bound. Final suite: 395 pass, 0 fail. + +## Evidence + +`review-plan.md`, `review-round*.md`, `label_disambiguate_enum.R` in this directory. + +Closed by: PR (see `gh pr list --search 98`) diff --git a/planning/active/findings.md b/planning/archive/2026-09-issue-98-summary-station-labels/findings.md similarity index 100% rename from planning/active/findings.md rename to planning/archive/2026-09-issue-98-summary-station-labels/findings.md diff --git a/planning/active/label_disambiguate_enum.R b/planning/archive/2026-09-issue-98-summary-station-labels/label_disambiguate_enum.R similarity index 100% rename from planning/active/label_disambiguate_enum.R rename to planning/archive/2026-09-issue-98-summary-station-labels/label_disambiguate_enum.R diff --git a/planning/active/progress.md b/planning/archive/2026-09-issue-98-summary-station-labels/progress.md similarity index 57% rename from planning/active/progress.md rename to planning/archive/2026-09-issue-98-summary-station-labels/progress.md index 8e9beeb..ab85dc6 100644 --- a/planning/active/progress.md +++ b/planning/archive/2026-09-issue-98-summary-station-labels/progress.md @@ -15,3 +15,16 @@ row-names warning from `labels["a"]` in `cd_plot_comparison()`) - `pkgdown::check_pkgdown()` aborts on an existing DESCRIPTION URL check (github.io url missing), unrelated to this branch; no new export + +### /code-check (whole branch, 2026-09-30) + +| Round | Findings | Fixed | Accepted | Inside previous fix? | +|-------|----------|-------|----------|----------------------| +| 1 | 2 | 2 | 0 | y — pass bound from the plan-review fix | +| 2 | 0 (1 note) | 1 (comment scope) | 1 (`a) (a` aborts) | y — the round-1 bound's comment overclaimed | +| 3 | 1 | 1 | 0 | n — CLAUDE.md reach claim from Phase 4 | + +Ended by enumeration: 66,711 input sets (0 aborts, 0 cross-variable shares) plus round 3's +list of the 12 places the mechanism ("`variable` makes a printed thing unique") reaches — +11 OK, 1 fixed. Commits `bb6718d`, `76a4428`, `40c18bd`. Final suite: +`[ FAIL 0 | WARN 6 | SKIP 0 | PASS 395 ]`. Reviewers spent: 1 plan review + 3 code-check. diff --git a/planning/active/review-plan.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-plan.md similarity index 100% rename from planning/active/review-plan.md rename to planning/archive/2026-09-issue-98-summary-station-labels/review-plan.md diff --git a/planning/active/review-round1.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round1.md similarity index 100% rename from planning/active/review-round1.md rename to planning/archive/2026-09-issue-98-summary-station-labels/review-round1.md diff --git a/planning/active/review-round2.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round2.md similarity index 100% rename from planning/active/review-round2.md rename to planning/archive/2026-09-issue-98-summary-station-labels/review-round2.md diff --git a/planning/active/review-round3.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round3.md similarity index 100% rename from planning/active/review-round3.md rename to planning/archive/2026-09-issue-98-summary-station-labels/review-round3.md diff --git a/planning/active/task_plan.md b/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md similarity index 93% rename from planning/active/task_plan.md rename to planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md index 1d795da..9ab1c8c 100644 --- a/planning/active/task_plan.md +++ b/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md @@ -46,6 +46,6 @@ NEWS + version bump are left to `/gh-pr-merge`, per the repo workflow (0.5.4, pa ## Validation - [x] Tests pass -- [ ] `/code-check` clean on each commit -- [ ] PWF checkboxes match landed work -- [ ] `/planning-archive` on completion +- [x] `/code-check` clean on each commit — run once over the whole branch (3 rounds + enumeration) rather than per commit; fixes landed as follow-up commits +- [x] PWF checkboxes match landed work +- [x] `/planning-archive` on completion From f4817f6f8aee3c757b4863d8105228ed50ecb8bb Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 08:16:14 -0700 Subject: [PATCH 11/11] Archive: stop a finding number autolinking to issue #3 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- .../2026-09-issue-98-summary-station-labels/task_plan.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md b/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md index 9ab1c8c..db2ea96 100644 --- a/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md +++ b/planning/archive/2026-09-issue-98-summary-station-labels/task_plan.md @@ -27,7 +27,7 @@ ## Phase 3: cd_summary() carries the scale when a table mixes them - [x] Scale = `col_or_na(trend, "trend_on") %in% "value"` — the `Unit` rule; anything else, - NA included, reads as anomaly (plan review #3). When it has more than one distinct value, add + NA included, reads as anomaly (plan review, finding 3). When it has more than one distinct value, add `Trend on` (`"Value"`/`"Anomaly"`) after `Period`; otherwise the output shape is unchanged. - [x] Tests: mixed table (`bind_rows(cd_trend(x), cd_trend(ano))` for tmean — identical Unit, the case the issue names) → `Trend on` present with `c("Value", "Anomaly")`; single-scale