diff --git a/CLAUDE.md b/CLAUDE.md index c74785c..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 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 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 8ecd8cd..e791986 100644 --- a/R/cd_anomaly.R +++ b/R/cd_anomaly.R @@ -185,6 +185,41 @@ 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. 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; +#' 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 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)))) { + variable <- as.character(variable) + shared_find <- function(label) { + lab <- unique(data.frame(variable = variable, label = label)) + 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 +} + #' 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..097c04e 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()]. 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")`. @@ -34,10 +35,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/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/R/cd_summary.R b/R/cd_summary.R index a019df8..259e055 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 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, #' 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( @@ -43,7 +55,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 |> @@ -58,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_plot_comparison.Rd b/man/cd_plot_comparison.Rd index 3d0406b..9d23e44 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()}}. Each variable gets its own facet whatever the +labels.} \item{title}{Optional plot title.} 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/man/cd_summary.Rd b/man/cd_summary.Rd index a57ac5c..1b3c0e2 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 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/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/archive/2026-09-issue-98-summary-station-labels/findings.md b/planning/archive/2026-09-issue-98-summary-station-labels/findings.md new file mode 100644 index 0000000..00966b4 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/findings.md @@ -0,0 +1,61 @@ +# 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 | +|-------|------------| + +## 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. + +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/archive/2026-09-issue-98-summary-station-labels/label_disambiguate_enum.R b/planning/archive/2026-09-issue-98-summary-station-labels/label_disambiguate_enum.R new file mode 100644 index 0000000..380cfe1 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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/archive/2026-09-issue-98-summary-station-labels/progress.md b/planning/archive/2026-09-issue-98-summary-station-labels/progress.md new file mode 100644 index 0000000..ab85dc6 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/progress.md @@ -0,0 +1,30 @@ +# 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 + +- 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 + +### /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/archive/2026-09-issue-98-summary-station-labels/review-plan.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-plan.md new file mode 100644 index 0000000..843c1a5 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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 #106 | +| — | Acceptance | Combined stations × scales × region test | Added | diff --git a/planning/archive/2026-09-issue-98-summary-station-labels/review-round1.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round1.md new file mode 100644 index 0000000..1d79dff --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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/planning/archive/2026-09-issue-98-summary-station-labels/review-round2.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round2.md new file mode 100644 index 0000000..c809d2e --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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/planning/archive/2026-09-issue-98-summary-station-labels/review-round3.md b/planning/archive/2026-09-issue-98-summary-station-labels/review-round3.md new file mode 100644 index 0000000..04e57e4 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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 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 new file mode 100644 index 0000000..db2ea96 --- /dev/null +++ b/planning/archive/2026-09-issue-98-summary-station-labels/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 +- [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. +- [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). +- [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). + +## Phase 2: cd_summary() disambiguates stations +- [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()` + for the same variables. + +## 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, 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 + and no-`trend_on` tables → `expect_named()` unchanged; value + `NA` trend_on → column + present, NA row reads `"Anomaly"`; `region_name` still last. +- [x] Roxygen: `@return` and description name both the ` (variable)` suffix and the conditional + `Trend on` column; `devtools::document()`. + +## Phase 4: Docs +- [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. +- [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 + +- [x] Tests pass +- [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 diff --git a/tests/testthat/test-cd_anomaly.R b/tests/testthat/test-cd_anomaly.R index f5f0394..ab6f437 100644 --- a/tests/testthat/test-cd_anomaly.R +++ b/tests/testthat/test-cd_anomaly.R @@ -273,3 +273,61 @@ 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)") + ) +}) + +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 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), + "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" + ) +}) 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", { diff --git a/tests/testthat/test-cd_summary.R b/tests/testthat/test-cd_summary.R index f49e81c..639976b 100644 --- a/tests/testthat/test-cd_summary.R +++ b/tests/testthat/test-cd_summary.R @@ -214,3 +214,99 @@ 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))) +}) + +# 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")) +})