Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
29 changes: 25 additions & 4 deletions R/cd_summary.R
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down
20 changes: 16 additions & 4 deletions man/cd_summary.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions planning/archive/2026-09-issue-106-summary-trend-start/README.md
Original file line number Diff line number Diff line change
@@ -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`)
38 changes: 38 additions & 0 deletions planning/archive/2026-09-issue-106-summary-trend-start/findings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# 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 |
|-------|------------|

## 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.
13 changes: 13 additions & 0 deletions planning/archive/2026-09-issue-106-summary-trend-start/progress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# 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
- 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
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Loading
Loading