From ff393b6df4f828239ec9035522a0064871548775 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 09:29:35 -0700 Subject: [PATCH 1/4] Initialize PWF baseline for #106 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- planning/active/findings.md | 26 +++++++++++++++++++++++++ planning/active/progress.md | 8 ++++++++ planning/active/task_plan.md | 37 ++++++++++++++++++++++++++++++++++++ 3 files changed, 71 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..5376966 --- /dev/null +++ b/planning/active/findings.md @@ -0,0 +1,26 @@ +# Findings — cd_summary(): trends from several trend_start values differ only by Years (#106) + +## Issue context + +**If we do it:** a `cd_summary()` table over `cd_trend(x, trend_start = c(1951, 1981))` says which row is which window. **If we never do:** two rows read `Mean temperature / Annual` with different slopes, told apart only by `Years`, which a reader has to subtract from the current year to decode. + +## Problem + +`cd_trend()` accepts several `trend_start` values (`R/cd_trend.R`), and `cd_summary()` drops `trend_start`. The rows keep `Years` (`n_years`), which varies with the start, but nothing names the window. Same family as #98: #98 kept stations apart (` (variable)` suffix) and raw vs anomaly trends apart (a conditional `Trend on` column). + +## Proposed Solution + +Carry the start the way #98 carries the scale: a `Start` (or `Window`) column added only when the table holds more than one `trend_start`, so single-window tables keep their shape. + +Found by the plan review for #98. + +## Exploration (2026-09-30) + +- Both vignettes render `cd_summary(trn)` over `cd_trend(ano, trend_start = c(1951, 1981))` (`peace-fwcp.Rmd:226`, `kootenay-lake.Rmd:234`) — the defect is live on the published site; the fix changes those tables (gains `Start`). +- `col_or_na()` (`R/cd_anomaly.R:136`) returns character; use it for detection only, copy values from `trend$trend_start`. +- Example data (`example_catalog.json` + `example_aoi.gpkg`): tmean only, years 1951–1960. + +## Errors Encountered + +| Error | Resolution | +|-------|------------| diff --git a/planning/active/progress.md b/planning/active/progress.md new file mode 100644 index 0000000..f6c2ce0 --- /dev/null +++ b/planning/active/progress.md @@ -0,0 +1,8 @@ +# Progress — cd_summary(): trends from several trend_start values differ only by Years (#106) + +## Session 2026-09-30 + +- Plan-mode exploration — phases approved by user +- Created branch `106-cd-summary-trends-from-several-trend-sta` off main +- Scaffolded PWF baseline from issue #106 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..46f203d --- /dev/null +++ b/planning/active/task_plan.md @@ -0,0 +1,37 @@ +# Task: cd_summary(): trends from several trend_start values differ only by Years (#106) + +`cd_trend()` accepts several `trend_start` values (`R/cd_trend.R`), and `cd_summary()` drops `trend_start`. The rows keep `Years` (`n_years`), which varies with the start, but nothing names the window. Same family as #98: #98 kept stations apart (` (variable)` suffix) and raw vs anomaly trends apart (a conditional `Trend on` column). + +## Decisions (approved at plan gate) +- **Column name `Start`, holding the start year** (e.g. `1951`). Not `Window "1951–2025"`: the end year isn't in the trend table, and `trend_start + n_years - 1` is wrong when the series has gaps. +- **Position:** identifier columns first — `Parameter, Period, [Trend on], [Start], Slope, Years, …`. +- **Detection:** `col_or_na(trend, "trend_start")` (`R/cd_anomaly.R:136`), counted over the whole table like `Trend on`. Missing column → no `Start`, no error (hand-built tables). `NA` beside a real start counts as distinct and shows `NA`. Values copied from `trend$trend_start` unchanged (keeps numeric type; `col_or_na` returns character). +- Summaries bound together (one per region) can differ in having `Start`, as #98 already documents for `Trend on`. + +## Phase 1: Failing tests (`tests/testthat/test-cd_summary.R`) +- [ ] Two starts (`cd_trend(raw_series("tmean"), c(2000, 2004))`) → `Start` after `Period`, values match `trend_start`, `Parameter/Period/Start` unique +- [ ] One start → no `Start` column (existing shape tests still pass; add explicit case) +- [ ] No `trend_start` column at all, and a 0-row table → no `Start`, no error +- [ ] `NA` start beside a real one → `Start` present, `NA` shown +- [ ] Mixed scales × two starts × region → order `Parameter, Period, Trend on, Start, …, Region`; rows unique on all four identifiers + +## Phase 2: Implement +- [ ] `cd_summary()`: add the `Start` block after the `Trend on` block, `.after` = `Trend on` if present else `Period` +- [ ] Roxygen: extend the "Rows are kept distinguishable" paragraph and `@return`; add an example `cd_summary(cd_trend(ts, trend_start = c(1951, 1956)))` (example data spans 1951–1960) +- [ ] `devtools::document()` + +## Phase 3: Docs + vignettes +- [ ] `CLAUDE.md` "A trend table can mix scales" paragraph: one sentence that `cd_summary()` also names the window (`Start`) when starts differ +- [ ] Render the `trend-table` chunk of both vignettes from their committed `.rds` and confirm `Start` appears and rows are unique (no data regen needed — `trn` already carries `trend_start`) + +## Phase 4: Verify +- [ ] `devtools::test()` all green; `lintr::lint_package()` clean; `pkgdown::check_pkgdown()` +- [ ] `/code-check` on each commit (with Plan-agent review of the task_plan run concurrently after baseline) + +NEWS/version bump left to `/gh-pr-merge`, as for #98. + +## Validation +- [ ] Tests pass +- [ ] `/code-check` clean on each commit +- [ ] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion From df11d7483f51f3f1f44fb70d12e1688aeabcdc0d Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 09:38:24 -0700 Subject: [PATCH 2/4] cd_summary(): add Start when a table holds several trend windows A table from cd_trend(x, trend_start = c(1951, 1981)) gave two rows per variable and period that differed only by Years. cd_summary() now adds a Start column (the requested trend_start) when the table holds more than one, after Period or Trend on, mirroring the conditional Trend on column (#98). Single-window tables keep their shape. Fixes #106 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- R/cd_summary.R | 29 ++++++++++++++--- man/cd_summary.Rd | 20 +++++++++--- planning/active/findings.md | 12 +++++++ planning/active/progress.md | 3 ++ planning/active/review-round1.md | 14 ++++++++ planning/active/task_plan.md | 16 +++++----- tests/testthat/test-cd_summary.R | 55 ++++++++++++++++++++++++++++++++ 7 files changed, 133 insertions(+), 16 deletions(-) create mode 100644 planning/active/review-round1.md diff --git a/R/cd_summary.R b/R/cd_summary.R index 259e055..498beca 100644 --- a/R/cd_summary.R +++ b/R/cd_summary.R @@ -21,10 +21,17 @@ #' 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. +#' `"Anomaly"`). A table on one scale has no such column. Likewise a table +#' holding several trend windows, such as +#' `cd_trend(x, trend_start = c(1951, 1981))`, gains a `Start` column: the +#' start year asked of [cd_trend()], not the first year with data. It is +#' added when the table as a whole holds more than one `trend_start` (an `NA` +#' counts as one), so binding a 1991 trend of one station to a 2000 trend of +#' another adds it too; a table with one window, or no `trend_start` column, +#' has none. All three 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` or `Start` 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, @@ -33,6 +40,8 @@ #' @return A tibble with columns `Parameter`, `Period`, `Slope`, `Years`, #' `Total Change`, `Unit`, `p-value`, and optionally `Region`. When `trend` #' mixes raw-value and anomaly trends, a `Trend on` column follows `Period`. +#' When it holds more than one `trend_start`, a `Start` column follows +#' `Period` (or `Trend on`). #' #' @examples #' catalog <- cd_catalog( @@ -51,6 +60,9 @@ #' # Add region label for multi-AOI reports #' cd_summary(trn, region_name = "Example AOI") #' +#' # Two trend windows: a Start column says which row is which +#' cd_summary(cd_trend(ts, trend_start = c(1951, 1956))) +#' #' @export cd_summary <- function(trend, region_name = NULL) { trend <- dplyr::ungroup(trend) @@ -83,6 +95,15 @@ cd_summary <- function(trend, region_name = NULL) { ) } + # Trends over several windows (cd_trend(x, trend_start = c(1951, 1981))) + # otherwise differ only by Years. An NA start counts as a window of its own. + if (length(unique(col_or_na(trend, "trend_start"))) > 1) { + out <- tibble::add_column( + out, Start = trend$trend_start, + .after = if ("Trend on" %in% names(out)) "Trend on" else "Period" + ) + } + if (!is.null(region_name)) { out$Region <- region_name } diff --git a/man/cd_summary.Rd b/man/cd_summary.Rd index 1b3c0e2..767bc8a 100644 --- a/man/cd_summary.Rd +++ b/man/cd_summary.Rd @@ -16,6 +16,8 @@ adds a \code{Region} column.} A tibble with columns \code{Parameter}, \code{Period}, \code{Slope}, \code{Years}, \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}. +When it holds more than one \code{trend_start}, a \code{Start} column follows +\code{Period} (or \verb{Trend on}). } \description{ Joins trend statistics with variable metadata and computes Total @@ -40,10 +42,17 @@ Rows are kept distinguishable. A \code{long_name} shared by several variables 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. +\code{"Anomaly"}). A table on one scale has no such column. Likewise a table +holding several trend windows, such as +\code{cd_trend(x, trend_start = c(1951, 1981))}, gains a \code{Start} column: the +start year asked of \code{\link[=cd_trend]{cd_trend()}}, not the first year with data. It is +added when the table as a whole holds more than one \code{trend_start} (an \code{NA} +counts as one), so binding a 1991 trend of one station to a 2000 trend of +another adds it too; a table with one window, or no \code{trend_start} column, +has none. All three 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} or \code{Start} column present in only some of them +is \code{NA} for the rest. } \examples{ catalog <- cd_catalog( @@ -62,4 +71,7 @@ cd_summary(trn) # Add region label for multi-AOI reports cd_summary(trn, region_name = "Example AOI") +# Two trend windows: a Start column says which row is which +cd_summary(cd_trend(ts, trend_start = c(1951, 1956))) + } diff --git a/planning/active/findings.md b/planning/active/findings.md index 5376966..42cf23a 100644 --- a/planning/active/findings.md +++ b/planning/active/findings.md @@ -24,3 +24,15 @@ Found by the plan review for #98. | Error | Resolution | |-------|------------| + +## Plan review (Plan agent, 2026-09-30) — no blockers + +Folded in: roxygen now says `Start` is the *requested* start year (not first data year) and is decided over the whole table (a 1991 trend of one station bound to a 2000 trend of another gains `Start`); grouped input with several starts tested. + +Not acted on, with reasons: +- Factor/character `trend_start` is copied unchanged, so per-region summaries with mixed types fail in `bind_rows`. `cd_trend()` always emits numeric; a hand-built factor start is out of contract. +- Rows still collide for anomaly runs against different baselines (baseline not stored), for tables bound across AOIs before summarising, and for periods differing only in case. None is a `trend_start` problem. +- `cd_trend()` returning a 0x0 tibble breaks `cd_summary()` — already filed as #101. +- Vignette tables list all 1951 rows then all 1981 rows (grid order), so comparing windows means scrolling 59 rows. Handled in Phase 3 (hidden chunk only). + +Pre-existing, not touched: 6 test warnings in `test-cd_plot_comparison.R` ("row names were found from a short variable"); `pkgdown::check_pkgdown()` aborts on main too (DESCRIPTION URL lacks the github.io url — custom domain); `object_usage_linter` flags `col_or_na` in `R/cd_summary.R` on main's line too. diff --git a/planning/active/progress.md b/planning/active/progress.md index f6c2ce0..0d8883b 100644 --- a/planning/active/progress.md +++ b/planning/active/progress.md @@ -6,3 +6,6 @@ - Created branch `106-cd-summary-trends-from-several-trend-sta` off main - Scaffolded PWF baseline from issue #106 with approved phases - Next: start Phase 1 +- Phase 1+2: 5 failing tests (7 expectations) → `Start` block in `cd_summary()`, roxygen + example; suite 410 pass +- Plan review landed: no blockers; three items folded in (see findings) +- Code-check round 1: Clean (`review-round1.md`) diff --git a/planning/active/review-round1.md b/planning/active/review-round1.md new file mode 100644 index 0000000..aedd7c5 --- /dev/null +++ b/planning/active/review-round1.md @@ -0,0 +1,14 @@ +# Code review — round 1 (#106, staged diff) + +## Clean +No issues found. + +Checked (probes read-only, against the source tree via `devtools::load_all()`): + +- `NOT_CRAN=true` `test_file("tests/testthat/test-cd_summary.R")`: all pass, no skips (Kendall/zyp installed), so the new `skip_if_not_installed` blocks do run. +- New `@examples` line `cd_summary(cd_trend(ts, trend_start = c(1951, 1956)))` runs on the bundled example data (years 1951-1960) and returns 2 rows with `Start` = 1951/1956, Years 10/5. +- Detection (`col_or_na()` -> character) and display (`trend$trend_start`, raw) read the same column; they can only disagree for numeric starts that differ below 15 significant digits, which is not a realistic input. Column absent -> all-NA -> length 1 -> no `Start`, and `trend$trend_start` is never evaluated, so no `$` partial-match exposure. 0-row input -> `unique(character(0))` has length 0 -> no column, no `add_column` recycling error. +- Row alignment: `out` comes from `mutate()`/`select()` on the ungrouped `trend`, so row order matches `trend$trend_start`. Grouped input is ungrouped on line 68 before either is read. +- `.after` handles the with/without `Trend on` cases; `Region` is appended afterwards, so the order `Parameter, Period, Trend on, Start, ..., Region` holds (and is tested). +- Tests would go red with the block removed (`expect_named` including `Start`), so they pin the change. +- Downstream consumers: the two vignettes pass `cd_summary()` output straight to `knitr::kable()` without positional column names or `col.names`, so the extra column cannot misalign anything; no other R function consumes `cd_summary()` output. diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index 46f203d..bbc4668 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -9,16 +9,16 @@ - Summaries bound together (one per region) can differ in having `Start`, as #98 already documents for `Trend on`. ## Phase 1: Failing tests (`tests/testthat/test-cd_summary.R`) -- [ ] Two starts (`cd_trend(raw_series("tmean"), c(2000, 2004))`) → `Start` after `Period`, values match `trend_start`, `Parameter/Period/Start` unique -- [ ] One start → no `Start` column (existing shape tests still pass; add explicit case) -- [ ] No `trend_start` column at all, and a 0-row table → no `Start`, no error -- [ ] `NA` start beside a real one → `Start` present, `NA` shown -- [ ] Mixed scales × two starts × region → order `Parameter, Period, Trend on, Start, …, Region`; rows unique on all four identifiers +- [x] Two starts (`cd_trend(raw_series("tmean"), c(2000, 2004))`) → `Start` after `Period`, values match `trend_start`, `Parameter/Period/Start` unique +- [x] One start → no `Start` column (existing shape tests still pass; add explicit case) +- [x] No `trend_start` column at all, and a 0-row table → no `Start`, no error +- [x] `NA` start beside a real one → `Start` present, `NA` shown +- [x] Mixed scales × two starts × region → order `Parameter, Period, Trend on, Start, …, Region`; rows unique on all four identifiers ## Phase 2: Implement -- [ ] `cd_summary()`: add the `Start` block after the `Trend on` block, `.after` = `Trend on` if present else `Period` -- [ ] Roxygen: extend the "Rows are kept distinguishable" paragraph and `@return`; add an example `cd_summary(cd_trend(ts, trend_start = c(1951, 1956)))` (example data spans 1951–1960) -- [ ] `devtools::document()` +- [x] `cd_summary()`: add the `Start` block after the `Trend on` block, `.after` = `Trend on` if present else `Period` +- [x] Roxygen: extend the "Rows are kept distinguishable" paragraph and `@return`; add an example `cd_summary(cd_trend(ts, trend_start = c(1951, 1956)))` (example data spans 1951–1960) +- [x] `devtools::document()` ## Phase 3: Docs + vignettes - [ ] `CLAUDE.md` "A trend table can mix scales" paragraph: one sentence that `cd_summary()` also names the window (`Start`) when starts differ diff --git a/tests/testthat/test-cd_summary.R b/tests/testthat/test-cd_summary.R index 639976b..a0193df 100644 --- a/tests/testthat/test-cd_summary.R +++ b/tests/testthat/test-cd_summary.R @@ -310,3 +310,58 @@ 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")) }) + +# Tables holding several trend windows (#106) ----------------------------- + +test_that("cd_summary adds Start when a table holds several trend_start (#106)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + trn <- cd_trend(raw_series("tmean"), trend_start = c(2000, 2004)) + smry <- cd_summary(trn) + expect_named(smry, c("Parameter", "Period", "Start", cols_summary[-(1:2)])) + expect_equal(smry$Start, trn$trend_start) + expect_equal(smry$Years, c(10, 6)) + expect_equal(anyDuplicated(smry[c("Parameter", "Period", "Start")]), 0) + expect_identical(cd_summary(dplyr::group_by(trn, variable)), smry) +}) + +test_that("cd_summary adds no Start column to a table with one trend_start (#106)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + expect_named(cd_summary(cd_trend(raw_series("tmean"), trend_start = 2000)), cols_summary) + # station_trend() has one start across three rows + expect_named(cd_summary(station_trend()), cols_summary) +}) + +test_that("cd_summary needs no trend_start column (#106)", { + trend <- station_trend() + trend$trend_start <- NULL + expect_named(cd_summary(trend), cols_summary) + expect_named(cd_summary(trend[0, ]), cols_summary) + expect_named(cd_summary(station_trend()[0, ]), cols_summary) +}) + +test_that("cd_summary shows an NA trend_start beside a real one (#106)", { + trend <- station_trend() + trend$trend_start <- c(2000, NA, 2000) + smry <- cd_summary(trend) + expect_named(smry, c("Parameter", "Period", "Start", cols_summary[-(1:2)])) + expect_equal(smry$Start, c(2000, NA, 2000)) +}) + +test_that("cd_summary keeps scales x windows x region distinct (#106)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- raw_series("tmean") + ano <- cd_anomaly(x, cd_baseline(x, 2000:2004)) + smry <- cd_summary( + dplyr::bind_rows(cd_trend(x, c(2000, 2004)), cd_trend(ano, c(2000, 2004))), + region_name = "AOI" + ) + expect_named( + smry, + c("Parameter", "Period", "Trend on", "Start", cols_summary[-(1:2)], "Region") + ) + expect_equal(nrow(smry), 4) + expect_equal(anyDuplicated(smry[c("Parameter", "Period", "Trend on", "Start")]), 0) +}) From 8175f2838bf810af4f31ede31f9cad3b8c841e4f Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 09:38:24 -0700 Subject: [PATCH 3/4] Vignettes: put each trend window pair on adjacent rows; CLAUDE.md Both vignettes' trend tables now carry Start (#106). The hidden trend-table chunk orders rows by variable, period and start so the 1951 and 1981 rows of each series sit together instead of 59 rows apart. Relates to #106 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- CLAUDE.md | 2 +- planning/active/progress.md | 2 ++ planning/active/review-round2.md | 23 +++++++++++++++++++ planning/active/review-round3.md | 38 ++++++++++++++++++++++++++++++++ planning/active/task_plan.md | 5 +++-- vignettes/kootenay-lake.Rmd | 5 ++++- vignettes/peace-fwcp.Rmd | 5 ++++- 7 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 planning/active/review-round2.md create mode 100644 planning/active/review-round3.md diff --git a/CLAUDE.md b/CLAUDE.md index 0833491..e16d1e6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,7 +33,7 @@ The historical `cd_fetch()` / `cd_derive()` R-side producer functions still ship **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). +**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). A trend table can also hold several windows (`trend_start = c(1951, 1981)`); `cd_summary()` then adds a `Start` column, as it adds `Trend on` for mixed scales — both only when the table needs them, so single-window, single-scale tables keep their shape (#106). ## Function Prefix diff --git a/planning/active/progress.md b/planning/active/progress.md index 0d8883b..2c1eb7f 100644 --- a/planning/active/progress.md +++ b/planning/active/progress.md @@ -9,3 +9,5 @@ - Phase 1+2: 5 failing tests (7 expectations) → `Start` block in `cd_summary()`, roxygen + example; suite 410 pass - Plan review landed: no blockers; three items folded in (see findings) - Code-check round 1: Clean (`review-round1.md`) +- Phase 3: CLAUDE.md sentence; both vignettes render (rmarkdown, load_all) with `Parameter | Period | Start | …`, 118 rows, 0 duplicates on Parameter/Period/Start; hidden chunk puts each window pair on adjacent rows +- Code-check rounds 2 (vignettes/docs) and 3 (adversarial row identity): both Clean; loop ended at round 3 with no finding inside a fix diff --git a/planning/active/review-round2.md b/planning/active/review-round2.md new file mode 100644 index 0000000..464a143 --- /dev/null +++ b/planning/active/review-round2.md @@ -0,0 +1,23 @@ +# Code review — round 2 (#106, staged diff) + +## Clean +No issues found. + +Checked (probes read-only, against the source tree via `devtools::load_all()` and a `cp -r` copy for `document()`): + +- **Chunk labels.** `grep -o '^```{r …' | sort | uniq -d` over both vignettes is empty. The edits added no chunk; `trend-table` is unique in each file. +- **No leakage.** `trn_tbl` appears only inside its own `trend-table` chunk in each vignette, and `trn` is not reassigned. Later consumers of `trn` (`plot-prcp`'s `trend = trn`, `compare-table`'s `trn$trend_start == 1951` filter, `rollup`'s `get_75()`) all subset by value, not position, and still see the unchanged `trn`. +- **Reorder against the shipped data.** Both `inst/vignette-data/*.rds` `trn` hold 118 rows (59 variable/period keys × {1951, 1981}), ungrouped, with no `trend_on` column (pre-0.5.2 save, so all rows read as anomaly and there is no `Trend on`). After the reorder: `rle(paste(variable, period))` gives 59 runs, all of length 2. `cd_summary(trn_tbl)` has columns `Parameter, Period, Start, Slope, …`, `Start` is identical to `trn_tbl$trend_start`, and it has exactly the same rows as `cd_summary(trn)`. The reorder only moves rows. The sort key (match codes plus a numeric start) is unique per row, so the order is deterministic and does not depend on `LC_COLLATE`. +- **Visible recipe versus table.** The shown `cd::cd_summary(trn)` gives the same rows and columns as the table. Only the row order differs (a live `cd_trend()` uses `expand.grid` order). This was a deliberate choice for the hidden chunk and does not break anything. +- **`man/` in sync.** `devtools::document()` in a copy produced no further changes, so `man/cd_summary.Rd` matches the roxygen. +- **Roxygen claims, each probed on the bundled example data:** + - "The start year asked of `cd_trend()`, not the first year with data" holds. `cd_trend()` writes the requested `ts` into `trend_start`, and `cd_trend(ts_tmean, 1940)` records Start 1940 over data that begins in 1951. + - "Decided over the whole table" holds. Detection is `length(unique(col_or_na(trend, "trend_start"))) > 1` on the ungrouped table. + - "An NA counts as one" holds. `unique()` keeps NA as a value. + - "No `trend_start` column → none" holds. `col_or_na` returns all-NA, which has length ≤ 1. + - "A column present in only some bound summaries is NA for the rest" holds. `bind_rows(cd_summary(a, "R1"), cd_summary(bind_rows(a, b), "R2"))` gives `Start` NA for R1. + - `@return` placement ("follows Period (or Trend on)") matches `.after`. +- **CLAUDE.md sentence** is true of the code. `Trend on` is added only when `on_value` takes more than one value, and `Start` only when `trend_start` does. A single-window, single-scale table keeps the 7-column shape (pinned by the existing `cols_summary` tests). +- **R change, fresh read.** `trend$trend_start` is reached only when the column exists (exact `%in% names` check first), so partial matching on a plain data.frame is not a risk. Rows of `out` line up with `trend`, because `mutate`/`select` on the ungrouped table do not reorder them. + +Non-blocking observation, not a defect: the prose "The trend table shows two rows per variable" (peace-fwcp.Rmd:202, kootenay-lake.Rmd:210) means per variable *and period*. It predates this diff, and the new adjacent pairs plus the `Start` column make it read more accurately than before. diff --git a/planning/active/review-round3.md b/planning/active/review-round3.md new file mode 100644 index 0000000..6228c5b --- /dev/null +++ b/planning/active/review-round3.md @@ -0,0 +1,38 @@ +# Code review — round 3 (#106, adversarial: row identity) + +## Clean +No issues found. + +Row identifiers of a `cd_trend()` row: `variable`, `period`, `trend_start`, +`trend_on` (plus baseline and AOI, which are not stored and excluded by the +brief). In `cd_summary()` these map to `Parameter` (via `label_disambiguate()`), +`Period`, `Start`, `Trend on`, `Region`. Constructions run against the source +tree (`devtools::load_all()`, scripts in the session scratchpad `probe3*.R`): + +- Shared `long_name` on two stations × two windows: 4 rows, `Q (q_a)` / `Q (q_b)` × + 2000/2004, `anyDuplicated(Parameter, Period, Start) == 0`. +- Mixed scales with *disjoint* windows (`bind_rows(cd_trend(x, 2000), cd_trend(ano, 2004))`): + both `Trend on` and `Start` added, values aligned. +- Mixed scales × two windows on a plain `data.frame` (not tibble) with `region_name`: + column order `Parameter, Period, Trend on, Start, …, Region`, 4 distinct rows; + `add_column(.after = "Trend on")` works on a data.frame. +- Integer start bound to double start (`cd_trend(x, 2000L)` + `cd_trend(x, 2004)`): `Start` added, correct. +- Requested start before the data (`c(1990, 2000, 2008)`, 2008 window dropped for <3 years): + the two surviving rows have identical stats and are told apart only by `Start` + (1990 / 2000) — the documented "start year asked" behaviour. +- Input grouped by `trend_start`, and `rowwise()` input: identical to ungrouped output. +- `NA` start beside a real one; a `Date` start: shown unchanged, aligned. +- Per-region summaries bound together, one region single-window: that region's + `Start` is `NA` — the documented behaviour, not a wrong value. +- Both vignettes' committed `trn` (`inst/vignette-data/{peace_fwcp,kootenay_lake}.rds`) + after the hidden-chunk reorder: 118 rows each, 0 duplicates on + `Parameter/Period/Start`, `Start` and `Years` equal `trn_tbl$trend_start` / `n_years` row for row. + +No input found where the function now errors and did not before: the new block +only runs when `trend_start` is present with >1 distinct value, `out` and +`trend` share row order, and `out` never already holds a `Start` column. + +Not a finding: `cd_trend(x, c(2000, 2000))` (or binding the same trend twice) +gives two summary rows identical on every column. The input rows are the same +trend twice — `cd_trend()` does not deduplicate `trend_start` — so there is +nothing for `cd_summary()` to distinguish; pre-existing and outside this change. diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md index bbc4668..2b7fccc 100644 --- a/planning/active/task_plan.md +++ b/planning/active/task_plan.md @@ -21,8 +21,9 @@ - [x] `devtools::document()` ## Phase 3: Docs + vignettes -- [ ] `CLAUDE.md` "A trend table can mix scales" paragraph: one sentence that `cd_summary()` also names the window (`Start`) when starts differ -- [ ] Render the `trend-table` chunk of both vignettes from their committed `.rds` and confirm `Start` appears and rows are unique (no data regen needed — `trn` already carries `trend_start`) +- [x] `CLAUDE.md` "A trend table can mix scales" paragraph: one sentence that `cd_summary()` also names the window (`Start`) when starts differ +- [x] Render the `trend-table` chunk of both vignettes from their committed `.rds` and confirm `Start` appears and rows are unique (no data regen needed — `trn` already carries `trend_start`) +- [x] Hidden `trend-table` chunk orders rows by variable, period, start so each 1951/1981 pair is adjacent (plan review: grid order put 59 rows between them) ## Phase 4: Verify - [ ] `devtools::test()` all green; `lintr::lint_package()` clean; `pkgdown::check_pkgdown()` diff --git a/vignettes/kootenay-lake.Rmd b/vignettes/kootenay-lake.Rmd index 04fe71f..fbcb066 100644 --- a/vignettes/kootenay-lake.Rmd +++ b/vignettes/kootenay-lake.Rmd @@ -230,8 +230,11 @@ cd::cd_summary(trn) ``` ```{r trend-table, echo = FALSE} +# Each variable and period's 1951 and 1981 windows on adjacent rows +trn_tbl <- trn[order(match(trn$variable, unique(trn$variable)), + match(trn$period, unique(trn$period)), trn$trend_start), ] kableExtra::kable_styling( - knitr::kable(cd::cd_summary(trn), label = NA, + knitr::kable(cd::cd_summary(trn_tbl), label = NA, caption = "Trend statistics for all variables and periods, Kootenay Lake Region."), bootstrap_options = c("striped", "hover", "condensed") ) |> diff --git a/vignettes/peace-fwcp.Rmd b/vignettes/peace-fwcp.Rmd index 57c6908..f41f261 100644 --- a/vignettes/peace-fwcp.Rmd +++ b/vignettes/peace-fwcp.Rmd @@ -222,8 +222,11 @@ cd::cd_summary(trn) ``` ```{r trend-table, echo = FALSE} +# Each variable and period's 1951 and 1981 windows on adjacent rows +trn_tbl <- trn[order(match(trn$variable, unique(trn$variable)), + match(trn$period, unique(trn$period)), trn$trend_start), ] kableExtra::kable_styling( - knitr::kable(cd::cd_summary(trn), label = NA, + knitr::kable(cd::cd_summary(trn_tbl), label = NA, caption = "Trend statistics for all variables and periods, FWCP Peace Region."), bootstrap_options = c("striped", "hover", "condensed") ) |> From 5c28dc47b2ebc877722737180188b6b1c0b195f9 Mon Sep 17 00:00:00 2001 From: almac2022 Date: Wed, 30 Sep 2026 09:38:44 -0700 Subject: [PATCH 4/4] Archive planning files for issue #106 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GignjoKpZADXywTf6pAMgj --- .../2026-09-issue-106-summary-trend-start/README.md | 13 +++++++++++++ .../findings.md | 0 .../progress.md | 0 .../review-round1.md | 0 .../review-round2.md | 0 .../review-round3.md | 0 .../task_plan.md | 12 ++++++------ 7 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 planning/archive/2026-09-issue-106-summary-trend-start/README.md rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/findings.md (100%) rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/progress.md (100%) rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/review-round1.md (100%) rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/review-round2.md (100%) rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/review-round3.md (100%) rename planning/{active => archive/2026-09-issue-106-summary-trend-start}/task_plan.md (87%) diff --git a/planning/archive/2026-09-issue-106-summary-trend-start/README.md b/planning/archive/2026-09-issue-106-summary-trend-start/README.md new file mode 100644 index 0000000..23c17e9 --- /dev/null +++ b/planning/archive/2026-09-issue-106-summary-trend-start/README.md @@ -0,0 +1,13 @@ +## Outcome + +`cd_summary()` now names the trend window (#106). A trend table holding more than one `trend_start`, such as `cd_trend(x, trend_start = c(1951, 1981))`, gains a `Start` column holding the start year asked of `cd_trend()`. It follows `Period`, or `Trend on` when that is present. The rule is the one #98 used for `Trend on`: the column is decided over the whole table, and only when the table needs it, so single-window tables keep their shape. Both vignettes had shown this defect on the published site: their trend tables gave 59 pairs of rows that differed only by `Years`. They now carry `Start`, and the hidden `trend-table` chunk orders rows so each 1951/1981 pair sits together. The plan review and three code-check rounds (vignettes and docs, then an adversarial search for row collisions) found no defects. The review's scope notes are in `findings.md`. They include rows that still collide for reasons outside this issue (baselines not stored, AOIs bound before summarising) and the pre-existing 0x0 `cd_trend()` failure, which was already filed as #101. + +## Measurement + +On the committed vignette data (`inst/vignette-data/{peace_fwcp,kootenay_lake}.rds`), each summary has 118 rows. Before the change, 59 of those rows duplicated another row on `Parameter`/`Period`. After it, 0 duplicate on `Parameter`/`Period`/`Start`. Final suite: 410 pass, 0 fail. + +## Evidence + +`review-round*.md` in this directory. + +Closed by: PR (see `gh pr list --search 106`) diff --git a/planning/active/findings.md b/planning/archive/2026-09-issue-106-summary-trend-start/findings.md similarity index 100% rename from planning/active/findings.md rename to planning/archive/2026-09-issue-106-summary-trend-start/findings.md diff --git a/planning/active/progress.md b/planning/archive/2026-09-issue-106-summary-trend-start/progress.md similarity index 100% rename from planning/active/progress.md rename to planning/archive/2026-09-issue-106-summary-trend-start/progress.md diff --git a/planning/active/review-round1.md b/planning/archive/2026-09-issue-106-summary-trend-start/review-round1.md similarity index 100% rename from planning/active/review-round1.md rename to planning/archive/2026-09-issue-106-summary-trend-start/review-round1.md diff --git a/planning/active/review-round2.md b/planning/archive/2026-09-issue-106-summary-trend-start/review-round2.md similarity index 100% rename from planning/active/review-round2.md rename to planning/archive/2026-09-issue-106-summary-trend-start/review-round2.md diff --git a/planning/active/review-round3.md b/planning/archive/2026-09-issue-106-summary-trend-start/review-round3.md similarity index 100% rename from planning/active/review-round3.md rename to planning/archive/2026-09-issue-106-summary-trend-start/review-round3.md diff --git a/planning/active/task_plan.md b/planning/archive/2026-09-issue-106-summary-trend-start/task_plan.md similarity index 87% rename from planning/active/task_plan.md rename to planning/archive/2026-09-issue-106-summary-trend-start/task_plan.md index 2b7fccc..4f2c7a2 100644 --- a/planning/active/task_plan.md +++ b/planning/archive/2026-09-issue-106-summary-trend-start/task_plan.md @@ -26,13 +26,13 @@ - [x] Hidden `trend-table` chunk orders rows by variable, period, start so each 1951/1981 pair is adjacent (plan review: grid order put 59 rows between them) ## Phase 4: Verify -- [ ] `devtools::test()` all green; `lintr::lint_package()` clean; `pkgdown::check_pkgdown()` -- [ ] `/code-check` on each commit (with Plan-agent review of the task_plan run concurrently after baseline) +- [x] `devtools::test()` all green (410 pass); `lintr::lint_package()` clean on changed code; `pkgdown::check_pkgdown()` aborts identically on main (DESCRIPTION URL) — pre-existing, not from this branch +- [x] `/code-check` on each commit (with Plan-agent review of the task_plan run concurrently after baseline) NEWS/version bump left to `/gh-pr-merge`, as for #98. ## Validation -- [ ] Tests pass -- [ ] `/code-check` clean on each commit -- [ ] PWF checkboxes match landed work -- [ ] `/planning-archive` on completion +- [x] Tests pass +- [x] `/code-check` clean on each commit +- [x] PWF checkboxes match landed work +- [x] `/planning-archive` on completion