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 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

Expand Down
24 changes: 20 additions & 4 deletions R/cd_trend.R
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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)
}
4 changes: 3 additions & 1 deletion man/cd_trend.Rd

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

18 changes: 18 additions & 0 deletions planning/archive/2026-09-issue-101-cd-trend-empty-shape/README.md
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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 = <typed empty>)` 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` |
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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 `<fct>` columns; binding under the `character()` template coerces them (`vctrs` factor + character -> character, silently). Measured: old `variable <fct>`, new `variable <chr>` 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.
Original file line number Diff line number Diff line change
@@ -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 <date>`, 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(<factor empty>, <chr full>)` 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`).
Loading
Loading