dft_rast_classify(): classify factor input from its raw codes (#91)

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"),

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.

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
 

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

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

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

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.

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.

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.