diff --git a/CLAUDE.md b/CLAUDE.md index e16d1e6..8e9bc2f 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 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). +**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). A trend table with no rows — every series under 3 years in its window — still carries every column, with key types as the input's, so consumers need no empty-table special case (#101). ## Function Prefix diff --git a/R/cd_trend.R b/R/cd_trend.R index 0435bff..97f1583 100644 --- a/R/cd_trend.R +++ b/R/cd_trend.R @@ -15,7 +15,9 @@ #' through — see the input contract in [cd_anomaly()]. `unit` is the #' anomaly's unit, so on raw values it is kept only for `absolute` and #' `pct_point_diff` series, where it is also the unit of the values. -#' [cd_summary()] reads them. +#' [cd_summary()] reads them. A combination with fewer than 3 years in +#' its window gives no row; when none has 3, the result is a zero-row +#' tibble with the same columns. #' #' @examples #' catalog <- cd_catalog( @@ -76,8 +78,8 @@ cd_trend <- function(x, trend_start = c(1950, 1980)) { variable = v, period = p, trend_start = ts, - slope = round(sen$coefficients[2], 4), - intercept = round(sen$coefficients[1], 4), + slope = round(unname(sen$coefficients[2]), 4), + intercept = round(unname(sen$coefficients[1]), 4), mk_pvalue = round(mk$sl[1], 4), n_years = nrow(dat), trend_on = val_col @@ -86,5 +88,19 @@ cd_trend <- function(x, trend_start = c(1950, 1980)) { out }) - dplyr::bind_rows(results) + # Bound under a typed zero-row template, so a trend with no series long + # enough keeps its columns rather than becoming a 0 x 0 tibble (#101). Key + # types come from combos, as the rows' do, so a factor variable stays one + template <- tibble::tibble( + variable = combos$variable[0], + period = combos$period[0], + trend_start = if (is.null(trend_start)) numeric() else trend_start[0], + slope = numeric(), + intercept = numeric(), + mk_pvalue = numeric(), + n_years = integer(), + trend_on = character() + ) + for (col in cols_meta) template[[col]] <- character() + dplyr::bind_rows(template, results) } diff --git a/man/cd_trend.Rd b/man/cd_trend.Rd index 0b72fd0..113d856 100644 --- a/man/cd_trend.Rd +++ b/man/cd_trend.Rd @@ -21,7 +21,9 @@ A tibble with columns \code{variable}, \code{period}, \code{trend_start}, through — see the input contract in \code{\link[=cd_anomaly]{cd_anomaly()}}. \code{unit} is the anomaly's unit, so on raw values it is kept only for \code{absolute} and \code{pct_point_diff} series, where it is also the unit of the values. -\code{\link[=cd_summary]{cd_summary()}} reads them. +\code{\link[=cd_summary]{cd_summary()}} reads them. A combination with fewer than 3 years in +its window gives no row; when none has 3, the result is a zero-row +tibble with the same columns. } \description{ Runs Mann-Kendall significance test and Theil-Sen slope estimator diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/README.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/README.md new file mode 100644 index 0000000..e46286f --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/README.md @@ -0,0 +1,18 @@ +## Outcome + +`cd_trend()` dropped every combination with fewer than 3 years and returned +`bind_rows()` of all-`NULL`, a 0 x 0 tibble, so `cd_summary()` died on +`Column 'period' not found` and `cd_plot_timeseries()` warned about uninitialised +columns. Rows are now bound under a zero-row template, so an empty trend carries every +column and flows through both consumers unchanged. The first version of the template +restated column types independently of the row builder, and the plan review and +code-check round 1 both caught what that cost: a factor `variable`/`period` came back +character on *every* result, and `trend_start = NULL` dropped its column. The key +columns now take their types from the same `combos` the rows are built from, and +`slope`/`intercept` are unnamed so empty and full results share one ptype. Round 3 +enumerated all 11 template columns against the row builder and found them in agreement. +The existing test for short series asserted only `nrow == 0`, which the 0 x 0 satisfied, +which is why this was never caught. Each new guard was shown to fail against its own +restored defect. + +Closed by: commit 8892556 / PR #110 diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/findings.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/findings.md new file mode 100644 index 0000000..3ee72f0 --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/findings.md @@ -0,0 +1,41 @@ +# Findings — cd_trend(): no series long enough gives a 0x0 tibble, and cd_summary() errors on it (#101) + +## Issue context + +**If we do it:** `cd_summary()` on a trend with no rows returns an empty table. **If we never do:** a chain run on short series (every series under 3 years in its trend window) aborts inside `cd_summary()` with an error naming a missing column, which says nothing about the cause. + +## Problem + +`cd_trend()` returns `NULL` for every combination with fewer than 3 years, and `dplyr::bind_rows()` of all-`NULL` is a **0 x 0** tibble — no `variable`, `period` or `slope` columns. `cd_summary()` then fails: + +```r +x <- tibble::tibble(variable = "tmean", period = "annual", year = 2000:2001, value = 1:2) +cd_summary(cd_trend(x, 2000)) +#> Error in dplyr::mutate(...): Column `period` not found in `.data`. +#> Warning: Unknown or uninitialised column: `variable`. +``` + +Reproduced on main's code (v0.5.1) and on the #97 branch. Found by the `/code-check` review for #97. + +## Proposed Solution + +- `cd_trend()` returns a zero-row tibble with its full column set when no combination has enough years, so every consumer sees a typed empty table rather than a shapeless one. +- Test: `cd_summary(cd_trend(<2-year series>))` has 0 rows and the documented columns. + + +## Plan-mode probe (2026-09-30, v0.5.5) + +- `cd_summary(cd_trend(<2-year series>, 2000))` reproduces the error on main. +- A hand-built typed zero-row trend (8 documented columns) through `cd_summary()` gives + 0 x 7; with `region_name` 0 x 8; with `anomaly_type`/`unit`/`long_name` also 0 x 7. + `cd_plot_timeseries(trend = )` draws with no warning; with the 0x0 it + warns `Unknown or uninitialised column: 'variable'` / `'period'`. +- `cd_trend(x[0, ], 2000)` is also 0x0 today. +- The existing test `cd_trend skips combos with < 3 years` asserts only `nrow == 0`, + which the 0x0 satisfies — a fixture that could not reach the failure. + +## Errors Encountered + +| Error | Resolution | +|-------|------------| +| Mutation check reported "0 lines mutated" while tests went red | `diff` is a shell function in this profile; used `/usr/bin/diff` | diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/progress.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/progress.md new file mode 100644 index 0000000..ff5f99c --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/progress.md @@ -0,0 +1,25 @@ +# Progress — cd_trend(): no series long enough gives a 0x0 tibble, and cd_summary() errors on it (#101) + +## Session 2026-09-30 + +- Plan-mode exploration — phases approved by user +- Created branch `101-cd-trend-no-series-long-enough-gives-a-0` off main +- Scaffolded PWF baseline from issue #101 with approved phases +- Next: start Phase 1 + +- Phase 1: 5 tests added across test-cd_{trend,summary,plot_timeseries}.R; against unchanged + `cd_trend()` they failed (FAIL 8 / 1 / 1 per file, NOT_CRAN=true test_file) +- Phase 2: `cd_trend()` binds results under a typed zero-row template; `@return` documents + the empty shape. `devtools::test()` FAIL 0 | WARN 6 | PASS 427 — the 6 warnings are in + test-cd_plot_comparison.R and identical on main. Lint on touched files clean apart from + pre-existing single-file `object_usage_linter` hits on the internal helpers. +- Plan review + code-check round 1 (both in `review-plan.md` / `review-round1.md`) found + that the `character()` template turned a factor `variable`/`period` into character on + every result, and that `trend_start = NULL` dropped the `trend_start` column. Fixed by + taking key types from `combos` and a `numeric()` fallback. Also `unname()` on + `slope`/`intercept` so the empty and full ptypes are identical (zyp names them `yr` / + `Intercept`). Three tests added; each fails against exactly its own restored defect + (mutation run in a scratch copy). +- `/code-check`: 3 rounds. R1 2 findings (factor keys, NULL trend_start) fixed; R2 clean + (reviewed the fixes); R3 clean with a per-column enumeration of template vs row types + (11 columns, all agree). No defect was found inside a review fix. diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-plan.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-plan.md new file mode 100644 index 0000000..0337967 --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-plan.md @@ -0,0 +1,20 @@ +# Plan review (#101) — Plan agent, 2026-09-30 + +Delivered as reply text (Plan agents cannot write files); recorded here. + +No blockers. Tests confirmed failing on HEAD and passing with the fix. + +1. **Assumption — factor input returns character.** `character()` template makes vctrs + combine chr + factor to chr; HEAD kept factors (expand.grid keeps them). Fix: build + the template from `combos$variable[0]`, `combos$period[0]`. **Probed: confirmed.** +2. **Assumption — `slope`/`intercept` named on non-empty rows only** (`"yr"`, + `"Intercept"` from `zyp.sen`). Predates #101, but blocks a ptype-identity test. + Fix: `unname()`. **Probed: confirmed (`Named num`).** +3. **Gap — `trend_start = NULL` gives 0 x 7 with no `trend_start` column**, breaking + the `@return` "same columns" promise. **Probed: confirmed.** +4. **Gap — no test pins that empty and non-empty share one ptype.** +5. **Gap — two new cd_trend tests lack `skip_if_not_installed("Kendall"/"zyp")`**, + which every test since #92 carries. + +Consumers: nothing in R/, vignettes/, data-raw/, scripts/ relies on the 0x0 shape; +vignette loops filter then check `nrow(sub) == 0`, which works on both shapes. diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round1.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round1.md new file mode 100644 index 0000000..467531c --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round1.md @@ -0,0 +1,25 @@ +# Code-check review — round 1 (#101) + +Reviewer: subagent, 2026-09-30. Probes run in scratch copies (`cdold` = HEAD `R/cd_trend.R`, `cdcopy` = working tree); the repo was not edited apart from this file. + +## Verdict + +No blocking bugs. Two low-severity observations below; neither breaks any in-repo consumer. + +## What was verified + +- **Restore-the-bug:** ran the three touched test files with `NOT_CRAN=true` against the pre-fix `cd_trend()`: + test-cd_trend.R FAIL 8 (the three #101 tests), test-cd_summary.R ERROR 1 (the #101 test), test-cd_plot_timeseries.R FAIL 1 (the #101 test). + Against the fix: all three files FAIL 0 (39 / 92 / 41 pass). The author's claim holds. +- **Type agreement template vs non-empty rows** (old vs new output compared on the same inputs): + - `trend_start = 1951` (double), `1951L` (integer), `"1951"` (character), integer start on a double `year`: `trend_start[0]` matches `combos$trend_start` in every case, so the column type is identical to the pre-fix output. + - `slope` / `intercept`: the named numeric from `zyp` (`names = "yr"`) survives the bind exactly as before; `vctrs` does not error or strip on binding unnamed `numeric()` with a named double. + - `n_years` integer, `trend_on` character, metadata columns character: consistent. + - Grouped input, multi-window input, `bind_rows(cd_trend(x), cd_trend(ano))` mixing an empty and a non-empty table: unchanged / fine. +- **Consumers:** `cd_summary()` on zero-row trends (plain, carried metadata, `region_name`, two windows) returns typed 0-row tables with no warning; the mixed-scale bind still summarises. `cd_plot_timeseries()` no longer hits the `Unknown or uninitialised column` warning that `trend$variable` raised on the old 0x0 tibble. The `nrow(trn_dat) > 0` guard prevents a spurious "not on the plotted scale" warning. +- Vignettes and `data-raw/` call `cd_trend()` on character `variable` input and convert to factor **after** the call, so no caller relied on the old shape; no caller tests `ncol()` / `length()` of a trend table. + +## Findings + +- **[severity: fragile]** R/cd_trend.R:93-94 — a **factor** `variable` / `period` input now comes back as **character** on the non-empty path too, not only when empty. `expand.grid(stringsAsFactors = FALSE)` leaves a factor as a factor, so pre-fix `cd_trend()` returned `` columns; binding under the `character()` template coerces them (`vctrs` factor + character -> character, silently). Measured: old `variable `, new `variable ` for the same input. Nothing in R/, vignettes/, data-raw/ or scripts/ passes factor series or depends on factor levels of a trend table (the vignettes re-factor after the call), so no in-repo breakage — but it is an unannounced change on the path the fix was meant to leave alone. Character is arguably the better contract; if kept, it is worth a NEWS line, or else build the template's `variable`/`period` from `x$variable[0]` / `x$period[0]`. +- **[severity: fragile]** R/cd_trend.R:96 — `trend_start = NULL` gives `trend_start[0]` = `NULL`, which `tibble()` drops, so the result is a 0 x 7 tibble **without** `trend_start`, contradicting the new `@return` sentence ("a zero-row tibble with the same columns"). Pre-fix it was 0 x 0. Degenerate input nobody passes; harmless in practice (`cd_summary()` on it is untested), noted only because the doc now makes a claim this input violates. diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round2.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round2.md new file mode 100644 index 0000000..d776706 --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round2.md @@ -0,0 +1,35 @@ +# Code-check review — round 2 (#101) + +Reviewer: subagent, 2026-09-30. Scope: whether the round-1 fixes (template key types from `combos`, `is.null(trend_start)` fallback, `unname()` on slope/intercept) are themselves correct. All probes ran in a scratch copy of the repo; the repo was not edited apart from this file. + +## Clean + +No issues found. + +## What was verified + +Probed `cd_trend()` (working tree) on each input the brief named, reading the class of every result column: + +| input | result | +|---|---| +| zero-row x, default `trend_start` | 0 x 8; variable/period `chr`, trend_start `dbl` | +| zero-row x with factor variable/period | 0 x 8; variable/period stay `fct` | +| `trend_start = NULL` | 0 x 8, `trend_start` present as `dbl` (round-1 finding 2 fixed) | +| `trend_start = "2000"` (character) | 1 row, trend_start `chr`; template and row agree | +| `trend_start = c(a = 2000)` (named) | 1 row, `dbl`; no bind error | +| `trend_start = factor(2000)` | template and row both `fct`; binds (year comparison is meaningless, which predates this change) | +| Date / POSIXct `trend_start` | empty path: `trend_start `, 0 x 8. Full path errors inside `zyp.sen` (`difftime` division) before reaching the bind — the same on HEAD, so not a regression. `expand.grid()` keeps the Date class, so template and rows could not disagree even if zyp accepted it | +| integer `variable` | template and row both `int` | +| `NA` variable | `chr`, binds | +| grouped input | ungrouped by `series_check()`, unchanged | +| factor variable, character period | `fct` / `chr`, each column from its own `combos` column | +| `bind_rows(, )` across two calls | vctrs coerces to `chr` silently, no error | + +No input found where the template's type and the rows' type disagree: both come from the same `combos` columns (or, for `trend_start = NULL`, there are no rows at all, since `expand.grid()` with a NULL argument gives zero rows). + +`unname()`: `slope` and `intercept` previously carried the name `"yr"` per element from `zyp`. Grepped R/, vignettes/, data-raw/ and scripts/ for any `names()` read on a slope or intercept: none. `mk_pvalue` has no attributes either way. So the only visible difference is that `names(trn$slope)` is now `NULL`, and no caller reads it. + +Restore-the-bug, in the scratch copy: +- template keys reverted to `character()` and the NULL fallback removed: `test-cd_trend.R` FAIL 5 (the factor test and the NULL-`trend_start` test). +- `unname()` removed: `test-cd_trend.R` FAIL 1 ("empty result has the same column types as a full one"). +- With the fix: test-cd_trend 48 / test-cd_summary 92 / test-cd_plot_timeseries 41 pass, 0 fail, 0 skip (`NOT_CRAN=true`). diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round3.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round3.md new file mode 100644 index 0000000..7af4d4c --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/review-round3.md @@ -0,0 +1,45 @@ +# Code-check review — round 3 (#101) + +Reviewer: subagent, 2026-09-30. Probes ran in a scratch copy (`/private/tmp/claude-501/r3/cd`, working tree, plus HEAD's `R/cd_trend.R` sourced as `old`); the repo was not edited apart from this file. + +## Clean + +No issues found. + +## Mechanism + +The three earlier findings share one cause: the zero-row template in `R/cd_trend.R:94-104` **restates** each column's type, while the per-row `tibble()` in the `lapply()` (lines 77-87) builds the same columns from its own sources. Nothing ties the two together; they are correct only while two independent lists happen to agree. `bind_rows()` then resolves any disagreement silently through vctrs' common type (factor + character -> character, integer + double -> double), so a mismatch shows up as a changed type on the *full* path, not as an error. + +Round 1's fixes removed the restatement for the three key columns (variable, period, trend_start now come from the same `combos` columns / `trend_start` vector the rows use). What still restates is the five computed columns and the metadata columns, so those are the ones enumerated below against what the row construction actually produces. + +## Enumeration: every template column vs the row's type + +Inputs probed (each on the full path, on the empty path by moving `trend_start` past the data, and against HEAD): character / factor / integer keys; double, integer, character and named `trend_start`; `value` vs `anomaly`; integer vs double values (including an odd-length integer series, where `median()` of integers would stay integer, and a constant integer anomaly); double `year`; an NA in the values; carried `anomaly_type`/`unit`/`long_name` as factor and as logical NA; a raw `prcp` series whose unit is dropped by `meta_resolve(raw = TRUE)`; a mixed table where one combo passes and one is too short; `bind_rows()` of an empty value-trend with a full anomaly-trend. + +| column | template source | row source | row type over the input space | verdict | +|---|---|---|---|---| +| `variable` | `combos$variable[0]` | `combos$variable[i]` | same vector: chr / fct (levels kept) / int | agree by construction | +| `period` | `combos$period[0]` | `combos$period[i]` | same vector: chr / fct / int | agree by construction | +| `trend_start` | `trend_start[0]`, or `numeric()` if NULL | `combos$trend_start[i]` (`expand.grid` keeps class and names) | dbl / int / chr / named dbl; NULL gives no rows | agree (names survive on both, as on HEAD) | +| `slope` | `numeric()` | `round(unname(sen$coefficients[2]), 4)` | always double: zyp computes it by division, so integer `y`/`yr` cannot make it integer | agree | +| `intercept` | `numeric()` | `round(unname(sen$coefficients[1]), 4)` | always double: `median(y - slope * yr)` with a double slope | agree | +| `mk_pvalue` | `numeric()` | `round(mk$sl[1], 4)` | double. `mk$sl` carries a `Csingle` attribute; `[1]` drops it, so no attribute reaches the column | agree | +| `n_years` | `integer()` | `nrow(dat)` | integer | agree | +| `trend_on` | `character()` | `val_col` | character | agree | +| `anomaly_type` | `character()` | `as.character(dat[[col]][1])`, after `meta_resolve()` (which reads via `col_or_na()`, already `as.character`) | character for factor, logical-NA and character input | agree | +| `unit` | `character()` | same | character, NA_character_ when dropped for raw `pct_normal` | agree | +| `long_name` | `character()` | same | character | agree | + +Column set and order: template and rows both add the metadata columns by looping over the same `cols_meta`, after the same eight, so names and order agree too. + +Against HEAD, on every input above, each column of the new result is `identical()` to the old one except `slope` and `intercept`, which differ only by the dropped `"yr"` names (accepted). On every input the empty result's column classes equal the full result's. + +## Consumers + +- `cd_summary()` on an empty factor-keyed trend: typed 0 x 7, no warning. Two windows with one empty: one row, no spurious `Start` column. Empty value-trend bound with a full anomaly-trend: one row. +- `cd_plot_timeseries()` with an empty factor-keyed trend: returns a ggplot, no warning. +- `NOT_CRAN=true` single-file runs of `test-cd_trend.R`, `test-cd_summary.R`, `test-cd_plot_timeseries.R`: no failures. + +## Not findings + +- `trend_start = NA` (logical) or a factor `trend_start`: the template and rows agree (same source); the year comparison is meaningless, which predates this change. diff --git a/planning/archive/2026-09-issue-101-cd-trend-empty-shape/task_plan.md b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/task_plan.md new file mode 100644 index 0000000..1b017b0 --- /dev/null +++ b/planning/archive/2026-09-issue-101-cd-trend-empty-shape/task_plan.md @@ -0,0 +1,37 @@ +# Task: cd_trend(): no series long enough gives a 0x0 tibble, and cd_summary() errors on it (#101) + +`cd_trend()` returns `NULL` for every combination with fewer than 3 years, and +`dplyr::bind_rows()` of all-`NULL` is a **0 x 0** tibble — no `variable`, `period` or +`slope` columns. `cd_summary()` then fails with `Column 'period' not found in '.data'`, +which says nothing about the cause. + +Plan-mode probe: a typed zero-row trend table already flows cleanly through +`cd_summary()` (with and without `region_name`, with and without meta columns) and +`cd_plot_timeseries()`. The fix is entirely `cd_trend()`'s return shape: bind results +onto a zero-row template carrying the full column set, so 0 and N surviving rows share +one code path. + +## Phase 1: Tests first (fail on main) +- [x] `test-cd_trend.R`: extend `skips combos with < 3 years` — 0 rows, `expect_named()` the documented 8 columns, `trend_on` character, `n_years` integer +- [x] `test-cd_trend.R`: zero-row result keeps `anomaly_type`/`unit`/`long_name` when the input carries them +- [x] `test-cd_trend.R`: 0-row input (`x[0, ]`) returns the same typed empty shape +- [x] `test-cd_summary.R`: `cd_summary(cd_trend(<2-year series>))` has 0 rows and the documented columns (`Parameter`, `Period`, `Slope`, `Years`, `Total Change`, `Unit`, `p-value`); with `region_name`, `Region` too +- [x] `test-cd_plot_timeseries.R`: `trend = cd_trend()` draws with `expect_no_warning()` +- [x] Confirm the new tests fail against unchanged `cd_trend()` + +## Phase 2: Fix `cd_trend()` +- [x] Zero-row template bound under `results` in `R/cd_trend.R` +- [x] `@return`: state that a trend with no combination of >= 3 years is a zero-row tibble with the same columns +- [x] `devtools::document()`; full `devtools::test()` green; `lintr::lint_package()` clean +- [x] `/code-check`, commit with checkbox flips, `Fixes #101` + +## Phase 3: Wrap up +- [x] `devtools::check()` — 0 errors; 1 warning + 8 notes all pre-existing (no VignetteBuilder, .Rbuildignore gaps #100/#107, `.data` bindings, unused `sf`) +- [x] `/planning-archive`, `/gh-pr-push` (SRED line in PR body). Merge is a separate instruction. + +## Validation + +- [x] Tests pass +- [x] `/code-check` clean on each commit +- [x] PWF checkboxes match landed work +- [x] `/planning-archive` on completion diff --git a/tests/testthat/test-cd_plot_timeseries.R b/tests/testthat/test-cd_plot_timeseries.R index e0bacc7..71b94d7 100644 --- a/tests/testthat/test-cd_plot_timeseries.R +++ b/tests/testthat/test-cd_plot_timeseries.R @@ -231,3 +231,14 @@ test_that("cd_plot_timeseries dashes the earliest trend_start whatever the row o expect_identical(ggplot2::layer_data(p, idx[1])$x[1], 1951) expect_identical(p$layers[[idx[2]]]$aes_params$linetype, "solid") }) + +test_that("cd_plot_timeseries takes a trend with no series long enough, silently (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + dat <- tibble::tibble( + variable = "tmean", period = "annual", year = 1951:1960, anomaly = seq(-2, 3, length.out = 10) + ) + trn <- cd_trend(dat, trend_start = 1959) + expect_no_warning(p <- cd_plot_timeseries(dat, trend = trn)) + expect_s3_class(p, "ggplot") +}) diff --git a/tests/testthat/test-cd_summary.R b/tests/testthat/test-cd_summary.R index a0193df..8c9583c 100644 --- a/tests/testthat/test-cd_summary.R +++ b/tests/testthat/test-cd_summary.R @@ -365,3 +365,18 @@ test_that("cd_summary keeps scales x windows x region distinct (#106)", { expect_equal(nrow(smry), 4) expect_equal(anyDuplicated(smry[c("Parameter", "Period", "Trend on", "Start")]), 0) }) + +test_that("cd_summary on a trend with no series long enough is an empty table (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- tibble::tibble(variable = "tmean", period = "annual", year = 2000:2001, value = 1:2) + out <- cd_summary(cd_trend(x, trend_start = 2000)) + expect_equal(nrow(out), 0) + expect_named(out, c("Parameter", "Period", "Slope", "Years", "Total Change", "Unit", "p-value")) + + out <- cd_summary(cd_trend(x, trend_start = 2000), region_name = "Example") + expect_equal(nrow(out), 0) + expect_named(out, c( + "Parameter", "Period", "Slope", "Years", "Total Change", "Unit", "p-value", "Region" + )) +}) diff --git a/tests/testthat/test-cd_trend.R b/tests/testthat/test-cd_trend.R index e874542..01d3289 100644 --- a/tests/testthat/test-cd_trend.R +++ b/tests/testthat/test-cd_trend.R @@ -68,6 +68,47 @@ test_that("cd_trend skips combos with < 3 years", { trn <- cd_trend(ts, trend_start = 1959) expect_equal(nrow(trn), 0) + # A typed empty table, not 0 x 0, so consumers can read its columns (#101) + expect_s3_class(trn, "tbl_df") + expect_named(trn, c( + "variable", "period", "trend_start", "slope", "intercept", "mk_pvalue", + "n_years", "trend_on" + )) + expect_type(trn$variable, "character") + expect_type(trn$trend_on, "character") + expect_type(trn$slope, "double") + expect_type(trn$n_years, "integer") +}) + +test_that("cd_trend keeps the carried metadata columns when no series is long enough (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- tibble::tibble( + variable = "q_mean", period = "spawn", year = 2000:2001, + anomaly = c(-5, 5), anomaly_type = "pct_normal", unit = "%", + long_name = "Mean discharge" + ) + trn <- cd_trend(x, trend_start = 2000) + expect_equal(nrow(trn), 0) + expect_named(trn, c( + "variable", "period", "trend_start", "slope", "intercept", "mk_pvalue", + "n_years", "trend_on", "anomaly_type", "unit", "long_name" + )) + expect_type(trn$long_name, "character") +}) + +test_that("cd_trend on a zero-row input returns the typed empty table (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + ts <- tibble::tibble( + variable = "tmean", period = "annual", year = 1951:1960, value = seq(0, 9) + ) + trn <- cd_trend(ts[0, ], trend_start = 1951) + expect_equal(nrow(trn), 0) + expect_named(trn, c( + "variable", "period", "trend_start", "slope", "intercept", "mk_pvalue", + "n_years", "trend_on" + )) }) test_that("cd_trend carries anomaly_type, unit and long_name through (#92)", { @@ -146,3 +187,44 @@ test_that("cd_trend errors when a raw series carries two units (#97)", { ) expect_error(cd_trend(x, trend_start = 2000), "unit.*q_mean/annual") }) + +test_that("cd_trend's empty result has the same column types as a full one (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- tibble::tibble( + variable = "q_mean", period = "spawn", year = 2000:2009, + anomaly = seq(-5, 13, by = 2), anomaly_type = "pct_normal", unit = "%", + long_name = "Mean discharge" + ) + full <- cd_trend(x, trend_start = 2000) + expect_equal(nrow(full), 1) + expect_identical(cd_trend(x, trend_start = 2009), full[0, ]) +}) + +test_that("cd_trend keeps a factor variable and period a factor, empty or not (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- tibble::tibble( + variable = factor("tmean"), period = factor("annual"), year = 2000:2009, + value = as.numeric(1:10) + ) + full <- cd_trend(x, trend_start = 2000) + expect_s3_class(full$variable, "factor") + expect_s3_class(full$period, "factor") + empty <- cd_trend(x, trend_start = 2009) + expect_equal(nrow(empty), 0) + expect_s3_class(empty$variable, "factor") + expect_s3_class(empty$period, "factor") +}) + +test_that("cd_trend with no trend_start still returns the full column set (#101)", { + skip_if_not_installed("Kendall") + skip_if_not_installed("zyp") + x <- tibble::tibble(variable = "tmean", period = "annual", year = 2000:2009, value = 1:10) + trn <- cd_trend(x, trend_start = NULL) + expect_equal(nrow(trn), 0) + expect_named(trn, c( + "variable", "period", "trend_start", "slope", "intercept", "mk_pvalue", + "n_years", "trend_on" + )) +})