diff --git a/DESCRIPTION b/DESCRIPTION index 6bd3689..95a5f44 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: drift Title: Detecting Riparian and Inland Floodplain Transitions -Version: 0.19.1 +Version: 0.19.2 Date: 2026-09-29 Authors@R: c( person("Allan", "Irvine", , "al@newgraphenvironment.com", role = c("aut", "cre"), diff --git a/NEWS.md b/NEWS.md index e7ea291..a5eada9 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,9 @@ +# drift 0.19.2 + +- **`dft_rast_classify()` now classifies a raster that is already a factor (#91).** Before, it returned zero levels and no colour table, with no error or warning. This affected the published floodplain rasters, which carry their own attribute table, and a raster re-classified after an earlier `dft_rast_classify()`. The cause was `terra::unique()`, which returns a factor's labels rather than its codes, so no code in `class_table` matched. A factor's levels are now stripped on a copy before its codes are read. The output takes its levels and colours from `class_table`, and the caller's raster keeps its own. A `remap =` that matched a class already worked, because `terra::classify()` reads raw codes. A `remap =` that matched nothing had the bug, and now works. +- **Cost at floodplain scale.** On BULK's published `classified_2017.tif` (169M cells, file-backed) the peak is 1.05 GiB, the same as 0.19.1, because both copies are metadata only. The extra copy costs something only for an in-memory factor with no matching remap: 5.47 GiB against 4.20 GiB. No terra call found reads a factor's codes without one: `activeCat<-` and `levels<-` both copy. +- A multi-layer `SpatRaster` still classifies layer 1 only, as before. The new factor check reads layer 1 so that a stack does not error. + # drift 0.19.1 - **`dft_rast_classify()` no longer modifies the raster it is given (#89).** It called the in-place `terra::set.cats()` on the caller's own object, so after classifying, the original was a factor named `class_name`. This affected file-backed, in-memory and list-element inputs alike; only a `remap =` that matched a class was spared, because that path already builds a new raster. The fix swaps the order of the two setters. `coltab<-` runs first and makes a copy, and the levels are then set on that copy. The output is unchanged: `identical()` `cats()` and `coltab()` against 0.19.0. The obvious alternative, `terra::deepcopy()` first, adds a second full copy. On BULK in memory (169M cells) the reorder peaks at 4.21 GiB, the same as 0.19.0, while `deepcopy()` peaks at 5.46 GiB. A test pins that the caller is unmodified, and it goes red if terra ever makes `coltab<-` work in place. diff --git a/R/dft_rast_classify.R b/R/dft_rast_classify.R index 8dc7177..f28fe78 100644 --- a/R/dft_rast_classify.R +++ b/R/dft_rast_classify.R @@ -4,7 +4,10 @@ #' remapping (collapsing) classes into broader groups. #' #' @param x A [terra::SpatRaster] or a named list of `SpatRaster`s (e.g. from -#' [dft_stac_fetch()]). +#' [dft_stac_fetch()]). A raster that is already a factor, such as a +#' published classified raster with its own attribute table, is accepted: +#' its codes are read from the raw values, and its levels and colours are +#' replaced by `class_table`'s. #' @param class_table A tibble with columns `code`, `class_name`, `color` #' (hex). When `NULL`, loaded via [dft_class_table()] using `source`. #' @param source Character. Used to load a shipped class table when @@ -40,16 +43,27 @@ dft_rast_classify <- function(x, class_table <- remapped$class_table } + # terra::unique() returns a factor's labels, not its codes, so a factor + # would match no code in class_table (#91). strip_copy() drops the levels on + # a copy, and the caller's raster keeps its own. It runs after the remap + # because terra::classify() already reads raw codes and returns a plain + # raster, so only an unremapped factor (no remap, or no group matched) pays + # for it. Layer 1 only, as everywhere below. + if (terra::is.factor(x)[1]) { + x <- strip_copy(x) + } + # Filter class_table to codes actually present in raster present_codes <- terra::unique(x)[, 1] ct <- class_table[class_table$code %in% present_codes, ] - # The order is load-bearing (#89). `x` is the caller's own raster, and - # set.cats() works in place, so it must only ever reach a copy. `coltab<-` - # deep-copies before setting (the fact strip_copy() in dft_rast_break_class.R - # also relies on), so colours first and levels on that copy: one copy, where - # an explicit deepcopy() before `coltab<-` would make two. Should terra make - # `coltab<-` in place, the caller-unmodified test goes red. + # The order is load-bearing (#89). Unless it was stripped or remapped above, + # `x` is the caller's own raster, and set.cats() works in place, so it must + # only ever reach a copy. `coltab<-` deep-copies before setting (the fact + # strip_copy() in dft_rast_break_class.R also relies on), so colours first + # and levels on that copy: one copy, where an explicit deepcopy() before + # `coltab<-` would make two (a stripped factor has already paid one more). + # Should terra make `coltab<-` in place, the caller-unmodified tests go red. coltab_df <- data.frame(value = ct$code, col = ct$color) terra::coltab(x) <- coltab_df diff --git a/man/dft_rast_classify.Rd b/man/dft_rast_classify.Rd index 52a12c1..23647dc 100644 --- a/man/dft_rast_classify.Rd +++ b/man/dft_rast_classify.Rd @@ -8,7 +8,10 @@ dft_rast_classify(x, class_table = NULL, source = "io-lulc", remap = NULL) } \arguments{ \item{x}{A \link[terra:SpatRaster-class]{terra::SpatRaster} or a named list of \code{SpatRaster}s (e.g. from -\code{\link[=dft_stac_fetch]{dft_stac_fetch()}}).} +\code{\link[=dft_stac_fetch]{dft_stac_fetch()}}). A raster that is already a factor, such as a +published classified raster with its own attribute table, is accepted: +its codes are read from the raw values, and its levels and colours are +replaced by \code{class_table}'s.} \item{class_table}{A tibble with columns \code{code}, \code{class_name}, \code{color} (hex). When \code{NULL}, loaded via \code{\link[=dft_class_table]{dft_class_table()}} using \code{source}.} diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/README.md b/planning/archive/2026-09-issue-91-classify-factor-input/README.md new file mode 100644 index 0000000..0f89f3a --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/README.md @@ -0,0 +1,21 @@ +## Outcome + +`dft_rast_classify()` returned a factor with no levels and no colours when its input was already a +factor (a published floodplain raster with a RAT, or one re-classified), because `terra::unique()` on +a factor returns labels, not codes. Fixed by stripping the levels on a copy with the existing +`strip_copy()` after the remap step. `terra::classify()` already reads raw codes, so a matching remap +needs no strip. The guard is `is.factor(x)[1]`. The plan review and code-check round 1 both found, independently, +that a bare `is.factor(x)` errors on a multi-layer stack, which `main` had classified on layer 1 without +error. Four factor-input tests plus stack and active-category tests. Mutation checks: an in-place strip +reddens both caller-unmodified tests, and dropping `[1]` reddens the stack test. Code-check ran 3 rounds +(1 finding, then Clean, Clean). + +## Measurement + +BULK `classified_2017.tif` (169M cells, published RAT), `/usr/bin/time -l`, `main` vs branch: +file-backed factor 1.05 vs 1.05 GiB (levels 0 → 9); in-memory factor 4.20 vs 5.47 GiB (the strip is +one full extra copy, and no R-level terra call reads a factor's codes without one); in-memory factor with +a matching remap 5.47 vs 5.47 GiB. That last number is why the strip was moved after the remap: the first +placement, before the remap, would have charged a copy that `classify()` throws away. Tables in `findings.md`. + +Closed by: commit 618e805 / PR #94 diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/findings.md b/planning/archive/2026-09-issue-91-classify-factor-input/findings.md new file mode 100644 index 0000000..340304a --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/findings.md @@ -0,0 +1,66 @@ +# Findings — dft_rast_classify() returns empty levels when its input is already a factor (#91) + +## Issue context + +## Problem + +`dft_rast_classify()` returns a raster with **zero levels and no colour table** when its input is already a factor. It raises no error and no warning. + +```r +r <- terra::rast(system.file("extdata", "example_2017.tif", package = "drift")) +cl <- dft_rast_classify(r * 1L, source = "io-lulc") +again <- dft_rast_classify(cl, source = "io-lulc") +nrow(terra::cats(again)[[1]]); terra::has.colors(again) # 0 FALSE +``` + +The cause is `present_codes <- terra::unique(x)[, 1]` (`R/dft_rast_classify.R:44`). On a factor, `terra::unique()` returns the **labels** (`"Water"`, `"Trees"`, ...), not the codes. `class_table$code %in% present_codes` then matches nothing, so `ct` is empty and both setters are given empty tables. + +This is the ordinary case, not a contrived one. The published floodplain rasters are factors: `stac-floodplains-bc/bulk_co_ff04/classified_2017.tif` carries a RAT with a `class_name` column and a palette. Classifying one of those, for example to apply a `remap =`, returns a factor raster with no categories. Found in #89's BULK scale check, where the output had neither levels nor colours on `main` and on the branch alike. + +## Fix + +Take the present codes from the raw values, not from `unique()` on the factor: for example `terra::unique(x, as.raster = FALSE)` on a level-stripped copy, or `terra::freq(x, bylayer = FALSE)` with `value` as codes. Add a test that re-classifies a classified raster and a test on a raster that carries its own RAT. Check whether the `remap =` path (`terra::classify()` on a factor) has the same shape. + +Related: #19, which concerns factor input to `dft_rast_transition()`. + +## Probes (terra 1.9.50, 2026-09-29) + +- `terra::unique()` on a SpatRaster calls `x@pntr$unique()` (raw codes) then `get_labels()` — labels for a factor. +- `activeCat(y) <- 0` or `levels(y) <- NULL` on a copy recovers the codes; `strip_copy()` (dft_rast_break_class.R) already does it with one copy. +- remap with a matching group: `terra::classify()` drops the factor, so levels come back fine (6). No matching group: classify skipped, bug reproduces. + +## Errors Encountered + +| Error | Resolution | +|-------|------------| + +## Scale check — BULK `classified_2017.tif` (2026-09-29, `/usr/bin/time -l`, terra 1.9.50) + +Input: the published file, a factor with a RAT (`value`, `class_name`, rgba) and a palette, 14651 x 11552. +`main` = `git archive main`, branch = working tree, both via `pkgload::load_all()`; script in session scratchpad (`scale91.R`). + +| input | src | classify() elapsed | peak RSS | levels out | colours | +|---|---|---|---|---|---| +| file-backed factor | main | 0.8 s | 1.05 GiB | 0 | FALSE | +| file-backed factor | branch | 0.8 s | 1.05 GiB | 9 | TRUE | +| in-memory factor | main | 1.1 s | 4.08 GiB | 0 | FALSE | +| in-memory factor | branch | 2.0 s | 5.46 GiB | 9 | TRUE | +| in-memory setup only | — | — | 2.94 GiB | — | — | + +- File-backed: `strip_copy()` and `coltab<-` copy metadata only; peak unchanged. +- In memory: the strip is one extra full copy of the values, +1.39 GiB peak (5.46 vs 4.08). Paid only by + in-memory factor input, i.e. re-classifying a raster already classified in this session. +- No R-level way to read a factor's raw codes without a copy was found: `activeCat<-` and `levels<-` both + `deepcopy()`; `unique()`/`freq()` return labels; mapping labels back through `cats()` breaks on duplicate + labels and on values absent from the RAT (plan review probe). + +### Re-run after moving the strip after remap (same script, 2026-09-29) + +| input | main peak | branch peak | branch levels / colours | +|---|---|---|---| +| file-backed factor | 1.05 GiB | 1.05 GiB | 9 / TRUE (main 0 / FALSE) | +| in-memory factor | 4.20 GiB | 5.47 GiB | 9 / TRUE (main 0 / FALSE) | +| in-memory factor + matching remap | 5.47 GiB | 5.47 GiB | 8 / TRUE (main 8 / TRUE) | + +The matching-remap path no longer pays for the strip. Only an unremapped in-memory factor does +(+1.27 GiB in this run, +1.39 GiB in the first; run-to-run spread of ~0.1 GiB on the main baseline). diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/progress.md b/planning/archive/2026-09-issue-91-classify-factor-input/progress.md new file mode 100644 index 0000000..3d81c65 --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/progress.md @@ -0,0 +1,15 @@ +# Progress — dft_rast_classify() returns empty levels when its input is already a factor (#91) + +## Session 2026-09-29 + +- Plan-mode exploration — phases approved by user ("go all phases") +- Created branch `91-dft-rast-classify-returns-empty-levels-w` off main +- Scaffolded PWF baseline from issue #91 with approved phases +- Next: start Phase 1 +- Phase 1: four factor-input tests; 9 assertions red on current code (remap-with-match green, as probed) +- Phase 2: `strip_copy()` on factor input before remap; 50/50 in the file, full suite 1336 pass / 0 fail / 15 skip +- Mutation: replacing `strip_copy()` with in-place `set.cats(x, NULL)` turns 4 assertions red, including both caller-unmodified checks +- Plan review + code-check round 1 both found: `if (is.factor(x))` errors on a multi-layer stack → `is.factor(x)[1]` (main classified layer 1 only; kept). Mutation: dropping `[1]` reddens the new stack test. +- Plan review: strip moved after remap (classify() already reads raw codes), stale #89 comment updated, colour check compares to class_table, active-category test added +- Scale check (Phase 3) numbers in findings.md +- code-check: 3 rounds (round 1: stack error, fixed; rounds 2-3 Clean, round 3 verified NEWS claims against main); full suite 1344 pass / 0 fail / 15 skip diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/review-plan.md b/planning/archive/2026-09-issue-91-classify-factor-input/review-plan.md new file mode 100644 index 0000000..923c1ba --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/review-plan.md @@ -0,0 +1,8 @@ +# Plan review (#91) — Plan agent, returned as reply text (read-only agent), written here by the session + +- **Blocker:** `if (terra::is.factor(x))` errors on a multi-layer stack (per-layer predicate). Fixed: `is.factor(x)[1]`; stack test added. +- **Gap:** no multi-layer test; no active-category != 1 test (a GeoTIFF round-trip resets activeCat, so in memory); RAT ids differing from raw values untested (fix does not depend on the RAT). Added the first two. +- **Ordering:** strip before remap wastes a copy when a remap group matches (`classify()` already reads raw codes). Moved after remap; measured equal to main (5.47 GiB). +- **Assumption:** stale #89 copy-count comment. Rewritten. `strip_copy()` leaves the caller untouched — verified. +- **Scope:** file-backed scale run is metadata-only; the in-memory run is the one that carries a cost. Both recorded. +- **Acceptance:** caller-unmodified test is a guard, not a reproduction (comment added); the no-black colour check depends on the fixture (code 0 is #000000) — replaced with a class_table comparison. diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/review-round1.md b/planning/archive/2026-09-issue-91-classify-factor-input/review-round1.md new file mode 100644 index 0000000..d0ab5a6 --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/review-round1.md @@ -0,0 +1,26 @@ +# Code review, round 1 (#91) + +## Findings + +- **[fragile]** R/dft_rast_classify.R:44 — `if (terra::is.factor(x))` fails on a multi-layer + SpatRaster. `terra::is.factor()` returns one logical per layer, so for `c(r17, r20)` the + condition has length 2, and R >= 4.2 stops with "the condition has length > 1". Measured in a + scratch copy on terra 1.9.50. Before the diff, the same stack returned without error, but only + layer 1 was classified (`is.factor` gave `TRUE FALSE`). The old behaviour was already silently + wrong, so no correct output is lost. What changes is that a documented input type ("A + SpatRaster") now fails with a message that names neither the function nor the cause. No caller + in R/, vignettes/, data-raw/ or tests passes a stack; they all pass named lists. Possible fixes: + guard `terra::nlyr(x) == 1` with a clear error, or use `if (any(terra::is.factor(x)))` and + accept that only layer 1 is handled, as before. Low severity. + +## Probed and clean + +- A factor input with levels but no colour table, both in memory and file-backed from a tif with + a RAT. `strip_copy()`'s `coltab<- NULL` still copies, so the caller keeps its levels, its lack + of colours and its names, and the output gets class_table's levels. +- A factor input whose active category is not the first column (`activeCat = 2`). The output + levels are correct, and the caller's `activeCat` and names are unchanged. +- The new test file passes 50/50 in a scratch copy (`NOT_CRAN=true`, `test_file`). +- The black-colour assertion in the RAT test depends on the fixture. io-lulc code 0 (No Data) + is `#000000`, but example_2017 contains no code 0, so the assertion is valid for this fixture. +- The planning files make no factual claim that contradicts the code. diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/review-round2.md b/planning/archive/2026-09-issue-91-classify-factor-input/review-round2.md new file mode 100644 index 0000000..21cc925 --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/review-round2.md @@ -0,0 +1,36 @@ +# Code review, round 2 (#91) + +## Clean + +No code defects found. The round-1 fixes (`is.factor(x)[1]`, strip moved after remap, +rewritten #89 comment) introduce none. + +## Probed (scratch copy, terra 1.9.50, `pkgload::load_all()`) + +- **Caller unmodified in every remap branch.** A factor input's `cats()`, `coltab()`, values, + `activeCat` and names are unchanged after each of: `remap = NULL`, a matching remap, a + non-matching remap (warning, `rcl` NULL, so `x` stays the caller's raster until + `strip_copy()`), and `remap = list()` (same path as non-matching, no warning). The same + holds for a file-backed factor read from a tif, and for plain input with a non-matching remap. +- **`terra::classify()` on a factor** reads raw codes and returns a plain raster + (`is.factor` FALSE, `has.colors` FALSE, `unique()` gives codes). So the matched-remap + branch needs no strip, as the comment says, and its output gets the remapped levels + (6 levels, `Vegetation` at code 2). +- **Stacks.** `c(factor, factor)`, `c(factor, plain)` and `c(plain, factor)`, each with all + three remap cases: no error, the caller's stack is unmodified in all 9 runs, and layer 1 gets + the expected levels. `strip_copy()` strips layer 1 only, which the layer-1-only + `unique(x)[, 1]` needs. +- **Named list** with one factor element and one plain element: both callers are unmodified + and both outputs have identical levels. +- **Test file:** all expectations pass (`NOT_CRAN=true`, `test_file`). +- **Comment accuracy.** "Unless it was stripped or remapped above, `x` is the caller's own + raster" holds in every branch, and non-matching remap does return the caller's raster. + "A stripped factor has already paid one more" also holds: `strip_copy()`'s `coltab<-` makes + one deep copy and the main `coltab<-` makes another, so the factor path makes two copies + and the plain path one. + +## Planning-file note (not a code issue) + +- `planning/active/task_plan.md` Phase 2 still reads "[x] Strip factor input with + `strip_copy()` before remap". The code now strips *after* remap. `progress.md` records the + move, but the task line is stale. diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/review-round3.md b/planning/archive/2026-09-issue-91-classify-factor-input/review-round3.md new file mode 100644 index 0000000..4346c37 --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/review-round3.md @@ -0,0 +1,58 @@ +# Code review, round 3 (#91) + +## Clean + +No code defects found, and every NEWS.md `# drift 0.19.2` claim matches the code and the +measurements. + +## The mechanism behind round 1, and where it could reach in this diff + +The mechanism is a per-layer terra predicate (`is.factor`, `has.colors`, `activeCat`, `nlyr`, +the columns of `unique()`) used as a scalar. On a multi-layer raster it returns a vector, and +`if ()` on that vector errors in R >= 4.2. I checked every place in the diff that it could reach: + +- `R/dft_rast_classify.R:52`, `if (terra::is.factor(x)[1])`. This is the fix. Round 2 probed + it on stacks (9 combinations). +- `strip_copy()` (reached from line 53). Both setters pass `layer = 1` explicitly. +- `R/dft_rast_classify.R:57`, `terra::unique(x)[, 1]`. It takes layer 1's column. Layer 1 has + been stripped whenever it was a factor, so that column holds codes whatever the other layers + are. +- `R/dft_rast_classify.R:68`, `coltab<-` with no `layer` argument. The default is layer 1, so + the result is a scalar. This line predates the diff. +- Tests: `expect_true(is.factor(twice))`, `expect_true(is.factor(rat))`, + `expect_true(has.colors(miss))`, `expect_true(has.colors(out))` and + `expect_identical(activeCat(r), 2L)` all run on single-layer inputs. If one of them got a + longer vector, `expect_true` would fail loudly rather than pass. In the stack test, + `expect_identical(terra::nlyr(out), 2)` is correct because `nlyr()` returns a double + (measured). + +None of them is affected. + +## NEWS claims checked (scratch copies of the branch and of `git archive main`, terra 1.9.50) + +- **"returned zero levels and no colour table, with no error or warning"**: on main, re-classifying + a classified raster gives `is.factor` TRUE, `cats()` NULL and `has.colors` FALSE. No + condition is raised; I caught all warnings and messages and there were none. +- **"the same as 0.19.1"**: `main` was 18153ba. The only commit after the v0.19.1 release is the + CITATION.cff update, and DESCRIPTION reads 0.19.1. Both measurement runs give 1.05 GiB for + main and for the branch on the file-backed input. +- **"5.47 GiB against 4.20 GiB"**: this is the re-run table in findings.md, main 4.20 against + branch 5.47. The first run (4.08 against 5.46) is within the spread that findings.md records. +- **"costs something only for an in-memory factor with no matching remap"**: in the re-run, the + matching remap peaks at 5.47 GiB on both main and the branch. There, the `classify()` copy + plays the part the strip copy plays otherwise. +- **"A remap = that matched a class already worked"**: on main, a factor with a matching remap + returns levels. A partial remap (one group matching, one not) also returns 6 levels on main. +- **"A remap = that matched nothing had the bug"**: on main, `apply_remap()` returns `x` + unchanged when `rcl` is NULL. The output then has 0 levels, and so does `remap = list()`. + On the branch, both return 7 levels. +- **"activeCat<- and levels<- both copy"**: both method bodies call `deepcopy()` in terra + 1.9.50. The claim is hedged ("No terra call found"), and I found nothing that contradicts + it. `values()` does return raw codes, but it loads every cell into R, which is itself a full + copy. `unique()` and `freq()` return labels. +- **"A multi-layer SpatRaster still classifies layer 1 only, as before"**: the setters and + `unique()[, 1]` work on layer 1 only, and the stack test pins this. + +## Test file + +58/58 pass (`NOT_CRAN=true`, `testthat::test_file`, scratch copy). diff --git a/planning/archive/2026-09-issue-91-classify-factor-input/task_plan.md b/planning/archive/2026-09-issue-91-classify-factor-input/task_plan.md new file mode 100644 index 0000000..fbfed9f --- /dev/null +++ b/planning/archive/2026-09-issue-91-classify-factor-input/task_plan.md @@ -0,0 +1,74 @@ +# Task: dft_rast_classify() returns empty levels when its input is already a factor (#91) + +`dft_rast_classify()` returns a raster with **zero levels and no colour table** when its input is already a factor. It raises no error and no warning. + +```r +r <- terra::rast(system.file("extdata", "example_2017.tif", package = "drift")) +cl <- dft_rast_classify(r * 1L, source = "io-lulc") +again <- dft_rast_classify(cl, source = "io-lulc") +nrow(terra::cats(again)[[1]]); terra::has.colors(again) # 0 FALSE +``` + +The cause is `present_codes <- terra::unique(x)[, 1]` (`R/dft_rast_classify.R:44`). On a factor, `terra::unique()` returns the **labels** (`"Water"`, `"Trees"`, ...), not the codes. `class_table$code %in% present_codes` then matches nothing, so `ct` is empty and both setters are given empty tables. + +This is the ordinary case, not a contrived one. The published floodplain rasters are factors: `stac-floodplains-bc/bulk_co_ff04/classified_2017.tif` carries a RAT with a `class_name` column and a palette. Classifying one of those, for example to apply a `remap =`, returns a factor raster with no categories. Found in #89's BULK scale check, where the output had neither levels nor colours on `main` and on the branch alike. + +## Context (from plan) + +`dft_rast_classify()` takes its present codes from `terra::unique(x)[, 1]` (`R/dft_rast_classify.R:44`). +terra's `unique()` reads the raw codes (`x@pntr$unique()`) and then turns them into labels with +`get_labels()`. So on a factor it returns `"Water"`, `"Trees"`, and so on. `class_table$code %in%` +those labels matches nothing, and the output has no levels and no colours. No error or warning is raised. +This is the ordinary case for the published floodplain rasters, which carry a RAT. + +Probed on terra 1.9.50: +- A classified raster reclassified gives 0 levels and `has.colors` FALSE. Confirmed. +- **The `remap =` path does not have this bug when a group matches.** `terra::classify()` drops the + factor, so `unique()` sees codes: 6 levels, correct. **It does have the bug when no remap group + matches.** `rcl` is then NULL, `classify()` is skipped, `x` stays a factor, and levels come back empty. +- The raw codes can be recovered from a copy with its levels stripped. There is already a helper + for this: `strip_copy()` (`R/dft_rast_break_class.R:361`). `coltab<-` makes one copy, and then + `set.cats(NULL)` strips the levels in place on that copy. + +## Approach + +At the top of the single-raster path, before the remap step, add +`if (terra::is.factor(x)) x <- strip_copy(x)`. Everything after that sees plain codes: `unique()`, +`apply_remap()` (including the case where nothing matches), and the setters. `coltab<-` then makes +a second copy, but only for factor input. Non-factor input keeps the one-copy path from #89 unchanged. +The input's own RAT and palette are replaced by `class_table`. That is the existing contract, +and it is documented in `@param x`. + +## Phase 1: Failing tests (`tests/testthat/test-dft_rast_classify.R`) +- [x] Reclassifying a classified raster returns the same `cats()` and `coltab()` as the first pass +- [x] A raster carrying its own RAT gets levels and colours from `class_table`. The raster is + written to a tempfile tif and read back, so it is file-backed. Its RAT labels differ from `class_table`. +- [x] Factor input with a matching `remap =` keeps working. Factor input with a non-matching `remap =` + gives a warning and still returns levels. +- [x] The caller's factor raster is not modified: its levels and colours are still its own after the call +- [x] Confirm that each new test fails on current code + +## Phase 2: Fix (`R/dft_rast_classify.R`) +- [x] Strip factor input with `strip_copy()` — after remap (plan review: `classify()` already reads raw codes), guarded `is.factor(x)[1]`; update the copy-count comment +- [x] Update `@param x` to say factor input is accepted and its RAT is replaced by `class_table`; + run `devtools::document()` +- [x] Full `devtools::test()` green; `lintr::lint_package()` clean + +## Phase 3: Scale check (BULK, per CLAUDE.md) +- [x] `/usr/bin/time -l` on `bulk_co_ff04/classified_2017.tif`, which is a file-backed factor with a RAT. + Compare `main` with the branch, checking peak RSS, wall time, and that the output has levels and colours. +- [x] The same for an in-memory factor input: the in-memory classified output fed back in. + Record the cost of the extra copy. +- [x] Put the numbers in `findings.md`; the script goes in scratchpad or `data-raw/` if worth keeping + +## Phase 4: Release bookkeeping +- [x] NEWS.md `# drift 0.19.2` entry, with numbers derived from the scale run +- [ ] `/planning-archive`, then bump the version to 0.19.2 as the final commit, then `/gh-pr-push`. + Tag the PR with `Relates to NewGraphEnvironment/sred-2025-2026#16`. + +## Validation + +- [ ] Tests pass +- [ ] `/code-check` clean on each commit +- [ ] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion diff --git a/tests/testthat/test-dft_rast_classify.R b/tests/testthat/test-dft_rast_classify.R index 7594a93..dafb68d 100644 --- a/tests/testthat/test-dft_rast_classify.R +++ b/tests/testthat/test-dft_rast_classify.R @@ -80,3 +80,106 @@ test_that("the caller's raster is not modified (#89)", { classified(dft_rast_classify(x, source = "io-lulc")[["2017"]]) unchanged(x[["2017"]], "data") }) + +# Factor input (#91). terra::unique() returns a factor's labels, not its +# codes, so present codes must come from the raw values. +ex_2017 <- function() { + terra::rast(system.file("extdata", "example_2017.tif", package = "drift")) +} + +test_that("re-classifying a classified raster keeps its levels and colours (#91)", { + once <- dft_rast_classify(ex_2017() * 1L, source = "io-lulc") + twice <- dft_rast_classify(once, source = "io-lulc") + expect_true(terra::is.factor(twice)) + expect_false(is.null(terra::cats(twice)[[1]])) + expect_identical(terra::cats(twice), terra::cats(once)) + expect_identical(terra::coltab(twice), terra::coltab(once)) +}) + +test_that("a raster carrying its own RAT gets class_table's levels (#91)", { + r <- ex_2017() * 1L + codes <- sort(terra::unique(r)[, 1]) + # A published-style RAT: its own labels and a palette that class_table + # must replace, read back from disk so the input is file-backed. + terra::set.cats(r, layer = 1, + value = data.frame(value = codes, label = paste0("c", codes))) + terra::coltab(r) <- data.frame(value = codes, col = "#000000") + f <- tempfile(fileext = ".tif") + on.exit(unlink(paste0(f, c("", ".aux.xml"))), add = TRUE) + terra::writeRaster(r, f, datatype = "INT1U") + rat <- terra::rast(f) + expect_true(terra::is.factor(rat)) + + out <- dft_rast_classify(rat, source = "io-lulc") + ct <- dft_class_table("io-lulc") + lv <- terra::cats(out)[[1]] + expect_equal(sort(lv$id), codes) + expect_identical(lv$class_name, ct$class_name[match(lv$id, ct$code)]) + ctab <- terra::coltab(out)[[1]] + expect_equal(sort(ctab$values), codes) + rgb_expected <- grDevices::col2rgb(ct$color[match(ctab$values, ct$code)]) + expect_equal(unname(rbind(ctab$red, ctab$green, ctab$blue)), + unname(rgb_expected)) + + # the caller's raster still carries its own RAT + expect_identical(terra::cats(rat)[[1]]$label, paste0("c", codes)) +}) + +test_that("remap on factor input returns levels, matched or not (#91)", { + once <- dft_rast_classify(ex_2017() * 1L, source = "io-lulc") + + hit <- dft_rast_classify(once, source = "io-lulc", + remap = list(Vegetation = c("Trees", "Rangeland"))) + lv <- terra::cats(hit)[[1]] + expect_true("Vegetation" %in% lv$class_name) + expect_false("Trees" %in% lv$class_name) + + expect_warning( + miss <- dft_rast_classify(once, source = "io-lulc", + remap = list(Nothing = "Not A Class")), + "No matching classes" + ) + expect_identical(terra::cats(miss), terra::cats(once)) + expect_true(terra::has.colors(miss)) +}) + +test_that("the caller's factor raster is not modified (#91)", { + # A guard, not a reproduction: the factor path adds strip_copy(), which + # calls the in-place set.cats(), so this pins that it reaches only a copy. + once <- dft_rast_classify(ex_2017() * 1L, source = "io-lulc") + cats_before <- terra::cats(once) + col_before <- terra::coltab(once) + # a class_table with different names and colours, so any leak shows + ct <- dft_class_table("io-lulc") + ct$class_name <- toupper(ct$class_name) + ct$color <- "#123456" + out <- dft_rast_classify(once, class_table = ct) + expect_true("TREES" %in% terra::cats(out)[[1]]$class_name) + expect_identical(terra::cats(once), cats_before) + expect_identical(terra::coltab(once), col_before) +}) + +test_that("a factor whose active category is not the first reads codes (#91)", { + r <- ex_2017() * 1L + codes <- sort(terra::unique(r)[, 1]) + terra::set.cats(r, layer = 1, value = data.frame( + value = codes, first = paste0("a", codes), second = paste0("b", codes) + )) + terra::activeCat(r) <- 2 + expect_identical(terra::unique(r)[, 1], paste0("b", codes)) + + out <- dft_rast_classify(r, source = "io-lulc") + expect_equal(sort(terra::cats(out)[[1]]$id), codes) + expect_true(terra::has.colors(out)) + expect_identical(terra::activeCat(r), 2L) +}) + +test_that("a multi-layer stack classifies layer 1, as before (#91)", { + # is.factor() is per layer; the factor check must not error on a stack. + once <- dft_rast_classify(ex_2017() * 1L, source = "io-lulc") + for (s in list(c(ex_2017() * 1L, ex_2017() * 1L), c(once, ex_2017() * 1L))) { + out <- dft_rast_classify(s, source = "io-lulc") + expect_identical(terra::nlyr(out), 2) + expect_identical(terra::cats(out)[[1]], terra::cats(once)[[1]]) + } +})