diff --git a/CLAUDE.md b/CLAUDE.md index 62c8db5..eb26c41 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,6 +43,10 @@ the cap 0.05, and the two readings of the sweep that were wrong first. `data-raw/mask_calibrate-border_threshold.R` reproduces everything from a directory of thumbnails, and `inst/extdata/mask_border_sweep.csv` ships the result so the test suite recomputes both constants rather than trusting them +- `data-raw/mask_measure-interior_zeros.R` — the two measurements fly#56 took before changing +the output contract: true black the old grayscale `-dstnodata 0` deleted, and how often +`fly_mask_one()` declines. PSOCK over a thumbnail directory; refuses to report if any frame +errored, because a closure missing on the workers once made every frame "error" and print zeros - `data-raw/height_calibrate-flying_height_slip.R` — pulls every catalogue centroid by year (1.67 million, cached under the gitignored `data-raw/.cache/`), samples MRDEM under the 7,156 frames that decide the `flying_height` constants, and prints a producer line for every figure @@ -235,22 +239,31 @@ of 264 frames** had under a tenth of theirs masked. `fly_mask()` replaces it wit seeded from the image border, so interior dark water survives — on bcb90128_213 a plain threshold takes 7.7% and this takes 2.5%, and the missing 5% is a lake. - **Output band counts do not change**, because `-srcalpha` excludes the alpha band from the -warped band list. That is what let masking default to on without moving `stac_airphoto_bc`. -Grayscale keeps `-dstnodata 0`, the weaker contract — fly#56. Do not re-derive the collar's -shape from geometry or re-propose a circle; read `inst/notes/border-masking.md` - - **That band-count claim is platform-conditional and was written as if it were not** -(fly#68, 2026-09-21). It was measured on macOS; the three-platform CI added in fly#52 -found on its first run that **Windows yields 2 bands for a masked grayscale frame** -against 1 with masking off. ubuntu and macOS give 1; RGB is 4 everywhere. So the stated -reason masking could default to on — that `stac_airphoto_bc` need not move — does not -hold for grayscale produced on Windows. A second symptom from the same root, on the same -runner: GDAL reports *"Value 0 in the source dataset has been changed to 1 ... to avoid -being treated as NoData"*, so genuine zeros are silently shifted in the warped output. -`test-fly_georef_mask.R` **pins** the observed Windows value rather than skipping it, so -it reddens if that platform moves in either direction — including the direction where -fly#68 is fixed. Do not re-state the invariant unconditionally while fly#56 is open (it absorbed fly#68) + Masking does not change the band count, because `-srcalpha` excludes the alpha band from +the warped band list. Do not re-derive the collar's shape from geometry or re-propose a +circle; read `inst/notes/border-masking.md` + +- **Every georef output carries one alpha band, and `srcnodata` backs a declined mask** +(v0.19.0, #56, absorbing #68 and #69) — grayscale warps with `-dstalpha` like RGB, so it +is **2 bands (Gray + Alpha, no NoData), RGB 4** — measured on macOS / sf's GDAL 3.8.5, +and `test-fly_georef_mask.R` asserts it unconditionally so the three-platform CI decides +the rest; do not restate it as cross-platform fact from one machine, which is #68's error. v0.11.0 kept grayscale on +`-dstnodata 0` so `stac_airphoto_bc` need not move, and that contract failed twice: GDAL +**rewrote** (silently on sf's 3.8.5) genuine black as 1 to dodge the nodata value — measured on the output, +**161 of 182** calibration grayscale frames warped axis-aligned (up to 3.6%) and 41 at a +30-degree bearing (up to 0.26%); an early draft measured 30 degrees alone and quoted it as +the loss, which is wrong for every bearingless thumbnail (isotropic pixels; an anisotropic +scan resamples even axis-aligned) — and it was not one number, since the +Windows runner gave 2 bands masked (#68). A UInt16 output's alpha is 0/65535, not 0/255. + + `fly_georef()` **accepts** `mask = "border"` with `srcnodata`: `fly_georef_warp_opts()` +is an `else if`, so `-srcnodata` reaches only a frame whose mask declined and never sits +beside `-srcalpha`, which would delete the interior black the mask kept. The v0.11.0 +refusal rested on the premise that both reached GDAL; they never did. Keep the `else if` +— it is now the whole of the safety. A decline with no `srcnodata` warns, because that +collar is written as data. The mask declined **0 of 10,105** public thumbnails, so the +fallback's reach is non-Byte scans nothing public can fixture. Read +`inst/notes/border-masking.md` - **`flying_height` is held against `scale x focal_length` before the DEM route believes it, and the one identifiable error is repaired** (v0.12.0, #54) — the catalogue's `FLYING_HEIGHT` diff --git a/NEWS.md b/NEWS.md index d5c769f..40a1039 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # fly (development version) +- **Breaking: grayscale georeferenced output is now 2 bands, Gray + Alpha, where it was 1 band with `NoData = 0`** ([#56](https://github.com/NewGraphEnvironment/fly/issues/56)). `fly_georef()` warps every frame with `-dstalpha`, so RGB stays 4 bands and, from a grayscale or RGB source, no output carries a NoData value. A consumer that reads band 1 alone, or treats 0 as fill, must now read the last band: 0 is fill, and on these outputs the opaque value is the datatype's maximum (255 on Byte, 65535 on UInt16). Under `-dstnodata 0`, GDAL rewrote genuine black inside the frame as 1 so it would not read as fill (or deleted it where `srcnodata = "0"` was also given). Measured on the output of the 182 grayscale frames the mask was calibrated on, it hit **161 frames** warped axis-aligned (every frame with no flight bearing): typically a few dozen pixels, up to 3.6% of a frame on roll bcb94081 (1250 x 1250 thumbnails on square footprints; an anisotropic scan resamples even when axis-aligned). On a footprint rotated 30 degrees it hit 41 frames, at most 0.26%, because resampling mixes a lone 0 with its neighbours. The Windows CI runner also gave a masked grayscale frame 2 bands where ubuntu and macOS gave 1 ([#68](https://github.com/NewGraphEnvironment/fly/issues/68)). Band count is now one number, measured on macOS with sf's GDAL 3.8.5 and asserted by the test suite on all three CI platforms. This supersedes 0.11.0's "nothing downstream changes band count". `stac_airphoto_bc` needs a COG-writer change and a republish of grayscale frames ([stac_airphoto_bc#36](https://github.com/NewGraphEnvironment/stac_airphoto_bc/issues/36)). +- **`fly_georef(mask = "border", srcnodata = ...)` is now accepted, and `srcnodata` is the fallback for frames whose mask declined** ([#69](https://github.com/NewGraphEnvironment/fly/issues/69), absorbed into #56). The warp options were always either/or, so the 0.11.0 refusal guarded a combination GDAL never received and only made the fallback unreachable. `srcnodata` reaches a frame only when `fly_mask()` declined it, and never alongside a mask that ran. A frame whose mask declines with no `srcnodata` is now warned about, since its collar reaches the output as image data. The mask declined none of 10,105 public thumbnails, so on thumbnails this changes nothing; it matters for non-8-bit scans. `data-raw/mask_measure-interior_zeros.R` reproduces both figures. + ## 0.18.0 (2026-09-28) - **Half the r ≈ 2 mass was a wrong scale, not a mislabelled lens, and it is now drawn at its true width** ([#72](https://github.com/NewGraphEnvironment/fly/issues/72)). v0.12.0 said the film frames around twice their nominal scale were a 305 mm lens catalogued as 153, so falling back to nominal scale was right. The flight logbooks and the spacing between adjacent frames, read per roll-height over all 252 sampled frames beyond the band, split it: **24 roll-heights (120 sampled frames; their keys reach at most 3,227 catalogue frames, fewer once terrain brings some inside the band, where they were already sized from their height) carry the height the crew flew beside a `scale` recorded too small** — mostly 1972–76 rolls at half the true denominator — and ship in `flying_height_rolls.csv` at factor 1, `scale_wrong`, under a new `tail` value, `"near_upper"`. The fallback drew them at 1/r of their width, half at r = 2; they are now `"corrected_roll_table"` and sized from their height. On 21 roll-heights (82 frames) the logbook writes a 12" lens, which confirms the lens reading there, and those stay on nominal scale, as do the rest; each is listed in `flying_height_rolls_excluded.csv` with its reason. diff --git a/R/fly_georef.R b/R/fly_georef.R index 11a36c9..747f9ba 100644 --- a/R/fly_georef.R +++ b/R/fly_georef.R @@ -15,17 +15,17 @@ #' exist. #' @param overwrite If `FALSE` (default), skip files that already exist. #' @param mask How to handle the black collar around the exposed frame. `"border"` -#' (default) masks it with [fly_mask()]; `"none"` reproduces the pre-0.11.0 warp -#' exactly, including its `srcnodata` handling. +#' (default) masks it with [fly_mask()]; `"none"` warps unmasked, applying `srcnodata` +#' to every frame as before v0.11.0. Output carries an alpha band either way. #' @param mask_threshold How close to black a pixel must be to count as collar. Passed #' to [fly_mask()]; the default is measured, see there. -#' @param srcnodata Source nodata value passed to GDAL warp, matched **exactly**. Now -#' defaults to `NULL`, and is an error alongside `mask = "border"` — the two are -#' different answers to one question and combining them silently undoes the mask (see -#' **Nodata handling**). It reached `"0"` before v0.11.0, which was close to useless: -#' scanned black runs 3-12, so it masked a median of 0.16% of a frame against the 3.11% -#' actually there, and 128 of 264 measured frames had under a tenth of their collar -#' removed. +#' @param srcnodata Source nodata value passed to GDAL warp, matched **exactly**, or +#' `NULL` (default). With `mask = "none"` it applies to every frame. With +#' `mask = "border"` it is the **fallback**: it applies only to a frame whose mask +#' [fly_mask()] declined, and never alongside a mask that ran (see **Nodata handling**). +#' On its own it is close to useless as a collar mask: scanned black runs 3-12, so +#' `"0"` masked a median of 0.16% of a frame against the 3.11% actually there, and 128 +#' of 264 measured frames had under a tenth of their collar removed. #' @param dem Optional elevation raster passed to [fly_footprint()], sizing each #' frame from its height above ground instead of the reported scale. See the #' **Terrain** section of [fly_footprint()]. @@ -127,8 +127,13 @@ #' #' **Nodata handling:** two sources of unwanted black pixels, handled separately. #' -#' 1. **Warp fill** — GDAL creates black pixels outside the rotated source frame. RGB -#' images get an alpha band (`-dstalpha`); grayscale use `dstnodata=0`. Unchanged. +#' 1. **Warp fill** — GDAL creates pixels outside the rotated source frame. Every output +#' carries an alpha band (`-dstalpha`) marking them, so **grayscale is written as 2 +#' bands and RGB as 4** for a Byte grayscale or RGB source. Grayscale used +#' `-dstnodata 0` before v0.19.0, and GDAL kept a genuine 0 inside the frame from +#' reading as fill by rewriting it as 1 — silently on sf's GDAL 3.8.5 — or, where +#' `srcnodata = "0"` was also given, deleted it. A value cannot be both content and +#' the fill marker. #' 2. **The frame collar** — film holder edges, fiducial marks and chamfered corners. #' Since v0.11.0 this is [fly_mask()]: a flood fill seeded from the image border, so #' only darkness *reachable from the edge* is masked and interior dark water survives. @@ -140,12 +145,24 @@ #' present, so it did not mask the borders — and the real black it was said to cost is a #' median 0.16% of the frame, which *is* the collar rather than shadow. #' -#' **`srcnodata` and `mask = "border"` are mutually exclusive, and GDAL will not say so.** -#' All combinations of `-srcalpha` and `-srcnodata` run clean and return the expected band -#' count. What they do is delete the interior black the mask exists to keep: on a synthetic -#' frame carrying an 11x11 block of true black, adding `-srcnodata "0 0 0"` to the masked -#' warp removed exactly those 121 pixels. So `fly_georef()` raises the error GDAL does not. -#' Pass `mask = "none"` to get the old behaviour, `srcnodata` and all. +#' **`srcnodata` with `mask = "border"` is a per-frame fallback, never a second mask.** +#' [fly_mask()] declines some frames — among them an unreadable or non-8-bit source, and a +#' mask that floods into the interior; `fly_mask()`'s `reason` column lists every path — +#' and a declined frame is warped unmasked. Given no +#' `srcnodata`, its collar is written as opaque data. Given one, the declined frame is +#' warped with `-srcnodata` and every masked frame with `-srcalpha` alone. The two are +#' never handed to GDAL together, because GDAL would apply both and the mask would lose: +#' on a synthetic frame carrying an 11x11 block of true black, adding `-srcnodata "0 0 0"` +#' to the masked warp removed exactly those 121 pixels. Before v0.19.0 this pair was +#' refused on the premise that both reached GDAL; they never did. +#' +#' "Declined" means [fly_mask()] returned `masked = FALSE`; an error inside it fails the +#' frame instead. A declined frame given no `srcnodata` is warned about, since its collar +#' reaches the output as data. The fallback is a weak last resort: it matches **exact** +#' values only, so it catches a collar written at exactly 0 and misses one scanned at +#' 3-12, and on the frame it reaches it also removes any true black inside the frame. On +#' all 10,105 public thumbnails measured (the 264 `mask_threshold` was calibrated on among +#' them), the mask declined none. #' #' **Accuracy:** footprints assume a nadir camera angle, and without `dem` #' they also assume flat terrain. Passing `dem` sizes each frame from its @@ -179,15 +196,9 @@ fly_georef <- function(fetch_result, photos_sf, } mask <- match.arg(mask) - # Refused rather than silently dropped. GDAL accepts both together and then deletes the - # interior black the mask exists to preserve, reporting nothing - so a caller who set - # `srcnodata` deliberately must be told their instruction and the mask disagree, not - # have one of them quietly win. - if (identical(mask, "border") && !is.null(srcnodata)) { - stop("`srcnodata` and `mask = \"border\"` are two answers to one question and GDAL ", - "applies both: the mask keeps interior black and `srcnodata` then deletes it, ", - "silently. Drop `srcnodata`, or pass `mask = \"none\"` to use it.", call. = FALSE) - } + # `srcnodata` alongside `mask = "border"` is the fallback for frames whose mask declines, + # not a second mask: `fly_georef_warp_opts()` emits one or the other per frame, never + # both. Refused before v0.19.0 on the mistaken premise that both reached GDAL (fly#69). if (identical(mask, "border")) fly_check_threshold(mask_threshold, "mask_threshold") auto_rotation <- identical(rotation, "auto") @@ -476,6 +487,14 @@ georef_one <- function(src, fp, out_file, srcnodata = NULL, rotation = 180, if (isTRUE(res$masked) && file.exists(mask_file)) { warp_src <- mask_file masked <- TRUE + } else if (is.null(srcnodata)) { + # The worst of the four mask x srcnodata outcomes (fly#69): no mask and no fallback, + # so the collar is warped in as opaque data. Not every decline path warns inside + # `fly_mask_one()` — a non-Byte source returns quietly — so it is named here. + warning(basename(src), ": the frame-border mask declined (", res$reason, ") and ", + "no `srcnodata` was given, so the collar is written as image data. Pass ", + "`srcnodata` as a fallback for such frames. ", + "See `inst/notes/border-masking.md`.", call. = FALSE) } } @@ -507,15 +526,17 @@ georef_one <- function(src, fp, out_file, srcnodata = NULL, rotation = 180, #' [fly_georef_gcps()] is — it is a thing that can be wrong while everything around it #' looks healthy, and an option vector is checkable offline where a warped GeoTIFF is not. #' -#' Three rules, and the first is the one that matters: +#' Three rules: #' #' * a masked source is read through `-srcalpha`, which forces the LAST band to be the -#' alpha band and excludes it from the warped band list. That is what keeps the output -#' band count identical to a pre-0.11.0 run — grayscale 1 band, RGB 4. -#' * `-srcnodata` is emitted only when there is no mask. The two together are refused at -#' the `fly_georef()` boundary; this function must not paper over a combination that -#' reaches it, so it simply never emits both. -#' * warp fill is unchanged: `-dstalpha` for RGB, `-dstnodata 0` for grayscale. +#' alpha band and excludes it from the warped band list, so masking does not change the +#' output band count. +#' * `-srcnodata` is emitted only when there is no mask. This `else if` is what makes +#' `srcnodata` a safe fallback under `mask = "border"`: GDAL given both applies both, +#' and `-srcnodata` then deletes the interior black the mask kept. +#' * warp fill is always `-dstalpha`, so grayscale is 2 bands and RGB 4 (fly#56). Nothing +#' is marked by value on the output. A genuine 0 still becomes transparent on the +#' `-srcnodata` leg, which reads it as source nodata; that is the fallback's known cost. #' #' @param n_bands Band count of the source, before any alpha band is appended. #' @param srcnodata The `srcnodata` argument, or `NULL`. @@ -533,7 +554,7 @@ fly_georef_warp_opts <- function(n_bands, srcnodata, masked) { opts <- c(opts, "-srcnodata", src_val) } - if (is_rgb) c(opts, "-dstalpha") else c(opts, "-dstnodata", "0") + c(opts, "-dstalpha") } #' Convert flight bearing to GCP rotation diff --git a/data-raw/mask_calibrate-border_threshold.R b/data-raw/mask_calibrate-border_threshold.R index 1f613ca..f944811 100644 --- a/data-raw/mask_calibrate-border_threshold.R +++ b/data-raw/mask_calibrate-border_threshold.R @@ -279,8 +279,9 @@ for (src in reps) { has_gcp <- any(grepl("GCP", readLines(v, warn = FALSE))) o <- file.path(work, "chk_warp.tif") - opts <- c("-t_srs", "EPSG:3005", "-r", "bilinear", "-srcalpha") - opts <- if (nb0 >= 3) c(opts, "-dstalpha") else c(opts, "-dstnodata", "0") + # The package's own option vector, not a copy of it: this line hard-coded the pre-fly#56 + # grayscale fill (`-dstnodata 0`) and would have kept printing the old band count. + opts <- fly_georef_warp_opts(nb0, NULL, masked = TRUE) sf::gdal_utils("warp", source = v, destination = o, options = opts) cat(sprintf("src bands %d -> nearblack %d [%s] -> vrt gcps %s -> warp %d bands\n", diff --git a/data-raw/mask_measure-interior_zeros.R b/data-raw/mask_measure-interior_zeros.R new file mode 100644 index 0000000..3df1eba --- /dev/null +++ b/data-raw/mask_measure-interior_zeros.R @@ -0,0 +1,225 @@ +# mask_measure-interior_zeros.R — the two measurements fly#56 asked for before the +# georeference output contract changed. +# +# 1. How much genuine black a grayscale frame loses to `-dstnodata 0`. Until v0.19.0 a +# grayscale output marked "no data" by the value 0, so any pixel of real content at +# exactly 0 that survived the frame-border mask came out as nodata or as 1 — either way +# not the value scanned. Counted on the +# SOURCE, after `fly_mask_one()`: a pixel that is 0 and still opaque in the masked copy +# is one the old output deleted. Bilinear resampling can also produce a 0 from a +# neighbourhood of zeros, so the output-side loss is of the same order but not +# identical; this is the source-side count and is labelled so. For isotropic pixels (these +# thumbnails, on square footprints) it is exact for an axis-aligned warp and an upper +# bound for a rotated one, where bilinear resampling turns a lone 0 among brighter +# neighbours into a non-zero output. An anisotropic scan resamples even axis-aligned. +# 1b. The same loss measured on the OUTPUT, over the calibration grayscale frames only: +# each masked frame is warped twice through one GCP VRT, at two bearings — axis-aligned +# (a frame with no flight bearing) and 30 degrees — once with the pre-fly#56 +# options (`-srcalpha -dstnodata 0`) and once with today's (`-srcalpha -dstalpha`). The +# two share a grid, so a cell opaque at 0 in the new output that the old one holds as +# nodata (deleted) or as 1 (rewritten by GDAL to dodge the nodata value) is a true 0 the +# old contract lost. On sf's GDAL 3.8.5 it is all rewriting and no deletion, and the +# axis-aligned count equals the source-side count. +# 2. How often `fly_mask_one()` declines at the shipped constants. A declined frame is +# warped unmasked, which is where `srcnodata` as a fallback (fly#69, absorbed into +# fly#56) does any work. +# +# `inst/extdata/mask_border_sweep.csv` answers neither: it measures the mask's extent, +# not what is left at 0 and not whether `fly_mask_one()` accepted it. +# +# Everything is public: the airphoto thumbnails at openmaps.gov.bc.ca, as fetched by +# `fly_fetch(type = "thumbnail")`. Run over two populations: +# +# * "calibration" — the 264 frames `mask_border_sweep.csv` was measured on, so the +# figures share a denominator with every other number in `inst/notes/border-masking.md` +# * "directory" — every thumbnail under `thumb_dir` today, which has grown since +# +# Usage: +# Rscript data-raw/mask_measure-interior_zeros.R [out_csv] [workers] +# +# Writes a per-frame CSV (default: data-raw/.cache/mask_interior_zeros.csv, gitignored) +# and prints a producer line for every figure the note and NEWS quote. + +pkgload::load_all(quiet = TRUE) + +args <- commandArgs(trailingOnly = TRUE) +thumb_dir <- if (length(args) >= 1) args[[1]] else "~/Projects/repo/stac_airphoto_bc/data/raw/thumbs" +out_csv <- if (length(args) >= 2) args[[2]] else "data-raw/.cache/mask_interior_zeros.csv" +workers <- if (length(args) >= 3) as.integer(args[[3]]) else 8L +thumb_dir <- path.expand(thumb_dir) +stopifnot(dir.exists(thumb_dir)) + +files <- list.files(thumb_dir, pattern = "\\.(jpg|jpeg|tif|tiff)$", + full.names = TRUE, recursive = TRUE, ignore.case = TRUE) +message("thumbnails: ", length(files), " under ", thumb_dir) +stopifnot(length(files) > 0) + +calib <- unique(utils::read.csv(system.file("extdata", "mask_border_sweep.csv", + package = "fly"))$file) + +measure_one <- function(src) { + threshold <- fly_mask_threshold() + out <- tempfile(fileext = ".tif") + on.exit(unlink(out), add = TRUE) + row <- suppressWarnings(fly_mask_one(src, out, threshold)) + + r <- suppressWarnings(terra::rast(src)) + n_bands <- terra::nlyr(r) + base <- data.frame(file = basename(src), year = basename(dirname(src)), + bands = n_bands, masked = isTRUE(row$masked), + reason = if (is.na(row$reason)) "" else row$reason, + n_px = terra::ncell(r), zero_src = NA_real_, zero_kept = NA_real_, + stringsAsFactors = FALSE) + if (n_bands != 1L) return(base) + + v <- terra::values(r, mat = FALSE) + zero <- !is.na(v) & v == 0 + base$zero_src <- sum(zero) + # Declined: the frame is warped unmasked, so under the old contract every source zero + # became nodata. Masked: only the zeros the mask left opaque are content being lost — + # zeros inside the masked collar were meant to go. + base$zero_kept <- if (isTRUE(row$masked) && file.exists(out)) { + m <- suppressWarnings(terra::rast(out)) + alpha <- terra::values(m[[terra::nlyr(m)]], mat = FALSE) + sum(zero & alpha == 255) + } else { + sum(zero) + } + base +} + +# The output-side count (1b), at a given bearing. It depends on the bearing, and that is +# the finding rather than a detail: an axis-aligned warp (every frame with no flight +# bearing) of an isotropic frame lands output cells on source pixels, so bilinear +# resampling reproduces every value and every opaque 0 is lost; a rotated warp mixes neighbours and loses fewer. An +# earlier run measured 30 degrees only and was quoted as the loss. Sized so an output cell +# is about one source pixel. +measure_output <- function(src, bearing) { + threshold <- fly_mask_threshold() + masked <- tempfile(fileext = ".tif") + vrt <- tempfile(fileext = ".vrt") + old <- tempfile(fileext = ".tif") + new <- tempfile(fileext = ".tif") + on.exit(unlink(c(masked, vrt, old, new)), add = TRUE) + row <- suppressWarnings(fly_mask_one(src, masked, threshold)) + if (!isTRUE(row$masked)) { + return(data.frame(file = basename(src), lost_out = NA_real_, shifted_out = NA_real_, + frame_out = NA_real_, zero_new = NA_real_, error = NA_character_)) + } + + dm <- fly_gdal_dim(fly_gdal_info(src)) + ring <- fly_rectangles(matrix(c(1.2e6, 9e5), ncol = 2), dm[1] / 2, dm[2] / 2, bearing = bearing) + gcp <- fly_georef_gcps(dm[1], dm[2], sf::st_coordinates(ring)[1:4, 1:2, drop = FALSE], 0) + gcp_args <- unlist(lapply(seq_len(nrow(gcp)), function(j) c("-gcp", unname(gcp[j, ])))) + sf::gdal_utils("translate", source = masked, destination = vrt, + options = c("-of", "VRT", "-a_srs", "EPSG:3005", gcp_args)) + base_opts <- c("-t_srs", "EPSG:3005", "-r", "bilinear", "-srcalpha") + sf::gdal_utils("warp", source = vrt, destination = old, + options = c(base_opts, "-dstnodata", "0")) + sf::gdal_utils("warp", source = vrt, destination = new, + options = fly_georef_warp_opts(1L, NULL, masked = TRUE)) + + o <- terra::values(suppressWarnings(terra::rast(old)), mat = FALSE) + n <- terra::values(suppressWarnings(terra::rast(new)), mat = TRUE) + inside <- n[, 2] == 255 + # Two ways the old output could lose a true 0, counted separately. Nodata inside the + # frame is deletion. A 1 where today's output holds 0 is GDAL moving a real value off + # the nodata value rather than let it read as fill — silent on sf's GDAL 3.8.5, and the + # "Value 0 ... changed to 1" message fly#68 saw printed on the Windows runner. + zero_new <- inside & n[, 1] == 0 + data.frame(file = basename(src), lost_out = sum(is.na(o) & inside), + shifted_out = sum(zero_new & !is.na(o) & o == 1), + frame_out = sum(inside), zero_new = sum(zero_new), error = NA_character_) +} + +# PSOCK, not fork: see CLAUDE.md on mclapply and GDAL. Every use of the cluster sits inside +# one `finally`, so an error anywhere leaves no detached workers behind. +cl <- parallel::makePSOCKcluster(workers) +bearings <- c(NA, 30) +run <- tryCatch({ + invisible(parallel::clusterEvalQ(cl, pkgload::load_all(quiet = TRUE))) + # A closure defined here is not on the workers; without this every frame "errors" and + # the report prints zeros that read as a measurement. + parallel::clusterExport(cl, c("measure_one", "measure_output")) + res <- do.call(rbind, parallel::parLapplyLB(cl, files, function(f) { + tryCatch(measure_one(f), error = function(e) { + data.frame(file = basename(f), year = basename(dirname(f)), bands = NA_integer_, + masked = NA, reason = paste("error:", conditionMessage(e)), + n_px = NA_real_, zero_src = NA_real_, zero_kept = NA_real_) + }) + })) + # Checked before the output pass: a frame that errored here would otherwise drop out of + # the calibration set below without a word, and a doomed run would still do the warps. + n_err <- sum(grepl("^error:", res$reason)) + if (n_err > 0) { + print(utils::head(unique(res$reason[grepl("^error:", res$reason)]))) + stop(n_err, " of ", nrow(res), " frames errored; refusing to report a partial measurement.") + } + res$calibration <- res$file %in% calib + + gray_calib <- files[basename(files) %in% res$file[res$calibration & res$bands %in% 1L]] + out_side <- lapply(bearings, function(b) { + do.call(rbind, parallel::parLapplyLB(cl, gray_calib, function(f, b) { + tryCatch(measure_output(f, b), error = function(e) { + data.frame(file = basename(f), lost_out = NA_real_, shifted_out = NA_real_, + frame_out = NA_real_, zero_new = NA_real_, + error = conditionMessage(e)) + }) + }, b = b)) + }) + list(res = res, out_side = out_side) +}, finally = parallel::stopCluster(cl)) +res <- run$res +out_side <- run$out_side +names(out_side) <- ifelse(is.na(bearings), "axis-aligned", paste(bearings, "degrees")) +out_err <- unlist(lapply(out_side, function(d) if ("error" %in% names(d)) d$error[!is.na(d$error)])) +if (length(out_err)) { + print(utils::head(unique(out_err))) + stop(length(out_err), " output-side warps errored; refusing to report a partial measurement.") +} + +dir.create(dirname(out_csv), recursive = TRUE, showWarnings = FALSE) +utils::write.csv(res, out_csv, row.names = FALSE) +message("wrote ", out_csv) + +report <- function(d, label) { + cat("\n== ", label, ": ", nrow(d), " frames ==\n", sep = "") + cat("errors:", sum(grepl("^error:", d$reason)), "\n") + cat("bands:\n") + print(table(d$bands, useNA = "ifany")) + cat("mask declined:", sum(d$masked %in% FALSE), "of", sum(!is.na(d$masked)), "\n") + if (any(d$masked %in% FALSE)) print(table(d$reason[d$masked %in% FALSE])) + g <- d[d$bands %in% 1L, ] + frac <- g$zero_kept / g$n_px + cat("grayscale frames:", nrow(g), "\n") + cat(" with any source pixel at exactly 0:", sum(g$zero_src > 0), "\n") + cat(" with a 0 the mask left opaque (lost to -dstnodata 0):", sum(g$zero_kept > 0), "\n") + cat(" lost fraction of frame, over those frames: median", + signif(stats::median(frac[g$zero_kept > 0]), 3), " max", signif(max(frac), 3), "\n") + cat(" lost pixels, over all grayscale frames: total", sum(g$zero_kept), + " frames above 0.1%:", sum(frac > 0.001), "\n") +} + +report(res[res$calibration, ], "calibration (mask_border_sweep.csv)") +for (lab in names(out_side)) { + g <- out_side[[lab]] + hit <- g$lost_out + g$shifted_out + f <- hit / g$frame_out + cat("\n== output side, calibration grayscale, warp ", lab, ", GDAL ", + sf::sf_extSoftVersion()[["GDAL"]], " ==\n", sep = "") + cat("frames measured:", sum(!is.na(hit)), "of", nrow(g), "\n") + cat(" true 0 deleted (nodata inside the frame): frames", sum(g$lost_out > 0, na.rm = TRUE), + " pixels", sum(g$lost_out, na.rm = TRUE), "\n") + cat(" true 0 rewritten as 1: frames", sum(g$shifted_out > 0, na.rm = TRUE), + " pixels", sum(g$shifted_out, na.rm = TRUE), "\n") + cat(" every opaque 0 today accounted for by one of the two:", + isTRUE(all(g$zero_new == hit, na.rm = TRUE)), "\n") + cat(" affected share of frame, over affected frames: median", + signif(stats::median(f[hit > 0], na.rm = TRUE), 3), " max", signif(max(f, na.rm = TRUE), 3), + " above 0.1%:", sum(f > 0.001, na.rm = TRUE), "\n") + cat(" median affected pixels over affected frames:", stats::median(hit[hit > 0], na.rm = TRUE), "\n") + top <- g[order(-f), c("file", "lost_out", "shifted_out", "frame_out")] + top$share <- signif((top$lost_out + top$shifted_out) / top$frame_out, 3) + print(utils::head(top, 5), row.names = FALSE) +} +report(res, "directory") diff --git a/inst/notes/border-masking.md b/inst/notes/border-masking.md index 5e3db9e..b8c834f 100644 --- a/inst/notes/border-masking.md +++ b/inst/notes/border-masking.md @@ -213,33 +213,72 @@ property; skipping would cost a georeferenceable frame. **A mask fraction of zero is not a warning.** 27 of 264 frames legitimately have no collar. -## Nothing downstream changes band count +## Output shape: every output carries one alpha band -Verified end to end on sf's GDAL 3.8.5: +Since fly#56 (v0.19.0), measured on macOS with sf's GDAL 3.8.5 (the test suite asserts the +same counts on the three CI platforms): ``` -gray 1 band -> nearblack -setalpha -> 2 -> translate -of VRT (+GCPs) -> warp -srcalpha -dstnodata 0 -> 1 band out -rgb 3 band -> nearblack -setalpha -> 4 -> translate -of VRT (+GCPs) -> warp -srcalpha -dstalpha -> 4 bands out +gray 1 band -> nearblack -setalpha -> 2 -> translate -of VRT (+GCPs) -> warp -srcalpha -dstalpha -> 2 bands out +rgb 3 band -> nearblack -setalpha -> 4 -> translate -of VRT (+GCPs) -> warp -srcalpha -dstalpha -> 4 bands out ``` `-srcalpha` forces the last band to be read as alpha and excludes it from the warped band -list, so today's output contract holds on both paths. The GCP step is a **VRT**, which -carries a `` that gdalwarp reads — so masking removes a full-size temp copy -rather than adding one. At 9600 x 9000 x 4 that is the difference between roughly 350 MB -and 700 MB of scratch per frame. - -Two wrinkles, both measured: - -- On a **grayscale** source `-setalpha` writes `ColorInterp=Undefined` on band 2, not - `Alpha`. RGB gets `Alpha` correctly. `-srcalpha` works either way because it forces the - last band regardless — but nothing may key on `ColorInterp` for the grayscale path. -- Grayscale output carries `-dstnodata 0` rather than an alpha band, which is strictly - weaker: it cannot express partial coverage at the mask boundary, and a genuine 0-valued - pixel inside the frame is indistinguishable from a masked one — the same class of defect - the mask exists to fix, on the output side. 182 of the 264 measured frames are grayscale. - Changing it moves the band count of every grayscale output from 1 to 2, which - `stac_airphoto_bc` consumes, so it is tracked separately as fly#56 rather than bundled - here. +list, so masking does not change the band count: a Byte grayscale or RGB source comes out +with its own bands plus one alpha, masked or not, and no `NoData Value`. The GCP step is a +**VRT**, which carries a `` that gdalwarp reads — so masking removes a full-size +temp copy rather than adding one. At 9600 x 9000 x 4 that is the difference between +roughly 350 MB and 700 MB of scratch per frame. The alpha band's opaque value is its +datatype's maximum: 255 on Byte, **65535 on a UInt16 output** — read alpha as "0 is fill", +never as "255 is opaque". + +**Before v0.19.0 grayscale was `-dstnodata 0`, 1 band.** That was kept in v0.11.0 so the +mask could ship without moving `stac_airphoto_bc`, and it was wrong on two counts: + +- **It was not one number.** The three-platform CI added in fly#52 found on its first run + that the Windows runner gave 2 bands for a masked grayscale frame against 1 with masking + off, while ubuntu and macOS gave 1 (fly#68). Which count a caller got depended on the + GDAL build, not on the frame. +- **It collided with content — by rewriting it, not deleting it.** A genuine 0 inside the + frame is the nodata value, so GDAL moves it to **1** to keep it from reading as fill. The + Windows runner printed this (*"Value 0 in the source dataset has been changed to 1 ... to + avoid being treated as NoData"*, fly#68); sf's GDAL 3.8.5 on macOS does the same and says + nothing. Only where `srcnodata = "0"` was also given is the 0 deleted instead. Measured + on the output by `data-raw/mask_measure-interior_zeros.R`: the 182 calibration grayscale + frames masked and warped twice, old options against new, on one grid. + + | warp | frames with black rewritten as 1 | pixels | median per frame | max of a frame | + | --- | --- | --- | --- | --- | + | axis-aligned (a frame with no flight bearing) | **161 of 182** | 242,439 | 47 | **3.6%** | + | rotated 30 degrees | 41 of 182 | 12,682 | 13 | 0.26% | + + None was deleted on this, the default, path. The worst frames are all one 1994 roll, + bcb94081. The loss depends on the bearing because an axis-aligned warp of these frames + lands output cells on source pixels, so bilinear resampling reproduces every value, while + a rotated one mixes neighbours and a lone 0 among brighter pixels comes out non-zero. So + the source-side count (an opaque 0 after masking) is exact for an axis-aligned warp and + an upper bound for a rotated one — **for isotropic pixels**, which these 1250 x 1250 + thumbnails on square footprints have by construction. A 9600 x 9000 scan on a square + footprint resamples even axis-aligned: in a probe with 200 isolated zeros, 200 x 200 + kept all 200 and 200 x 188 kept none. An earlier draft of this note measured only 30 degrees + and quoted it as the loss. Silent value corruption in kind — the class of defect fly#23 + fixed on the input side (`srcnodata = "0"` deleting real black), reintroduced on the + output. + +What the alpha band does **not** buy is partial coverage: the section below measures two +distinct output alpha values on a rotated warp, 0 and 255, so do not cite fractional alpha +at the mask boundary as a reason for the change. + +One wrinkle, measured: on a **grayscale** source `-setalpha` writes `ColorInterp=Undefined` +on band 2, not `Alpha`. `-srcalpha` works regardless because it forces the last band, and +the warped output's band 2 is `Alpha` because `-dstalpha` creates it. Nothing may key on +the *masked intermediate's* `ColorInterp`. + +**A consumer that copies the raster must carry the alpha interpretation itself.** +`stac_airphoto_bc`'s COG writer, run on a v0.19.0 grayscale output, lost it: rasterio +ignores `colorinterp = (gray, alpha)` on a 2-band in-memory GTiff unless the dataset is +created with `alpha="YES"`. Its own round-trip check refused the file, so the failure was +loud (stac_airphoto_bc#36). ### The warp does not pull masked black into the pixels beside it @@ -269,10 +308,9 @@ Two ways this check goes vacuous, both met on the way to the number above: Had a fringe been present the remedy would have been to erode the mask inward by a pixel, **not** to change the resampling. -### `-srcnodata` and the mask are mutually exclusive, and GDAL will not tell you +### `-srcnodata` and the mask are never applied together — so `srcnodata` is a fallback -`fly_georef()` refuses the combination. The reason is **not** that GDAL rejects it — it -does not. All four forms run clean and produce the expected band count: +GDAL accepts both at once. All four forms run clean and produce the expected band count: | warp options | result | | --- | --- | @@ -282,11 +320,10 @@ does not. All four forms run clean and produce the expected band count: | `-srcnodata "0 0 0" -dstalpha` (no srcalpha) | ok, 4 bands | An early draft of this note asserted the three-value form was a length mismatch against a -four-band source. Measured, it is not, and neither is the four-value form. **The package -refuses the combination precisely because GDAL accepts it silently.** +four-band source. Measured, it is not, and neither is the four-value form. -What it silently does, measured on a 100 x 100 synthetic frame with an 8-pixel collar and -an 11 x 11 block of *true black* (value 0) at the centre: +What both at once silently does, measured on a 100 x 100 synthetic frame with an 8-pixel +collar and an 11 x 11 block of *true black* (value 0) at the centre: | | opaque | transparent | | --- | --- | --- | @@ -294,9 +331,33 @@ an 11 x 11 block of *true black* (value 0) at the centre: | plus `-srcnodata "0 0 0"` | **6279** | 3721 | The difference is 121 pixels — exactly the interior block. Adding `-srcnodata` to a masked -warp deletes the real black content the mask exists to preserve, which is the defect -fly#23 was filed about, reintroduced by the option that was supposed to fix it. Nothing is -reported, so the package raises the error GDAL does not. +warp deletes the real black content the mask exists to preserve. + +**`fly_georef_warp_opts()` never emits both.** It is an `else if`: `-srcalpha` when the frame +was masked, `-srcnodata` only when it was not. v0.11.0 nonetheless refused +`mask = "border"` together with `srcnodata` at the `fly_georef()` boundary, on the premise +that both would reach GDAL. They never did; the refusal only made one combination +unreachable, and it was the useful one (fly#69, absorbed into fly#56). Tabulated per frame: + +| mask ran? | `srcnodata` | warp options | collar | +| --- | --- | --- | --- | +| yes | `"0"` | `-srcalpha` (srcnodata unused) | masked | +| yes | `NULL` | `-srcalpha` | masked | +| declined | `"0"` | `-srcnodata 0` | exact zeros only | +| declined | `NULL` | neither | **written as data** — warned since v0.19.0 | + +So since v0.19.0 the pair is accepted and `srcnodata` means "for frames whose mask +declined". "Declined" is `fly_mask_one()` returning `masked = FALSE`, for any of the reasons +in `fly_mask()`'s `reason` column; an error inside it fails the frame instead. The fallback +is a weak last resort: it matches **exact** values, so it misses a collar scanned at 3-12, +and on the frame it reaches it also deletes true black — the 121-pixel row above. + +How much it matters, measured by `data-raw/mask_measure-interior_zeros.R`: the mask +declined **0 of 264** calibration thumbnails and **0 of all 10,105** in the directory +today. Every thumbnail is 8-bit and none trips the interior cap, so on this population the +fallback never fires. Its reach is the decline paths thumbnails cannot exercise — a +non-Byte full-resolution scan above all — which is the population the section below says +this note cannot reach. ## What was tried and rejected diff --git a/man/fly_georef.Rd b/man/fly_georef.Rd index 839d5be..2a6d74d 100644 --- a/man/fly_georef.Rd +++ b/man/fly_georef.Rd @@ -32,19 +32,19 @@ exist.} \item{overwrite}{If \code{FALSE} (default), skip files that already exist.} \item{mask}{How to handle the black collar around the exposed frame. \code{"border"} -(default) masks it with \code{\link[=fly_mask]{fly_mask()}}; \code{"none"} reproduces the pre-0.11.0 warp -exactly, including its \code{srcnodata} handling.} +(default) masks it with \code{\link[=fly_mask]{fly_mask()}}; \code{"none"} warps unmasked, applying \code{srcnodata} +to every frame as before v0.11.0. Output carries an alpha band either way.} \item{mask_threshold}{How close to black a pixel must be to count as collar. Passed to \code{\link[=fly_mask]{fly_mask()}}; the default is measured, see there.} -\item{srcnodata}{Source nodata value passed to GDAL warp, matched \strong{exactly}. Now -defaults to \code{NULL}, and is an error alongside \code{mask = "border"} — the two are -different answers to one question and combining them silently undoes the mask (see -\strong{Nodata handling}). It reached \code{"0"} before v0.11.0, which was close to useless: -scanned black runs 3-12, so it masked a median of 0.16\% of a frame against the 3.11\% -actually there, and 128 of 264 measured frames had under a tenth of their collar -removed.} +\item{srcnodata}{Source nodata value passed to GDAL warp, matched \strong{exactly}, or +\code{NULL} (default). With \code{mask = "none"} it applies to every frame. With +\code{mask = "border"} it is the \strong{fallback}: it applies only to a frame whose mask +\code{\link[=fly_mask]{fly_mask()}} declined, and never alongside a mask that ran (see \strong{Nodata handling}). +On its own it is close to useless as a collar mask: scanned black runs 3-12, so +\code{"0"} masked a median of 0.16\% of a frame against the 3.11\% actually there, and 128 +of 264 measured frames had under a tenth of their collar removed.} \item{rotation}{Image rotation in degrees clockwise. One of \code{"auto"}, \code{0}, \code{90}, \code{180}, or \code{270}. \strong{Applies only to frames whose footprint was @@ -155,8 +155,13 @@ about, for film as well as digital. \strong{Nodata handling:} two sources of unwanted black pixels, handled separately. \enumerate{ -\item \strong{Warp fill} — GDAL creates black pixels outside the rotated source frame. RGB -images get an alpha band (\code{-dstalpha}); grayscale use \code{dstnodata=0}. Unchanged. +\item \strong{Warp fill} — GDAL creates pixels outside the rotated source frame. Every output +carries an alpha band (\code{-dstalpha}) marking them, so \strong{grayscale is written as 2 +bands and RGB as 4} for a Byte grayscale or RGB source. Grayscale used +\verb{-dstnodata 0} before v0.19.0, and GDAL kept a genuine 0 inside the frame from +reading as fill by rewriting it as 1 — silently on sf's GDAL 3.8.5 — or, where +\code{srcnodata = "0"} was also given, deleted it. A value cannot be both content and +the fill marker. \item \strong{The frame collar} — film holder edges, fiducial marks and chamfered corners. Since v0.11.0 this is \code{\link[=fly_mask]{fly_mask()}}: a flood fill seeded from the image border, so only darkness \emph{reachable from the edge} is masked and interior dark water survives. @@ -169,12 +174,24 @@ exact-zero matching removed a median of 0.16\% of a frame where 3.11\% of collar present, so it did not mask the borders — and the real black it was said to cost is a median 0.16\% of the frame, which \emph{is} the collar rather than shadow. -\strong{\code{srcnodata} and \code{mask = "border"} are mutually exclusive, and GDAL will not say so.} -All combinations of \code{-srcalpha} and \code{-srcnodata} run clean and return the expected band -count. What they do is delete the interior black the mask exists to keep: on a synthetic -frame carrying an 11x11 block of true black, adding \verb{-srcnodata "0 0 0"} to the masked -warp removed exactly those 121 pixels. So \code{fly_georef()} raises the error GDAL does not. -Pass \code{mask = "none"} to get the old behaviour, \code{srcnodata} and all. +\strong{\code{srcnodata} with \code{mask = "border"} is a per-frame fallback, never a second mask.} +\code{\link[=fly_mask]{fly_mask()}} declines some frames — among them an unreadable or non-8-bit source, and a +mask that floods into the interior; \code{fly_mask()}'s \code{reason} column lists every path — +and a declined frame is warped unmasked. Given no +\code{srcnodata}, its collar is written as opaque data. Given one, the declined frame is +warped with \code{-srcnodata} and every masked frame with \code{-srcalpha} alone. The two are +never handed to GDAL together, because GDAL would apply both and the mask would lose: +on a synthetic frame carrying an 11x11 block of true black, adding \verb{-srcnodata "0 0 0"} +to the masked warp removed exactly those 121 pixels. Before v0.19.0 this pair was +refused on the premise that both reached GDAL; they never did. + +"Declined" means \code{\link[=fly_mask]{fly_mask()}} returned \code{masked = FALSE}; an error inside it fails the +frame instead. A declined frame given no \code{srcnodata} is warned about, since its collar +reaches the output as data. The fallback is a weak last resort: it matches \strong{exact} +values only, so it catches a collar written at exactly 0 and misses one scanned at +3-12, and on the frame it reaches it also removes any true black inside the frame. On +all 10,105 public thumbnails measured (the 264 \code{mask_threshold} was calibrated on among +them), the mask declined none. \strong{Accuracy:} footprints assume a nadir camera angle, and without \code{dem} they also assume flat terrain. Passing \code{dem} sizes each frame from its diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/README.md b/planning/archive/2026-09-issue-56-georef-output-contract/README.md new file mode 100644 index 0000000..be49270 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/README.md @@ -0,0 +1,42 @@ +## Outcome + +Every `fly_georef()` output now carries one alpha band: grayscale is 2 bands (Gray + Alpha, +no NoData) where it was 1 band with `-dstnodata 0`, and RGB stays 4. `mask = "border"` with +`srcnodata` is accepted, and `srcnodata` is the fallback for frames whose mask declined. +`fly_georef_warp_opts()` was always an `else if`, so the v0.11.0 refusal guarded a pair GDAL +never received. `georef_one()` warns when a mask declines with no fallback. Absorbs #68 and +#69. The lesson is in the measurement: the headline number moved twice under review, and +the old defect turned out to be a different mechanism than the issue said. + +## Measurement + +All on the 182 grayscale frames of the 264-thumbnail calibration set, sf's GDAL 3.8.5, +macOS. Produced by `data-raw/mask_measure-interior_zeros.R`. + +- **The old default path rewrote true black as 1 and deleted none.** + - Axis-aligned warp: 161 frames, 242,439 px, median 47 per frame, max 3.6% + (bcb94081_042). + - 30° warp: 41 frames, 12,682 px, median 13 per frame, max 0.26%. + - Deletion happened only where `srcnodata = "0"` was also given. + - #68's "Windows shifts 0 to 1" is a GDAL behaviour. 3.8.5 does it silently; other + builds print it. +- **Wrong turns, kept:** + 1. The first figure was source-side ("161 frames lost black"). It was presented as + *deletion*, which is the wrong mechanism. + 2. Review flagged it as a proxy. The replacement measured one bearing (30°) and quoted + 41 frames as the loss. + 3. The next review showed an axis-aligned warp of isotropic pixels loses every opaque 0. + So the source-side count was right for bearingless frames, and the 30° figure is the + lower end. +- **Mask decline rate:** 0 of 264, and 0 of all 10,105 thumbnails in the directory. The + fallback never fires on thumbnails; its reach is non-8-bit scans. +- **Downstream:** `stac_airphoto_bc`'s `03_cog.py` refuses gray+alpha, because rasterio + drops the alpha colorinterp without `alpha="YES"`. Its own guard caught it; filed + stac_airphoto_bc#36. A private sibling that pins the warp options was filed separately. + +## Evidence + +`data-raw/.cache/mask_interior_zeros*` (gitignored, regenerate with the script). The +reviews are `review-*.md` in this directory. + +Closed by: PR (see branch `56-georef-output-contract-a-real-alpha-band`) diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/findings.md b/planning/archive/2026-09-issue-56-georef-output-contract/findings.md new file mode 100644 index 0000000..0edea3d --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/findings.md @@ -0,0 +1,136 @@ +# Findings — Georef output contract (#56) + +## Issue context + +## Problem + +`fly_georef()` gives a **grayscale** output `-dstnodata 0` rather than a real alpha band, +while RGB gets `-dstalpha`. That was preserved deliberately in v0.11.0 (fly#23) so the +frame-border mask could ship without changing any output's band count, but it is strictly +the weaker contract: + +- `-dstnodata 0` cannot express **partial coverage** at the mask boundary. Alpha can. +- It collides with real content: a genuine 0-valued pixel inside the frame is + indistinguishable from a masked one. This is the same class of defect the mask exists to + fix — `srcnodata = "0"` was deleting real black — reintroduced on the output side. +- It reads back as `NA` rather than as a maskable band, so a consumer counting coverage has + to special-case grayscale. + +182 of the 264 measured thumbnails are grayscale, so this is the majority of the corpus. + +## Why it was not done in #23 + +Switching grayscale to alpha changes the band count of **every** grayscale output from 1 to +2. `stac_airphoto_bc` consumes those GeoTIFFs and turns them into COGs, so the change has +to be co-ordinated with that repo rather than shipped underneath it. Bundling it into #23 +would have blocked a fix for a live defect on a downstream repo's readiness. + +## What to do + +- Emit `-dstalpha` for grayscale as well, giving 2-band output. +- Decide and record whether `fly_georef()` gains an argument for this or simply changes, + and say so in NEWS as a breaking change with the band count named up front. +- Re-run the `stac_airphoto_bc` COG pipeline against the new shape before releasing. +- The measurement to make first: how many grayscale frames actually carry interior pixels + at exactly 0, i.e. how much real content the current `-dstnodata 0` is losing. + `inst/extdata/mask_border_sweep.csv` does not answer this — it measures the mask, not the + output — so it needs a new pass. + +See `inst/notes/border-masking.md`, "Nothing downstream changes band count". + + +## Absorbs #68: the Windows band count + +#68 is the same decision seen from another platform, and is closed into this issue. Kept here as acceptance: + +- On **Windows**, masking a **grayscale** frame yields **2 bands** against 1 with masking off (ubuntu and macOS give 1; RGB is 4 everywhere), so "output band counts do not change" was only ever true on the platforms it was measured on. Found by the three-platform CI on its first run ([job log](https://github.com/NewGraphEnvironment/fly/actions/runs/35680424973)). +- Same root, same runner: GDAL reports *"Value 0 in the source dataset has been changed to 1 ... to avoid being treated as NoData"*, so genuine zeros are silently shifted in the warped output — the output-side collision this issue already describes, measured. +- `tests/testthat/test-fly_georef_mask.R` (~l.200-215) **pins** the observed Windows value rather than skipping it. Emitting a real alpha band for grayscale should make all three platforms agree on 2 bands; that test and the CLAUDE.md Key Decision for #23 are then rewritten to state the new invariant unconditionally. + + +## Absorbs #69: the exported API refuses the fallback the internals already support + +#69 is closed into this issue because it changes the same branch of `fly_georef_warp_opts()`, and what it should become depends on what this issue decides for grayscale output. + +`fly_georef()` refuses `mask = "border"` together with `srcnodata` (`R/fly_georef.R:186-190`), on the grounds that GDAL would apply both and `srcnodata` would delete the interior black the mask kept. `fly_georef_warp_opts()` (`R/fly_georef.R:525-537`) does not apply both — it chooses with an `else if`. Measured on fly 0.14.1, all four combinations: + +| `masked` | `srcnodata` | warp options | +|---|---|---| +| TRUE | `"0"` | `-srcalpha` (srcnodata ignored) | +| TRUE | `NULL` | `-srcalpha` | +| FALSE | `"0"` | `-srcnodata 0` | +| FALSE | `NULL` | neither | + +So the refused pair is exactly **"mask where the mask runs, nodata the collar where it declines"** — and `fly_mask_one()` declines on four documented paths (`R/fly_mask.R:215-305`). The fourth row is the worst outcome and is reachable: a frame whose mask declined, given no `srcnodata` because the exported docs say the mask supersedes it, has its black collar warped in **as real data**. A downstream repo currently reaches the fallback by calling `fly:::georef_one()` and re-asserting the table above on every run, which pins it to a `@noRd` helper. + +Acceptance, in addition to the grayscale work above: + +- The exported API expresses the fallback: either relax the refusal where the two are not both applied, or add a `mask` value (e.g. `"border-or-nodata"`) that says it in one argument, keeping the refusal for the genuinely contradictory case. Decide after the grayscale output shape is settled — with a real alpha band, the right answer may differ. +- Reconcile the docstring at `R/fly_georef.R:143-148`, which describes both options applied together, with the code, which chooses between them. +- Measure the **mask decline rate** first; it decides how much the fallback matters. A 6-frame downstream probe saw 1 of 6 consistent with a decline, which is not a rate. + + +## Phase 1 measurements (2026-09-28) + +Producer: `data-raw/mask_measure-interior_zeros.R` over +`stac_airphoto_bc/data/raw/thumbs` (read-only), 8 PSOCK workers, 2.5 min. Per-frame CSV in +`data-raw/.cache/mask_interior_zeros.csv` (gitignored). Counted on the SOURCE after +`fly_mask_one()` at threshold 16: a pixel exactly 0 and still opaque in the masked copy is +one the old `-dstnodata 0` output wrote as nodata. + +**Calibration set (the 264 of `mask_border_sweep.csv`; 182 grayscale, 82 RGB):** +- 172 of 182 grayscale frames carry some source pixel at exactly 0; the mask removes some + of those zeros on 170 (they are collar), and leaves opaque zeros on **161 of 182**. +- Lost share of frame over those 161: median **3.0e-05** (median 47 pixels of 1.56 M), + max **3.5%**; **12** frames above 0.1%. Total 242,439 pixels. +- The tail is one roll: bcb94081 frames 040-053 (1994) hold 1-3.5% true black each. +- **Mask declined on 0 of 264.** + +**Output-side pass (docs review round 1 flagged the source count as a proxy; round 2 +found the fix measured one bearing only):** the 182 calibration grayscale frames warped +twice, old options (`-srcalpha -dstnodata 0`) against new, on sf's GDAL 3.8.5. **0 pixels +deleted at either bearing measured.** Axis-aligned: 161 frames rewritten 0→1, 242,439 px, median 47, +max 3.6% — identical to the source count. At 30°: 41 frames, 12,682 px, median 13, max +0.26%. Every opaque 0 in the new output is accounted for at both. +Synthetic check on 3.8.5: masked `-srcalpha -dstnodata 0` → 1; unmasked `-dstnodata 0` → 1; +`-srcnodata 0 -dstnodata 0` → nodata. So the 0→1 rewrite of #68 is not +Windows-specific. Whether GDAL prints it depends on the build: sf's 3.8.5 is silent, +Homebrew's GDAL 3.13 CLI prints it (review round 2). The source-side figures below are an upper +bound, kept for the record. + +**Whole directory today (10,105; 3,751 grayscale, 6,354 RGB):** 3,506 of 3,751 grayscale +frames lose some zero, median 3.1e-05, max 3.5%, 34 above 0.1%. **Declined on 0 of 10,105.** + +Reading: the collision is near-universal in *incidence* and small in *extent* — a few dozen +pixels on most frames, percent-level on a dark roll. The fallback (#69) never fires on +thumbnails: every thumbnail is 8-bit and none trips the interior cap. Its reach is the +paths thumbnails cannot exercise — non-Byte full-resolution scans and unreadable sources — +which nothing public here can measure (the catalogue carries no full-res URL; see the +calibration script header). + +Grayscale output with -dstalpha on a UInt16 source: alpha is 0/65535, not 0/255 (measured +on a synthetic INT2U frame). Consumers must read alpha as "0 = fill", not "255 = opaque". + +## Errors Encountered + +| Error | Resolution | +|-------|------------| +| PSOCK workers: `could not find function "measure_one"` on every frame, report printed zeros | `clusterExport()`; script now stops if any frame errored | +| fallback test: both legs identical | terra writes `NAflag = NA` on UInt16 as nodata 0, which GDAL honours regardless; fixture uses `NAflag = 65535` | +| fallback test: opaque count 0 | UInt16 alpha is 65535; `opaque(full =)` | + +## Phase 4: downstream COG round trip (2026-09-28) + +Producer: `georef_one()` (this branch) on two real grayscale thumbnails +(`bc5282_231`, `bcb94081_042`), then `stac_airphoto_bc/scripts/03_cog.py`'s own +`write_cog()` imported from scratch, in its conda env. Nothing written to that repo. + +- fly's output is right: 2 bands, `Gray` + `Alpha`, no `NoData Value`. +- **`03_cog.py` refuses it**, correctly and loudly: `check_same_raster()` reports + colorinterp `(gray, alpha)` → `(gray, undefined)`. The loss is in the in-memory GTiff: + rasterio ignores `mem.colorinterp = (gray, alpha)` on a 2-band dataset unless it is + created with `alpha="YES"`. With that creation option the COG keeps `(gray, alpha)` and + mask flags `[per_dataset, alpha]`. RGBA is unaffected (it already publishes). +- So the downstream change is one creation option in `write_cog()` plus the + `tests/test_cog.py` grey fixture, then a full republish of grayscale frames (every + `fly_sha` changes, so `02_georef.R` regenerates them anyway). diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/progress.md b/planning/archive/2026-09-issue-56-georef-output-contract/progress.md new file mode 100644 index 0000000..b5587ab --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/progress.md @@ -0,0 +1,30 @@ +# Progress — Georef output contract (#56) + +## Session 2026-09-28 + +- Plan-mode exploration — phases approved by user; "go all phases to pr" +- Gate decisions: grayscale just changes to 2 bands (no opt-out arg); relax the + border + srcnodata refusal rather than add a mask value +- Created branch `56-georef-output-contract-a-real-alpha-band` off main +- Scaffolded PWF baseline from issue #56 with approved phases +- Next: start Phase 1 +- Phase 1: measured interior zeros and mask decline over 264 calibration + 10,105 current + thumbnails (findings.md). Plan review returned; dispositions in review-plan.md +- Phases 2-3: tests first (red for the right reasons), then `-dstalpha` for all outputs, + refusal relaxed, declined-mask-without-fallback warning (review G4), roxygen rewritten. + Mutations checked: dropping `-srcnodata` reddens the fallback test; restoring + `-dstnodata 0` beside alpha reddens the collision test on `anyNA`, not only band count. + Three aspect tests in test-fly_georef_digital.R used UInt32 fixtures the mask declines; + given `mask = "none"` (aspect guard runs before masking). Full suite 2585 pass, 0 warn. +- /code-check on Phases 2-3: 3 rounds, 6 prose findings fixed, ended by enumeration + (review-summary.md). Committed cf47430. +- Phase 4: downstream COG round trip — 03_cog.py refuses gray+alpha (colorinterp lost in + rasterio's in-memory GTiff without alpha="YES"); filed stac_airphoto_bc#36 and an issue + in the private sibling that pins warp opts (one-directional reference). +- Phase 5: border-masking.md output-shape and srcnodata sections rewritten with producers; + CLAUDE.md Key Decision + Architecture; NEWS; calibration script now calls + fly_georef_warp_opts() rather than hard-coding the old vector. +- /code-check on Phase 5: 3 rounds, 12 findings fixed, ended by enumeration + (review-docs-summary.md). The measurement grew an output-side pass at two bearings; the + old default path rewrote true 0 as 1 (never deleted): 161/182 frames axis-aligned, 41 at + 30°. stac_airphoto_bc#36 body corrected to match. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round1.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round1.md new file mode 100644 index 0000000..c958b0a --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round1.md @@ -0,0 +1,16 @@ +# Review — docs round 1 (staged diff after cf47430, fly#56) + +## Findings + +- **[medium]** inst/notes/border-masking.md:~246 — "roll bcb94081 (1994), whose frames 040-053 each hold 1-3.5%" is false against its own producer. `data-raw/.cache/mask_interior_zeros.csv`: the roll's frames present are 040, 041, 042, 043, 052, 053, 054, 055, 069, 070. Inside 040-053, frame **052 is 0.84%** (13,134 / 1,562,500), below the stated 1% floor. Actual in-range values: 040 1.03%, 041 1.95%, 042 3.50%, 043 3.12%, 052 0.84%, 053 1.07%. The roll's other calibration frames (054 0.68%, 055 0.62%, 069 0.70%, 070 0.78%) are all below 1%. So either "each hold 1-3.5%" or the frame range is wrong. Suggested wording: "frames 040-043 and 053 hold 1-3.5%, and six more on the roll hold 0.6-0.8%." + +- **[low/medium]** inst/notes/border-masking.md:~243-246, NEWS.md:3, CLAUDE.md Key Decision ("deleted genuine black — 161 of 182") — the figures count **source** pixels that are exactly 0 and still opaque after `fly_mask_one()`. The producer labels them that way (`data-raw/mask_measure-interior_zeros.R` lines 8 and 117, "a 0 the mask left opaque (lost to -dstnodata 0)"). The prose, though, says those pixels "was deleted" from the output ("hold true black that survives the mask and was deleted — a median of 47 pixels"). The old warp was bilinear onto a rotated grid. An output pixel comes out 0, and so becomes nodata, only when the interpolated value rounds below 0.5, which effectively needs a neighbourhood of zeros. An isolated source 0 among dark-but-nonzero pixels (3-12) is resampled to a nonzero value and was never deleted. So 161/182 and "median 47 pixels" are an upper-bound proxy, not the number of pixels the old contract deleted. This matters most for the frames that carry only a few dozen zero pixels, which are most of the 161. Either qualify it as "source pixels at exactly 0 the mask left opaque, which `-dstnodata 0` would delete wherever they survive resampling", or measure on warped output. The direction of the conclusion (a value cannot be both content and fill) is unaffected. I confirmed on local GDAL 3.8.5 that `-srcalpha -dstnodata 0` does turn an 11x11 zero block into NA, so the mechanism itself is real. + +## Checked and correct + +- 161 of 182 calibration grayscale frames with zero_kept > 0; median 47 px (3.0e-05) among those; max 3.495% (bcb94081_042); 3,506 of 3,751 grayscale frames in the directory; mask declined 0 of 264 and 0 of 10,105. All reproduce from the CSV. +- `fly_georef_warp_opts()` is `if (masked) -srcalpha else if (!is.null(srcnodata)) -srcnodata`, then always `-dstalpha`. The else-if was already present at v0.11.0 (line 531), so "they never did [both reach GDAL]" holds. The fallback table in the note matches the code. +- "No NoData value" on every output: probed locally on the `-srcnodata 0 -dstalpha`, `-srcalpha -dstalpha` and `-dstalpha` legs; gdalinfo shows no NoData on any of them. +- data-raw/mask_calibrate-border_threshold.R: the script runs `pkgload::load_all(quiet = TRUE)` at line 23, so the internal `fly_georef_warp_opts()` is visible. The new call's options match the old ones plus the post-#56 `-dstalpha` for grayscale, which is the intent. +- The 65535 UInt16 alpha claim matches the test helper `opaque(path, full = 65535)` and the fallback tests. +- Public-repo check: the diff names only fly#N, stac_airphoto_bc and stac_airphoto_bc#36. The private sibling in progress.md is left unnamed. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round2.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round2.md new file mode 100644 index 0000000..6ce4966 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round2.md @@ -0,0 +1,71 @@ +# Review — docs round 2 (staged diff after cf47430, fly#56) + +Probes run in a scratch copy of the repo against the public thumbnails (read only), on +sf's GDAL 3.8.5, plus Homebrew's CLI GDAL 3.13.0 for the message check. + +## Findings + +- **[high]** `data-raw/mask_measure-interior_zeros.R` `measure_output()` (the fixed + `bearing = 30`), and every doc that quotes its result: `NEWS.md:3`, `CLAUDE.md` Key Decision + ("41 of 182 ... max 0.26%; the source-side count of 161 is an upper bound that an early draft + quoted as the loss"), `inst/notes/border-masking.md` ("41 of 182 frames", "The same script's + source-side count ... is an upper bound, not the loss: bilinear resampling turns a lone 0 + among collar-dark 3-12 into a non-zero output. An earlier draft of this note quoted it as the + loss."), and `planning/active/findings.md` ("The source-side figures below are an upper + bound"). **The 41-frame figure measures a 30-degree rotation, and the path most grayscale + output took is not rotated.** A film frame with no flight bearing is warped axis-aligned (13 of + the 20 bundled frames have none). A film frame with a bearing is refused unless the caller + supplies `rotation`. On an axis-aligned warp the output grid lines up with the source pixels, + bilinear resampling reproduces every value, and **every opaque source 0 is rewritten**. The + source-side count is then the output loss exactly, not an upper bound. + I measured this with `measure_output()`'s own code, changing only `bearing`: + + | frame | bearing 30 (what the docs quote) | bearing 0 / NA | source opaque zeros | + | --- | --- | --- | --- | + | bcb94081_042 | 3,922 rewritten (0.26%) | **54,612** rewritten (3.5%) | 54,612 | + | bc5300_010 | 31 | **173** | 173 | + + I also ran bcb94081_042 on a realistic ground footprint: half-side 2,537 m, so about 4 m + cells, with bearing NA. It rewrote 54,612 of 54,612. So the round-1 finding (b) that prompted + this pass assumed "bilinear onto a rotated grid". That assumption does not hold for the common + path, and the correction swapped a right-for-unrotated number for a wrong-for-unrotated one. + Readers get the wrong magnitude by up to about 14x, and the NEWS "at most 0.26% of one frame" + bound is false: 3.5% for that frame unrotated. Remedy: measure at bearing NA (or at both), and + state the old figures (161 of 182 frames, up to 3.5%) as the axis-aligned loss, with the + rotated result as the case where resampling masks isolated zeros. The "lone 0 among collar-dark + 3-12" mechanism only operates under rotation, and it is conjecture in any case: the output + pass did not test it. + +- **[low]** `data-raw/mask_measure-interior_zeros.R`, the second `parLapplyLB(cl, gray_calib, + measure_output)`, has no `tryCatch`. That is not silent, because an error aborts the script. + But the abort happens before `parallel::stopCluster(cl)`, so the PSOCK workers are orphaned. + Detached workers outlive their master (spatial checklist, "PSOCK workers outlive their + master"). Use `tryCatch(..., finally = stopCluster(cl))` or `on.exit`. + +- **[low]** `planning/active/findings.md` (new paragraph): "the 'Windows shifts to 1' framing + of #68 was a GDAL behaviour everywhere, only printed there". It is printed by GDAL version, not + by platform. Homebrew's GDAL 3.13.0 CLI on this Mac prints `Warning 1: Value 0 in the source + dataset has been changed to 1 ...` for the same VRT, and sf's 3.8.5 prints nothing. The + public note's wording ("sf's GDAL 3.8.5 on macOS does the same and says nothing") is accurate. + The planning file's framing is the one that is off. + +## Checked and correct + +- **Measurement validity, apart from the geometry.** Old and new outputs share a grid. Dims + and extent were identical in every probe (1708x1708 at 30 degrees, 1250x1250 unrotated). + The script does not assert this, but it holds, and a mismatch would surface as an R length + warning. terra reads the old output's nodata 0 as NA. The new alpha has only the values + {0, 255}, so `inside` misses no partial-alpha cells. Partial-alpha cells holding a new 0 + numbered 0. There is no confound: inside the frame, old and new differ nowhere except at the + 0-to-1 cells, and no old 1 sits where the new value is above 1. +- **"Silently on sf's GDAL 3.8.5".** Supported. With `sf::gdal_utils(..., quiet = FALSE)` and + calling handlers for warnings and messages, the probe captured nothing and printed nothing. + CLI 3.13 prints the warning. +- **Log figures.** Every one matches log3.txt as the docs state it: 41 frames, 12,682 px, + median 13, max 0.26% (bcb94081_042), ten worst all bcb94081, 0 deleted, 161 of 182 and 3.5% + source-side, 0 of 264 and 0 of 10,105 declined, 182 of 264 grayscale. The table's framing + as the figure is what is wrong; see the high finding. +- **Rotated warp alpha.** Two distinct output alpha values on a rotated warp: confirmed + (0, 255). +- **Public repo.** No private sibling is named anywhere in the diff. progress.md says "the + private sibling" only. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round3.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round3.md new file mode 100644 index 0000000..9f3c86b --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-round3.md @@ -0,0 +1,73 @@ +# Docs review, round 3 (#56, staged diff after cf47430) + +## The mechanism + +Rounds 1 and 2 and the failures below share one shape: **a claim keeps the result and drops +the condition it was measured under.** The instrument fixes something (one bearing, one GDAL +build on one OS, one image geometry, one source band layout) and the prose states the +result as a property of the output contract. Round 2 dropped the bearing. This round finds +three more conditions dropped the same way: the **platform** ("on every platform", measured +on macOS 3.8.5 only, CI never run on this branch), the **pixel geometry** ("exact for an +axis-aligned warp", true only where image aspect equals footprint aspect, which the 1250 x +1250 thumbnails and `measure_output()`'s `dm/2` ring satisfy by construction), and the +**GDAL build** ("silently", when two of the three builds named in the diff print the +message). + +Probes were run in a scratch copy of the repo (`fly3`) on sf's GDAL 3.8.5; nothing in the +repo was edited apart from this file. + +## Enumeration + +| # | claim (location) | producer | scope stated vs scope supported | verdict | +|---|---|---|---|---| +| 1 | Gray 2 bands, RGB 4, "on every platform" (NEWS l.3 "Band count is now one number on every platform"; CLAUDE.md Key Decision; roxygen l.131-132; test asserts it unconditionally) | test-fly_georef_mask.R on macOS/3.8.5. `gh run list --branch 56-…` returns `[]` and the branch has no origin ref, so no ubuntu or Windows run exists | every platform vs one platform. The Windows 2-band result under the old options (#68) was never explained, so what that build does with `-srcalpha -dstalpha` is unknown | **FAIL (F1)** | +| 2 | No NoData value on any output (NEWS, CLAUDE.md "no NoData", note) | probe: Byte 1/3 band and UInt16 1 band, with and without `-srcnodata 0`, all without `NoData` | srcnodata leg included, supported on 3.8.5 | pass | +| 3 | Alpha opaque = datatype max, 255 Byte / 65535 UInt16 (NEWS, CLAUDE.md, note) | tests l.297-321 (UInt16); probe gives 65535 on UInt16 1 and 3 band | 1-3 band sources pass. A 4-band source gives alpha 0/100 (source band 4 taken as alpha), and a 2-band source gives 3 bands | pass for gray/RGB; **edge (F6)** | +| 4 | 161 of 182 axis-aligned, 242,439 px, median 47, max 3.6% (NEWS, CLAUDE.md, note table, findings) | log "warp axis-aligned" block | the 182 calibration thumbnails, 3.8.5 | pass | +| 5 | 41 of 182 at 30 deg, 12,682 px, median 13, max 0.26% (same places) | log "30 degrees" block | same | pass | +| 6 | 0 deleted on the default path (note "None was deleted on this, the default, path") | log: `true 0 deleted … frames 0` at both bearings | the two bearings, masked, no srcnodata | pass | +| 7 | "**0 pixels deleted at any bearing**" (findings.md) | same log | two bearings, not any | **FAIL, low (F5)** | +| 8 | Axis-aligned count "identical to"/"equals" the source count (findings, script header 1b) | log 242,439 = 242,439 | the 182 thumbnails | pass | +| 9 | Source-side count "exact for an axis-aligned warp"; axis-aligned "lands output cells on source pixels … every opaque 0 is lost" (note new para; script header 1 and `measure_output()` comment; CLAUDE.md "wrong for every bearingless frame") | the 182 thumbnails, all 1250 x 1250 on square footprints, and `measure_output()` sizes the ring at `dm/2`, i.e. isotropic 1 m pixels by construction | every axis-aligned warp vs isotropic-pixel warps only. Probe: 200 lone zeros, axis-aligned, `fly_georef_warp_opts(1, NULL, FALSE)`: 200 x 200 on a square footprint keeps **200/200**; 200 x 188 on the same square footprint keeps **0/200** | **FAIL (F2)** | +| 10 | Rotated loss is lower "because resampling mixes a lone 0 with its neighbours" (NEWS, note) | mechanism, consistent with log and probe | general | pass | +| 11 | Worst frames all roll bcb94081 (NEWS, note, findings) | log top-5, both bearings | calibration set | pass | +| 12 | GDAL rewrote 0 as 1 "silently" (NEWS l.3, CLAUDE.md "GDAL silently **rewrote**") | findings synthetic check: silent on sf 3.8.5; Homebrew 3.13 CLI prints it; the Windows runner printed it (#68) | unscoped vs one build of three. Roxygen, note and test comments scope it to 3.8.5 correctly | **FAIL, low (F4)** | +| 13 | Deleted instead where `srcnodata = "0"` was given (NEWS, roxygen, note, tests) | findings: `-srcnodata 0 -dstnodata 0` gives nodata on 3.8.5 | pre-0.19.0 unmasked path only, which is the only one that could carry it | pass | +| 14 | Windows gave 2 bands masked, 1 on ubuntu/macOS (NEWS, CLAUDE.md, note) | #68 CI run | as stated | pass | +| 15 | "Which count a caller got depended on the GDAL build, not on the frame" (note) | #68: three runners that differ in OS and GDAL together | build-vs-OS is inference. Harmless, and it is the natural reading | pass (inference) | +| 16 | `else if`: `-srcnodata` never beside `-srcalpha` (NEWS, CLAUDE.md, note table, roxygen) | R/fly_georef.R:549-554; tests l.79-87 | code | pass | +| 17 | "srcnodata reaches a frame only when fly_mask() declined it" (NEWS, CLAUDE.md) | code | under `mask = "border"`, which the NEWS heading names. Under `mask = "none"` it reaches every frame | pass (scoped by heading) | +| 18 | A decline with no srcnodata warns (NEWS, CLAUDE.md, note table) | R/fly_georef.R:489-496 | code | pass | +| 19 | "Declined" = `masked = FALSE`; an error fails the frame (note) | georef_one: `fly_mask_one()` is called without tryCatch | code | pass | +| 20 | Mask declined 0 of 264 and 0 of 10,105 (NEWS, CLAUDE.md, note, roxygen) | log, both populations | public thumbnails | pass | +| 21 | Fallback reaches non-Byte (non-8-bit) scans (NEWS, CLAUDE.md, note) | R/fly_mask.R:234-240 refuses non-Byte | code | pass | +| 22 | "Every thumbnail is 8-bit" (note) | JPEG thumbnails; 0 declines | population | pass | +| 23 | 121-px deletion with both options (note, roxygen) | pre-existing synthetic measurement | unchanged | pass | +| 24 | Two distinct output alpha values, 0 and 255, on a rotated warp (note) | note l.288, the existing section | Byte masked | pass | +| 25 | Warped band 2 is `Alpha` because `-dstalpha` creates it (note) | probe: `ColorInterp=Alpha` on 1-band Byte and UInt16 | pass | pass | +| 26 | COG writer loses alpha interp without `alpha="YES"`; round-trip check refuses (note) | stac_airphoto_bc#36 body, measured with its `write_cog()` | as stated | pass, but see F3 | +| 27 | VRT GCP step saves ~350 MB vs 700 MB per frame (note) | pre-existing | unchanged | not re-checked | +| 28 | Calibration script now prints the post-#56 band count (script comment) | `fly_georef_warp_opts(nb0, NULL, masked = TRUE)`; nb0 is the source count, which is the function's contract; the script runs `load_all()` | code | pass | +| 29 | Script "refuses to report if any frame errored" (CLAUDE.md Architecture) | `n_err` stop precedes CSV and report; an output-pass error aborts parLapplyLB | code: holds | pass, but see F7 | +| 30 | Public repo: no private sibling named | grep of diff: only "the private sibling" | pass | pass | + +## Findings + +- **[medium] NEWS.md:3, CLAUDE.md (Key Decision "on every platform"), R/fly_georef.R:131-132, tests/testthat/test-fly_georef_mask.R (unconditional assertion)**: "Band count is now one number on every platform" has no producer. The branch is unpushed (`gh run list --branch 56-georef-output-contract-a-real-alpha-band` returns `[]`, and there is no origin ref), so only macOS on sf's GDAL 3.8.5 has run it. This is the #68 error repeated. The old claim was measured on macOS and stated for all platforms, and the Windows runner's 2-band result under `-srcalpha -dstnodata 0` was **never explained**. So nothing establishes what that build gives under `-srcalpha -dstalpha`, and 2 or 3 bands are both possible. Fix: let the three-platform CI run before stating it, or scope it to "on macOS/GDAL 3.8.5; CI decides the rest". The note's own header ("on sf's GDAL 3.8.5") already scopes it correctly. + +- **[medium] inst/notes/border-masking.md (new para: "an axis-aligned warp lands output cells on source pixels … So the source-side count … is exact for an axis-aligned warp"), data-raw/mask_measure-interior_zeros.R header item 1 ("It is exact for an axis-aligned warp") and the `measure_output()` comment ("every opaque 0 is lost"), CLAUDE.md ("wrong for every bearingless frame")**: output cells land on source pixels only when the image's aspect matches the footprint's, so the source pixels are isotropic. The measurement satisfies that by construction on two counts. Every calibration thumbnail is 1250 x 1250 on a square film footprint, and `measure_output()` sizes the ring at `dm[1]/2, dm[2]/2`, which is 1 m pixels whatever the image. A full-resolution 9-inch scan at 9600 x 9000 on a square footprint is the note's own example of a production shape, and it does not satisfy it. Probe (scratch copy, GDAL 3.8.5, `fly_georef_warp_opts(1L, NULL, FALSE)`, axis-aligned, 200 isolated zeros): a 200 x 200 image on a square footprint keeps **200 of 200** as opaque 0, and a 200 x 188 image on the same footprint keeps **0 of 200**. So "axis-aligned" does not by itself mean "exact, every 0 lost". The condition is isotropic source pixels. The numbers (161/182 and so on) stand for their stated population. The general sentences need "on a frame whose pixels map square to the ground (every thumbnail here)", or the equivalent. + +- **[low] stac_airphoto_bc#36 body (cited from NEWS.md:3 and the note as the downstream spec)**: it still carries the claims this round refuted. It says `-dstnodata 0` "wrote genuine black inside the frame as nodata" (the output measurement shows 0 deleted and all of it rewritten as 1), gives "up to 3.5%" (a source-side figure; the output figure is 3.6%), and says "On Windows it also shifted real zeros to 1" (3.8.5 on macOS does it too, silently). The staged fix repaired fly's copies of the sentence and not this one, even though the issue is read as the spec for the republish. Edit the body. + +- **[low] NEWS.md:3 ("GDAL silently rewrote"), CLAUDE.md ("GDAL silently **rewrote** genuine black as 1")**: "silently" holds on one of the three builds the diff itself names. findings.md records that Homebrew's GDAL 3.13 CLI prints the message and the Windows runner printed it (#68). Silent on sf's 3.8.5 only. The roxygen, the note and the test comments already scope it that way. + +- **[low] planning/active/findings.md (+"**0 pixels deleted at any bearing.**")**: the producer measured two bearings, axis-aligned and 30 degrees. Say "at either bearing measured". This is the same class as round 2's finding, but on a durable-record file. + +- **[low] NEWS.md:3 ("A consumer … must now read the last band: 0 is fill, and the opaque value is the datatype's maximum")**: this holds for 1- and 3-band sources, and I verified it on Byte and UInt16. An unmasked 4-band Byte source is different: GDAL takes the source's band 4 as alpha and passes its values through. The probe gave alpha 0/100 with no appended band. A 2-band source comes out as 3 bands. The note correctly scopes its sentence to "a Byte grayscale or RGB source". Whether any catalogue scan is 4-band is unknown, so this is low, but the NEWS reading instruction is stated for every output. + +- **[low] data-raw/mask_measure-interior_zeros.R, code**: the check ordering is correct in effect but runs in the wrong order, and it can lose the expensive pass. + - (a) `tryCatch(finally = stopCluster)` wraps only the output pass. `clusterEvalQ(load_all)` and the source-pass `parLapplyLB` sit outside it. The source pass's per-frame `tryCatch` does not catch a cluster-level error such as a `load_all` failure or a worker dying, and on that path the PSOCK workers are never stopped (CLAUDE.md: PSOCK workers outlive their master). Open the `tryCatch` immediately after `makePSOCKcluster()`. + - (b) `n_err` is checked **after** the output pass. A doomed run (for example, every frame "error" from the missing-closure case) still spends 2 x 182 warps before it refuses, and frames that errored at source are silently dropped from `gray_calib` because `bands` is NA. Check `n_err` first. + - (c) `measure_output()` has no per-frame `tryCatch`, so one bad frame aborts `parLapplyLB` before `write.csv`. That discards the whole 10,105-frame source pass it would have written. + + The report loop reads the right columns: `lost_out + shifted_out` over `frame_out`, the identity check against `zero_new`, and NA rows from declined frames are handled by `na.rm`. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-summary.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-summary.md new file mode 100644 index 0000000..6153e34 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-docs-summary.md @@ -0,0 +1,17 @@ +# /code-check summary — Phase 5 (docs + measurement) of #56 + +| Round | Findings | Fixed | Accepted | Inside previous fix? | +|-------|----------|-------|----------|----------------------| +| 1 | 2 (per-frame range false; source-side count quoted as output loss) | 2 | 0 | — | +| 2 | 3 (output-side fix measured one bearing, 30°, and quoted it as the loss — axis-aligned loses every opaque 0; cluster not stopped on error; "printed only on Windows" is a GDAL-build effect) | 3 | 0 | **y** | +| 3 | 7 (mechanism: results stated without the condition they were measured under — platform, pixel isotropy, GDAL build, bearings measured, source band count; plus downstream issue body and script error handling) | 7 | 0 | n | + +Ended by enumeration: round 3 tabled 30 quantified claims in the diff against their +producers (`review-docs-round3.md`); every failing row is fixed. The measurement it rests +on was re-run after the script restructure and reproduced the report byte for byte +(`data-raw/.cache/mask_interior_zeros_log.txt`). + +What changed in substance: the headline moved twice. Source-side 161/182 → output-side at +30° 41/182 → both bearings, where axis-aligned is 161/182 (rewritten as 1, never deleted) +and 30° is 41/182. The mechanism changed from "deleted" to "rewritten as 1", and the 0→1 +shift of #68 turned out not to be Windows-specific. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-plan.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-plan.md new file mode 100644 index 0000000..ee1b4d3 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-plan.md @@ -0,0 +1,24 @@ +# Plan review — #56 (Plan agent, 2026-09-28) + +Delivered as reply text (Plan agents cannot write files); recorded here with disposition. + +| id | finding | disposition | +|---|---|---| +| B1 | `opaque()` assumes alpha 255; UInt16 output alpha is 65535 | Confirmed independently before the review landed; `opaque(full =)` | +| B2 | does `-srcnodata` without `-dstnodata` leave `NoData=` on output? | Checked in Phase 3; asserted in tests | +| G1 | stale roxygen: `@param mask` "exactly", warp_opts bullets, code comment | Fixed in Phase 3 | +| G2 | `data-raw/mask_calibrate-border_threshold.R` replicates old warp; `georef_calibrate-corner_mapping.R` passes srcnodata | Phase 5: label/update | +| G3 | note §236-241 and §272-298 also stale | Phase 5 | +| G4 | declined mask is silent; row 4 of #69 table ("collar warped in as data") still unreported | Warn in `georef_one()` for row 4 | +| G5 | "declined" = `masked = FALSE` returned, not an error | Stated in roxygen | +| G6 | "accepted" test does not reach GDAL | Fallback exercised end to end through `georef_one()`; wording kept honest | +| A1 | alpha "partial coverage" contradicted by note's fringe measurement (2 alpha values) | Claim kept out of NEWS/roxygen | +| A2 | srcnodata fallback is a weak last resort (exact zeros only) | Stated in roxygen and note | +| A3 | invariant is "non-alpha bands + 1" for Byte gray/RGB | Stated that narrowly | +| A4 | #68 fix is reasoning until three-platform CI runs | CI is the acceptance | +| A5 | v0.19.0 hard-coded | Breaking, pre-1.0 → minor; 0.19.0 is what the release will be | +| O2 | NoData check before Phase 4 | Done in Phase 3 | +| S1 | downstream needs full grayscale republish, not only a fixture update | In the downstream issue | +| S2 | check mask flags, colorinterp, overviews of COG, not just check_same_raster | Phase 4 | +| AC1 | assert core is non-NA and exactly 0 (not 1: the Windows symptom) | Added | +| AC4 | locate block by coordinate not array index | Added | diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-round1.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-round1.md new file mode 100644 index 0000000..e053b88 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-round1.md @@ -0,0 +1,32 @@ +# Review round 1 — fly#56 staged diff + +## Clean + +No issues found. + +Checked, and why each held: + +- `fly_georef_warp_opts()`: `-srcalpha` / `-srcnodata` is a strict else-if, so the pair the + checklist's "GDAL applies -srcnodata and an alpha mask together" rule warns about cannot + reach GDAL. `n_bands` is the ORIGINAL source's band count and is only consulted on the + unmasked (`-srcnodata`) arm, so the extra alpha band on the masked copy never inflates the + srcnodata vector. +- gdalwarp propagating an explicit `-srcnodata` to the output as dstnodata (which would bring + the value collision back on the mask = "none" / fallback path): probed in a scratch dir on + this machine's GDAL — `-srcnodata 0 -dstalpha` on a 1-band Byte source writes 2 bands and + NO NoData Value; mask flags are PER_DATASET ALPHA. The new test asserting no "NoData Value" + on both legs covers this. +- Decline warning in `georef_one()`: `res$reason` exists on the `fly_mask_row()` tibble + (no partial-match or missing-column risk). Only fires when srcnodata is NULL; the non-Byte + decline path in `fly_mask_one()` is silent, so this is the only warning there and the + `expect_warning(..., "declined .*band type.*srcnodata")` regexp matches it. +- Tests discriminate: the interior-zero test passes `srcnodata = "0"` on a frame the mask + accepts, so leaking `-srcnodata` would turn the block transparent (alpha != 255); reverting + to `-dstnodata 0` makes the block NA. The fallback test's 4400 +- 5% (relative, +-220) fails + on the no-op mutation (difference 0). UInt16 fixture written with NAflag = 65535 so the file's + own nodata does not mask the collar in both legs (comment explains, and is correct). +- `test-fly_georef_digital.R` `mask = "none"` changes: the aspect guard runs before masking, + so the tests still exercise what they claim; `expect_no_warning` would otherwise catch the + new decline warning on UInt32 fixtures. +- No other in-package consumer (R/, vignettes) reads georef outputs' band count or nodata. +- `data-raw/mask_measure-interior_zeros.R`: statement split only. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-round2.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-round2.md new file mode 100644 index 0000000..7ee18d4 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-round2.md @@ -0,0 +1,41 @@ +# Code-check round 2 — fly#56 staged diff + +## Findings + +- **[severity: fragile]** R/fly_georef.R:161 (and man/fly_georef.Rd:190) — "On the 264 thumbnails + `mask_threshold` was calibrated on, and on 10,105 more, the mask declined none." The 10,105 is + the **whole directory, which contains the 264**. Verified from the producer's own output, + `data-raw/.cache/mask_interior_zeros.csv`: 10,105 rows, `sum(calibration) == 264`, no duplicate + basenames. `findings.md` has it right ("Whole directory today (10,105 ...)"), and the script's + `report(res, "directory")` covers all rows. So "10,105 more" overstates the evidence by 264 + frames. Should read "and on all 10,105 in the directory" or "and on 9,841 more". This is an + exported help page, and a published number that was copied rather than derived (karpathy §7). + +## Checked and clean + +- Decline paths vs. docs: `fly_mask_one()` returns `masked = FALSE` for unreadable dimensions, + non-Byte or unreadable band type, no nearblack output, an unmeasurable interior fraction, the + interior cap, and a failed write. The doc lists three of those as examples ("an unreadable or + non-8-bit source, or a mask that floods into the interior"), which is not wrong. "An error + inside it fails the frame instead" holds: the `georef_one()` call in `fly_georef()` sits inside + a `tryCatch(error =)` and returns FALSE. The claim that "a non-Byte source returns quietly" is + true. The warning never interpolates an NA reason in practice: every `masked = FALSE` row + carries one. +- `fly_georef()` wraps `georef_one()` in `tryCatch(error = )` only, with no `warning =` handler, + so the new warning cannot throw away the frame's success value. +- `fly_georef_warp_opts()`: `-srcalpha` and `-srcnodata` stay either/or, and `-dstalpha` is + added unconditionally. This matches the roxygen. +- Measurement script: the calibration join by basename matches all 264 and has no collisions. + All sources are JPEG, which terra and nearblack both read through GDAL, so the two agree. The + masked copy is GTiff with 0/255 alpha, so `alpha == 255` is right. RGB rows carry NA zero + counts, and `report()` only reads `bands == 1`. The only diff is splitting one line. +- Tests: `terra::extract(SpatRaster, matrix)` returns no ID column (checked: names are + `lyr.1 lyr.2`), so `v[[1]]`/`v[[2]]` index gray and alpha correctly. The + `expect_warning` regex `"declined .*band type.*srcnodata"` matches the actual message. The + relative `tolerance = 0.05` on 4400 accepts 4180–4620, and a vacuous 0 cannot pass. The + "accepted" test would go red under the old top-level refusal. +- Ran `test-fly_georef_mask.R` (90 pass) and `test-fly_georef_digital.R` (53 pass) with + `NOT_CRAN=true` in a copy of the repo. There were no failures and no skips. +- The "on every platform" band-count claim is backed only by macOS/ubuntu so far. The e2e test + asserts it with no condition, so a Windows disagreement will turn CI red rather than pass + silently. That makes it a claim waiting on CI, not a defect. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-round3.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-round3.md new file mode 100644 index 0000000..aec1e8e --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-round3.md @@ -0,0 +1,79 @@ +# Review round 3 — fly#56 staged diff + +Scope: `git diff --cached` (R/fly_georef.R, man/fly_georef.Rd, tests/testthat/test-fly_georef_mask.R, +tests/testthat/test-fly_georef_digital.R, data-raw/mask_measure-interior_zeros.R). Worked in a +`git archive` copy of the staged tree under the session scratchpad; the repo was not touched. + +## Mechanism behind the round-2 defect + +A quantified claim was **restated from a derived document** (findings.md) instead of being +re-derived from its producer, so how two populations relate (264 is a subset of 10,105) was lost +in the copy. The same mechanism reaches any claim whose wording carries a scope ("on every +platform", "never", "on Windows"), because a scope is exactly what a summary compresses. Every +quantified claim in the diff is checked against its producer below. + +## Findings + +- **[low]** R/fly_georef.R:132 (roxygen, Nodata handling item 1), tests/testthat/test-fly_georef_mask.R + comments in the "every output carries a real alpha band" and "true black" tests — + "on Windows had GDAL shift it to 1 instead" names the **platform** where the cause is the + **GDAL version**. Measured: local macOS GDAL 3.8.5 writes the 0s as nodata (NA, 121 of 121 + in a probe) with no shift; ubuntu CI installs GDAL 3.8.4; the Windows CI log (run + 36448274403) prints `Value 0 in the source dataset has been changed to 1`, the gdalwarp + behaviour of a newer GDAL. So mac/ubuntu would shift too after a GDAL upgrade. It also + qualifies the data-raw header ("any pixel ... at exactly 0 ... was written as nodata"): true + only on the GDAL the measurement ran on. The shipped code is unaffected (it no longer uses + `-dstnodata`); the prose is a proxy for the discriminating fact. Suggest "on newer GDAL (seen + on the Windows runner)". + +- **[low]** R/fly_georef.R:534-535 (`fly_georef_warp_opts()` roxygen) — "Nothing is marked by + value, so a genuine 0 is **never** mistaken for fill." False on the `-srcnodata` leg, which + this same function emits (mask = "none", or a declined frame given `srcnodata`): there a + genuine source 0 becomes transparent, i.e. indistinguishable from fill. `fly_georef()`'s own + roxygen says so ("on the frame it reaches it also removes any true black inside the frame"), + so the two docs contradict. Scope it to the fill marker (`-dstalpha`), or to the no-srcnodata case. + +- **[low]** tests/testthat/test-fly_georef_mask.R, "a declined mask falls back to srcnodata" + comment — "`NA` is written to a UInt16 file as nodata 0". Probed (terra 1.9.50): + `NAflag = NA` writes `NoData Value=nan`, not 0. The behavioural conclusion is right — GDAL + honours that NaN as 0 on an integer band, so both legs masked the collar (10000/10000 opaque) + where `NAflag = 65535` separates them (10000 vs 14400) — but the stated mechanism is wrong, + and the NaN-to-integer reading is exactly the kind of thing a GDAL upgrade changes. Note the + 8-bit `collar_frame()` fixture carries the same `nan` nodata; its assertions passed here. + +- **[low]** tests/testthat/test-fly_georef_digital.R:~225 — "the fixture is UInt32, which the + border mask declines with a warning **of its own**". `fly_mask_one()`'s non-Byte path returns + without warning (R/fly_mask.R:234-243, and the new georef_one comment says so); the warning is + `georef_one()`'s new one. Misattribution in a comment only. + +- **[low]** R/fly_georef.R:146-147 (roxygen) — "[fly_mask()] declines some frames — an + unreadable or non-8-bit source, or a mask that floods into the interior" reads as the full + list; `fly_mask_one()` also declines on "nearblack wrote no output", "interior mask fraction + could not be measured" and "could not write the masked copy" (fly#69 counts four documented + paths). Doc accuracy only; the code handles every decline identically. + +## Claims checked and found correct (producer in brackets) + +- "all 10,105 public thumbnails ... (the 264 ... among them), the mask declined none" + [data-raw/.cache/mask_interior_zeros.csv: 10,105 rows, masked TRUE on all, calibration TRUE on + 264, 0 error reasons]. +- "Before v0.19.0 this pair was refused ...; they never did" [git: the `else if` in + `fly_georef_warp_opts()` exists since a767204, the same commit that added the refusal; + DESCRIPTION is 0.18.0]. +- "120^2 - 100^2 = 4400 source pixels" [arithmetic; probe gives 14400 vs 10000 opaque]. +- "65535 on a UInt16 output" [probe: alpha max 65535]. +- "+-20 m of the ring's centre is inside" the 11x11 block [block spans pixels 55-65, i.e. + -60..+50 m about centre; ±20 m plus the bilinear reach of 10 m stays inside]. +- "an error inside it fails the frame" [georef_one calls fly_mask_one with no tryCatch; the + error reaches fly_georef's per-frame tryCatch]. +- "The aspect guard under test runs before masking" [georef_one: aniso check precedes Step 1]. +- fly#56, fly#68, fly#69 resolve to on-topic issues. +- man/fly_georef.Rd regenerates identically from the roxygen. +- test-fly_georef_mask.R and test-fly_georef_digital.R: all blocks pass (NOT_CRAN=true), 0 + failures, 0 warnings; test-fly_georef.R and test-fly_georef_aspect.R pass. + +Accepted tradeoffs not re-raised: "on every platform" for the band count (CI-asserted, not yet +observed on Windows), unused `is_rgb`, thumbnail-only decline count, `mask = "none"` in the +digital tests. + +No correctness, security or data-loss defect found in the code change itself. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/review-summary.md b/planning/archive/2026-09-issue-56-georef-output-contract/review-summary.md new file mode 100644 index 0000000..267ddf6 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/review-summary.md @@ -0,0 +1,11 @@ +# /code-check summary — Phases 2-3 of #56 + +| Round | Findings | Fixed | Accepted | Inside previous fix? | +|-------|----------|-------|----------|----------------------| +| 1 | 0 | 0 | 0 | — | +| 2 | 1 (docs count: 10,105 already contains the 264) | 1 | 0 | n | +| 3 | 5 (prose claims vs producers: "on Windows" is a GDAL-build effect; "never mistaken for fill" false on the srcnodata leg; terra NA → `nan` not 0; warning attributed to the mask; decline list read as complete) | 5 | 0 | n | + +Ended by enumeration: round 3 named the mechanism (claims copied from a summary rather than +re-derived) and swept every quantified claim in the diff's roxygen, warnings, test comments +and data-raw header against its producer; the five it found were fixed, the rest confirmed. diff --git a/planning/archive/2026-09-issue-56-georef-output-contract/task_plan.md b/planning/archive/2026-09-issue-56-georef-output-contract/task_plan.md new file mode 100644 index 0000000..ec02b61 --- /dev/null +++ b/planning/archive/2026-09-issue-56-georef-output-contract/task_plan.md @@ -0,0 +1,85 @@ +# Task: Georef output contract: a real alpha band for grayscale, and a supported mask-or-nodata fallback (#56) + + +`fly_georef()` gives a **grayscale** output `-dstnodata 0` rather than a real alpha band, +while RGB gets `-dstalpha`. That was preserved deliberately in v0.11.0 (fly#23) so the +frame-border mask could ship without changing any output's band count, but it is strictly +the weaker contract: + +- `-dstnodata 0` cannot express **partial coverage** at the mask boundary. Alpha can. +- It collides with real content: a genuine 0-valued pixel inside the frame is + indistinguishable from a masked one. This is the same class of defect the mask exists to + fix — `srcnodata = "0"` was deleting real black — reintroduced on the output side. +- It reads back as `NA` rather than as a maskable band, so a consumer counting coverage has + to special-case grayscale. + +182 of the 264 measured thumbnails are grayscale, so this is the majority of the corpus. + +Absorbs #68 (Windows band count) and #69 (mask-or-nodata fallback). + +**Decisions taken at the gate (user):** +- Grayscale **just changes** to 2 bands (gray + alpha). No opt-out argument; breaking + change in NEWS with "1 → 2 bands" first. `fly_georef()` formals unchanged. +- **Relax the refusal**: `mask = "border"` + `srcnodata` is allowed; `srcnodata` applies + only to frames whose mask declined. No new `mask` value. + +## Phase 1: Measure first +- [x] `data-raw/mask_measure-interior_zeros.R`: over the 264 thumbnails + (`stac_airphoto_bc/data/raw/thumbs`, read-only), for each grayscale frame count pixels + exactly 0 that the threshold-16 floodfill mask does **not** remove — the real content + `-dstnodata 0` loses today. Report frames affected / 182 and pixel fractions (median, + max). Producer line for each figure. +- [x] Mask decline rate: count frames where `fly_mask_one()` declines at the shipped + constants (cap from `inst/extdata/mask_border_sweep.csv` offline, plus a live run of + `fly_mask_one()` over the 264 for the other paths). State what thumbnails cannot speak + to (non-Byte full-res scans are the decline path most likely to matter). +- [x] Record both in `findings.md`. + +## Phase 2: Tests first (red) +- [x] Rewrite warp-opts table tests: `-dstalpha` for every band count, `-dstnodata` + never emitted; `-srcalpha` when masked, `-srcnodata` only when unmasked (else-if kept, + asserted for all four masked × srcnodata rows). +- [x] End-to-end: grayscale → 2 bands **unconditionally** (Windows pin removed), RGB → 4; + masked and unmasked agree; grayscale collar removal now countable via the alpha band + (same `opaque()` check RGB already has). +- [x] New: an interior block of exact 0 in a grayscale frame survives as opaque data + (the output-side collision this issue exists to fix). Restore `-dstnodata 0` and + confirm it goes red. +- [x] `fly_georef()` accepts `mask = "border"` + `srcnodata`; a frame whose mask declines + (synthetic non-Byte or cap-tripping source) gets `-srcnodata`, a masked frame does not. + Remove the test asserting the refusal. + +## Phase 3: Implement +- [x] `fly_georef_warp_opts()`: `-dstalpha` for all inputs; drop the `-dstnodata 0` arm. +- [x] Remove the `mask = "border"` + `srcnodata` refusal in `fly_georef()`. +- [x] Rewrite roxygen: `@param srcnodata` (fallback where the mask declines), **Nodata + handling** item 1 and the "mutually exclusive" paragraph, `fly_georef_warp_opts()`'s + three rules. `devtools::document()`. +- [x] Tests green; `lintr::lint_package()` clean. + +## Phase 4: Downstream check (read-only on other repos) +- [x] Georef a few bundled grayscale frames with the new build; run `stac_airphoto_bc`'s + `scripts/03_cog.py` `write_cog()` + `check_same_raster()` on them from scratch space + (its conda env), without writing into that repo. Record result. +- [x] File a `stac_airphoto_bc` issue: grey outputs become gray+alpha 2-band; its + `tests/test_cog.py` "grey frame with nodata 0" fixture describes the old shape. +- [x] File an issue in the private sibling that pins `fly_georef_warp_opts()`'s table and + calls `fly:::georef_one()`: its guard will now (correctly) fail, and the exported + `fly_georef(mask = "border", srcnodata = ...)` replaces the `:::` reach. Reference kept + one-directional (backtick `fly#56` there; nothing named from fly). + +## Phase 5: Docs and record +- [x] `inst/notes/border-masking.md`: rewrite "Nothing downstream changes band count" to + the new unconditional invariant (grayscale 2, RGB 4, all platforms); add the Phase 1 + measurements with producers; the fallback semantics. +- [x] CLAUDE.md Key Decision for #23: replace the platform-conditional band-count + paragraph (fly#68) with the new invariant and the #69 decision. +- [x] NEWS.md entry (breaking, band count named first). Version bump left to `/gh-pr-merge`. + +## Validation +- [x] Tests pass (`devtools::test()`), including the restore-the-bug checks above +- [ ] Three-platform CI green on the PR (the Windows 2-band pin is gone, so this is the + real test of #68) +- [x] `/code-check` clean on each commit +- [x] PWF checkboxes match landed work +- [x] `/planning-archive` on completion, then `/gh-pr-push` diff --git a/tests/testthat/test-fly_georef_digital.R b/tests/testthat/test-fly_georef_digital.R index 23c87bc..6078202 100644 --- a/tests/testthat/test-fly_georef_digital.R +++ b/tests/testthat/test-fly_georef_digital.R @@ -195,7 +195,9 @@ test_that("a mapping that would stretch the image is refused, not written squash # sides, so rotation 270 pairs them isotropically and the file is written. ok <- ring(hc = 1000, ha = 500) out <- tempfile(fileext = ".tif") - expect_true(georef_one(src, ok, out, rotation = fly_digital_rotation())) + # `mask = "none"`: a UInt32 fixture the border mask declines, which `georef_one()` + # warns about (fly#56). + expect_true(georef_one(src, ok, out, rotation = fly_digital_rotation(), mask = "none")) expect_true(file.exists(out)) # The same image on the same footprint at rotation 0 pairs 100 px with the 2000 m edge @@ -221,7 +223,11 @@ test_that("a mapping that would stretch the image is refused, not written squash terra::writeRaster(terra::rast(nrows = 100, ncols = 100, vals = seq_len(10000)), square_image, overwrite = TRUE) out4 <- tempfile(fileext = ".tif") - expect_no_warning(res4 <- georef_one(square_image, sq, out4, rotation = 0)) + # `mask = "none"`: the fixture is UInt32, which the border mask declines, and + # `georef_one()` warns about a decline with no `srcnodata` (fly#56). The aspect guard under test runs before masking either way. + expect_no_warning( + res4 <- georef_one(square_image, sq, out4, rotation = 0, mask = "none") + ) expect_true(res4) }) @@ -255,7 +261,8 @@ test_that("a film scan carrying the negative's rebate still georeferences", { for (rot in c(0, 90, 180, 270)) { out <- tempfile(fileext = ".tif") - expect_no_warning(res <- georef_one(src, sq, out, rotation = rot)) + # `mask = "none"` for the same reason as above: a UInt32 fixture the mask declines. + expect_no_warning(res <- georef_one(src, sq, out, rotation = rot, mask = "none")) expect_true(res) } }) diff --git a/tests/testthat/test-fly_georef_mask.R b/tests/testthat/test-fly_georef_mask.R index 7499097..45bd1cc 100644 --- a/tests/testthat/test-fly_georef_mask.R +++ b/tests/testthat/test-fly_georef_mask.R @@ -6,7 +6,8 @@ # The exact vector `fly_georef()` emitted before v0.11.0. Hardcoded rather than derived — # it is a contract this package chose, and a parity assertion computed from the code it is -# meant to pin could never fail. +# meant to pin could never fail. Since v0.19.0 (fly#56) it holds for RGB only: grayscale +# fill moved from `-dstnodata 0` to `-dstalpha`, deliberately. warp_opts_v0_10_0 <- function(n_bands, srcnodata = "0") { is_rgb <- n_bands >= 3 o <- c("-t_srs", "EPSG:3005", "-r", "bilinear") @@ -16,13 +17,17 @@ warp_opts_v0_10_0 <- function(n_bands, srcnodata = "0") { } -test_that("mask = \"none\" reproduces the pre-0.11.0 option vector exactly", { - # The backward-compatibility proof, and it is free. If this drifts, every caller who - # opted out of masking silently got a different warp. +test_that("mask = \"none\" reproduces the pre-0.11.0 option vector for RGB, and grayscale differs only in its fill", { + # The backward-compatibility proof for RGB. If this drifts, every caller who opted out + # of masking silently got a different warp. expect_identical(fly_georef_warp_opts(3L, "0", masked = FALSE), warp_opts_v0_10_0(3L)) - expect_identical(fly_georef_warp_opts(1L, "0", masked = FALSE), warp_opts_v0_10_0(1L)) expect_identical(fly_georef_warp_opts(4L, "0", masked = FALSE), warp_opts_v0_10_0(4L)) + # Grayscale differs from v0.10.0 in exactly the last arm and nowhere else. + old <- warp_opts_v0_10_0(1L) + new <- fly_georef_warp_opts(1L, "0", masked = FALSE) + expect_identical(new, c(old[seq_len(length(old) - 2L)], "-dstalpha")) + # Spelled out once, so a reader can see what the contract is without evaluating it. expect_identical( fly_georef_warp_opts(3L, "0", masked = FALSE), @@ -30,7 +35,7 @@ test_that("mask = \"none\" reproduces the pre-0.11.0 option vector exactly", { ) expect_identical( fly_georef_warp_opts(1L, "0", masked = FALSE), - c("-t_srs", "EPSG:3005", "-r", "bilinear", "-srcnodata", "0", "-dstnodata", "0") + c("-t_srs", "EPSG:3005", "-r", "bilinear", "-srcnodata", "0", "-dstalpha") ) }) @@ -41,26 +46,44 @@ test_that("a masked source is read through -srcalpha and never through -srcnodat expect_true("-srcalpha" %in% o) expect_false("-srcnodata" %in% o) } +}) + + +test_that("srcnodata is the fallback: live exactly where the mask did not run", { + # The four rows fly#69 measured, pinned. `fly_georef()` now accepts `mask = "border"` + # with `srcnodata`, and that is only safe because this is an either/or: GDAL given both + # applies both, and `-srcnodata` then deletes the interior black the mask kept. + for (nb in c(1L, 3L)) { + both <- fly_georef_warp_opts(nb, "0", masked = TRUE) + expect_true("-srcalpha" %in% both) + expect_false("-srcnodata" %in% both) + + expect_identical(fly_georef_warp_opts(nb, "0", masked = TRUE), + fly_georef_warp_opts(nb, NULL, masked = TRUE)) + + declined <- fly_georef_warp_opts(nb, "0", masked = FALSE) + expect_true("-srcnodata" %in% declined) + expect_false("-srcalpha" %in% declined) - # And the two are never emitted together even if a caller reached this helper directly. - # `fly_georef()` refuses the combination at its boundary; this must not paper over it by - # emitting both, because GDAL applies both and the mask loses. - o <- fly_georef_warp_opts(3L, "0", masked = TRUE) - expect_true("-srcalpha" %in% o) - expect_false("-srcnodata" %in% o) + neither <- fly_georef_warp_opts(nb, NULL, masked = FALSE) + expect_false(any(c("-srcnodata", "-srcalpha") %in% neither)) + } }) -test_that("warp fill is unchanged, so output band counts do not move", { - # -dstalpha for RGB, -dstnodata 0 for grayscale, masked or not. This is what keeps the - # stac_airphoto_bc COG pipeline reading the same shape it always has. - for (masked in c(TRUE, FALSE)) { - rgb <- fly_georef_warp_opts(3L, if (masked) NULL else "0", masked = masked) - gray <- fly_georef_warp_opts(1L, if (masked) NULL else "0", masked = masked) - expect_true("-dstalpha" %in% rgb) - expect_false("-dstalpha" %in% gray) - expect_true(all(c("-dstnodata", "0") %in% gray)) - expect_false("-dstnodata" %in% rgb) +test_that("every output carries a real alpha band and nothing is marked by value", { + # fly#56. `-dstnodata 0` on grayscale made a genuine 0 inside the frame indistinguishable + # from fill, so GDAL rewrote real zeros as 1 (measured on 3.8.5; printed on the Windows + # runner in fly#68), or deleted them where `-srcnodata 0` was also given. + # Alpha is the only fill marker, for every band count, masked or not. + for (nb in c(1L, 2L, 3L, 4L)) { + for (masked in c(TRUE, FALSE)) { + for (snd in list(NULL, "0")) { + o <- fly_georef_warp_opts(nb, snd, masked = masked) + expect_true("-dstalpha" %in% o) + expect_false("-dstnodata" %in% o) + } + } } }) @@ -70,33 +93,29 @@ test_that("no options are emitted when there is neither a mask nor a srcnodata", fly_georef_warp_opts(3L, NULL, masked = FALSE), c("-t_srs", "EPSG:3005", "-r", "bilinear", "-dstalpha") ) + expect_identical( + fly_georef_warp_opts(1L, NULL, masked = FALSE), + c("-t_srs", "EPSG:3005", "-r", "bilinear", "-dstalpha") + ) }) -test_that("srcnodata alongside mask = \"border\" is refused, naming both and the remedy", { +test_that("srcnodata alongside mask = \"border\" is accepted", { centroids <- sf::st_read(system.file("testdata/photo_centroids.gpkg", package = "fly"), quiet = TRUE) fetched <- dplyr::tibble(airp_id = centroids$airp_id[1], dest = NA_character_, success = FALSE) - # GDAL accepts the combination and then deletes the interior black the mask exists to - # keep — measured, exactly the 121 true-black pixels of a synthetic test frame. Silent, - # so the package raises what GDAL will not. - err <- tryCatch( - fly_georef(fetched, centroids[1, ], dest_dir = tempfile(), srcnodata = "0"), - error = function(e) conditionMessage(e) - ) - expect_match(err, "srcnodata") - expect_match(err, "mask") - expect_match(err, "mask = \"none\"", fixed = TRUE) - - # ...and it is legal with the mask off, which is the documented escape hatch. - expect_no_error( - suppressWarnings(suppressMessages( - fly_georef(fetched, centroids[1, ], dest_dir = tempfile(), - mask = "none", srcnodata = "0") - )) - ) + # Refused before v0.19.0 on the grounds that GDAL would apply both. It never did — + # `fly_georef_warp_opts()` chooses one — so the refusal only made the fallback + # unreachable from the exported API (fly#69, absorbed into fly#56). + for (m in c("border", "none")) { + expect_no_error( + suppressWarnings(suppressMessages( + fly_georef(fetched, centroids[1, ], dest_dir = tempfile(), mask = m, srcnodata = "0") + )) + ) + } }) @@ -150,40 +169,50 @@ test_that("the mask is measured on the SOURCE, before any GCP or warp step", { }) -test_that("end to end, masking removes the collar and leaves the band count alone", { - skip_if_no_terra() - - # A frame with a collar the old exact-zero path could not see: 3-12, never 0. - build <- function(path, bands) { - n <- 120L - m <- matrix(200L, n, n) - vals <- rep(seq.int(3L, 12L), length.out = n) - for (k in seq_len(10L)) { - m[k, ] <- vals - m[n - k + 1L, ] <- vals - m[, k] <- vals - m[, n - k + 1L] <- vals - } - r <- terra::rast(nrows = n, ncols = n, nlyrs = bands, vals = 0L) - for (b in seq_len(bands)) terra::values(r[[b]]) <- as.integer(t(m)) - terra::writeRaster(r, path, datatype = "INT1U", overwrite = TRUE, NAflag = NA) - path +# A frame with a collar the old exact-zero path could not see: 3-12, never 0. `hole` puts +# an 11x11 block of TRUE black in the middle, which is scene content (shadow, dark water) +# that no mask should take and no fill marker should collide with. +collar_frame <- function(path, bands, hole = FALSE, datatype = "INT1U") { + n <- 120L + m <- matrix(200L, n, n) + vals <- rep(seq.int(3L, 12L), length.out = n) + for (k in seq_len(10L)) { + m[k, ] <- vals + m[n - k + 1L, ] <- vals + m[, k] <- vals + m[, n - k + 1L] <- vals } + if (hole) m[55:65, 55:65] <- 0L + r <- terra::rast(nrows = n, ncols = n, nlyrs = bands, vals = 0L) + for (b in seq_len(bands)) terra::values(r[[b]]) <- as.integer(t(m)) + terra::writeRaster(r, path, datatype = datatype, overwrite = TRUE, NAflag = NA) + path +} - ring <- sf::st_sf(geometry = sf::st_sfc(sf::st_polygon(list(matrix( +# Axis-aligned and the same 1200 m on both sides as the 120 px frame, so one source pixel +# is exactly one 10 m output cell and the middle of the frame lands at a known place. +collar_ring <- function() { + sf::st_sf(geometry = sf::st_sfc(sf::st_polygon(list(matrix( c(-600, -600, 600, -600, 600, 600, -600, 600, -600, -600), ncol = 2, byrow = TRUE ) + rep(c(1.2e6, 9e5), each = 5))), crs = 3005)) +} - opaque <- function(path) { - a <- terra::as.array(suppressWarnings(terra::rast(path))) - sum(a[, , dim(a)[3]] == 255) - } +# Opaque pixels, read from the LAST band, which is alpha on every output since fly#56. +# `full` is the alpha band's opaque value, which GDAL sets to its datatype's maximum: 255 +# on Byte, 65535 on a UInt16 output. +opaque <- function(path, full = 255) { + a <- terra::as.array(suppressWarnings(terra::rast(path))) + sum(a[, , dim(a)[3]] == full) +} - win_grayscale_band_bug <- tolower(Sys.info()[["sysname"]]) == "windows" + +test_that("end to end, every output carries alpha: grayscale 2 bands, RGB 4, on this platform too", { + skip_if_no_terra() + ring <- collar_ring() for (bands in c(1L, 3L)) { - src <- build(tempfile(fileext = ".tif"), bands) + src <- collar_frame(tempfile(fileext = ".tif"), bands) on_file <- tempfile(fileext = ".tif") off_file <- tempfile(fileext = ".tif") @@ -192,46 +221,109 @@ test_that("end to end, masking removes the collar and leaves the band count alon georef_one(src, ring, off_file, rotation = 270, mask = "none", srcnodata = "0") )) - # The band count is what stac_airphoto_bc consumes, and it must not move. - # - # Skipped on Windows for the GRAYSCALE arm only, and tracked rather than silenced: - # the three-platform CI added in fly#52 found on its first run that masking a 1-band - # source there yields 2 bands against 1 with masking off, while ubuntu and macOS both - # give 1. So the invariant this asserts -- recorded unconditionally in CLAUDE.md and - # inst/notes/border-masking.md -- was measured on one platform. fly#68. - # - # The skip is printed by the workflow's "Report skipped tests" step on every run, so - # it stays visible rather than becoming the quiet kind. RGB is unaffected: both arms - # give 4 on all three runners, so it keeps asserting everywhere. - # Windows is PINNED, not skipped. Masking a 1-band source there yields 2 bands - # against 1 with masking off, where ubuntu and macOS both give 1 -- found by the - # three-platform CI added in fly#52 on its first run, and tracked as fly#68. So the - # invariant recorded unconditionally in CLAUDE.md and inst/notes/border-masking.md - # was measured on one platform. - # - # Asserting the observed Windows value beats skipping it twice over: nothing goes - # unasserted, and the test goes red if that platform's behaviour moves in EITHER - # direction -- including the direction where fly#68 gets fixed and this line is the - # thing that says so. `skip()` was the other option and is worse still, because it - # unwinds the whole `test_that()` block and would take the RGB iteration and the - # collar measurements below with it. - expect_equal(fly_gdal_bands(fly_gdal_info(on_file)), - if (bands >= 3L) 4L else if (win_grayscale_band_bug) 2L else 1L, + # The band count is what stac_airphoto_bc consumes. Before fly#56 grayscale was 1 band + # on ubuntu and macOS and 2 on Windows when masked (fly#68), so the old invariant was + # platform-conditional and pinned per platform here. With `-dstalpha` everywhere it is + # one number, and asserted unconditionally: red on any runner that disagrees. + want <- bands + 1L + expect_equal(fly_gdal_bands(fly_gdal_info(on_file)), want, label = paste0("bands=", bands, " masked, on ", Sys.info()[["sysname"]])) - if (bands >= 3L || !win_grayscale_band_bug) { - expect_equal(fly_gdal_bands(fly_gdal_info(on_file)), - fly_gdal_bands(fly_gdal_info(off_file))) - } - expect_equal(fly_gdal_bands(fly_gdal_info(off_file)), if (bands >= 3L) 4L else 1L) - - # RGB carries an alpha band, so the collar's removal is directly countable. Grayscale - # expresses it as nodata rather than alpha and has no alpha band to count, which is - # the weaker contract recorded in the note. - if (bands >= 3L) { - expect_lt(opaque(on_file), opaque(off_file)) - # ...and by roughly the collar's share of the frame, not by a rounding error. - expect_gt((opaque(off_file) - opaque(on_file)) / opaque(off_file), 0.1) + expect_equal(fly_gdal_bands(fly_gdal_info(off_file)), want, + label = paste0("bands=", bands, " unmasked, on ", Sys.info()[["sysname"]])) + + # Fill is marked by alpha alone. gdalwarp copies a source nodata to the output unless + # `-dstalpha` is given; a `NoData Value` here would bring the value collision back. + for (f in c(on_file, off_file)) { + expect_false(grepl("NoData Value", fly_gdal_info(f)), label = basename(f)) } + + # Both shapes now carry an alpha band, so the collar's removal is countable for + # grayscale too — which it was not while grayscale expressed fill as a value. + expect_lt(opaque(on_file), opaque(off_file)) + # ...and by roughly the collar's share of the frame, not by a rounding error. + expect_gt((opaque(off_file) - opaque(on_file)) / opaque(off_file), 0.1) } }) + +test_that("true black inside a grayscale frame is written as opaque data, not as fill", { + skip_if_no_terra() + + # The output-side collision fly#56 exists to fix. Under `-dstnodata 0` the 11x11 block + # of real zeros below came out as 1 — GDAL moving it off the nodata value, silently on + # 3.8.5 — or as nodata where `-srcnodata 0` was also given. With alpha it keeps value 0 + # and alpha 255. + src <- collar_frame(tempfile(fileext = ".tif"), 1L, hole = TRUE) + out <- tempfile(fileext = ".tif") + # `srcnodata` too: on a frame the mask accepts it must not reach GDAL, or it deletes + # exactly this block — the reason `fly_georef()` refused the pair before v0.19.0. + expect_true(suppressWarnings( + georef_one(src, collar_ring(), out, rotation = 270, srcnodata = "0") + )) + + r <- suppressWarnings(terra::rast(out)) + expect_equal(terra::nlyr(r), 2L) + # The block's middle, located by ground coordinate rather than array index, and well + # clear of the bilinear kernel's reach into the 200s around it: 11 cells of 10 m, so + # +-20 m of the ring's centre is inside it. + pts <- expand.grid(x = 1.2e6 + c(-20, 0, 20), y = 9e5 + c(-20, 0, 20)) + v <- terra::extract(r, as.matrix(pts)) + expect_true(all(v[[2]] == 255)) + # Exactly 0: not NA (read as nodata) and not 1 (GDAL's rewrite of a real 0 away from a + # nodata of 0, what the old default path actually did on 3.8.5 and on the fly#68 runner). + expect_false(anyNA(v[[1]])) + expect_true(all(v[[1]] == 0)) +}) + + +test_that("a declined mask falls back to srcnodata, and without it the collar is warped in as data", { + skip_if_no_terra() + + # fly#69's fourth row, reached through the exported semantics. A 16-bit source is + # declined by `fly_mask_one()` (the threshold is calibrated on 8-bit imagery), so the + # frame is warped unmasked. The collar here is EXACT 0, the only thing `srcnodata` + # can see. + src <- tempfile(fileext = ".tif") + n <- 120L + m <- matrix(2000L, n, n) + m[1:10, ] <- 0L + m[(n - 9):n, ] <- 0L + m[, 1:10] <- 0L + m[, (n - 9):n] <- 0L + # NAflag must be a value the frame does not hold. `NA` is written as `NoData Value=nan`, + # which GDAL reads as 0 on an integer band and honours with or without `srcnodata`, so + # both legs mask the collar and the test cannot tell them apart. + terra::writeRaster(terra::rast(nrows = n, ncols = n, vals = as.integer(t(m))), src, + datatype = "INT2U", overwrite = TRUE, NAflag = 65535) + + with_fallback <- tempfile(fileext = ".tif") + without <- tempfile(fileext = ".tif") + ok1 <- suppressWarnings( + georef_one(src, collar_ring(), with_fallback, rotation = 270, srcnodata = "0") + ) + ok2 <- suppressWarnings(georef_one(src, collar_ring(), without, rotation = 270)) + expect_true(ok1) + expect_true(ok2) + + # With the fallback the collar is transparent; without it, all of it is opaque. The + # difference is the collar: 120^2 - 100^2 = 4400 source pixels, one output cell each. + expect_equal(opaque(without, 65535) - opaque(with_fallback, 65535), 4400, tolerance = 0.05) +}) + + +test_that("a declined mask with no fallback is warned about, and a masked or fallen-back frame is not", { + skip_if_no_terra() + + # Row 4 of fly#69's table: no mask, no srcnodata, collar written as data. The non-Byte + # decline path in `fly_mask_one()` returns without warning, so `georef_one()` must say it. + src16 <- tempfile(fileext = ".tif") + terra::writeRaster(terra::rast(nrows = 60, ncols = 60, vals = 2000L), src16, + datatype = "INT2U", overwrite = TRUE, NAflag = 65535) + expect_warning(georef_one(src16, collar_ring(), tempfile(fileext = ".tif"), rotation = 270), + "declined .*band type.*srcnodata") + expect_no_warning(georef_one(src16, collar_ring(), tempfile(fileext = ".tif"), + rotation = 270, srcnodata = "0")) + + src8 <- collar_frame(tempfile(fileext = ".tif"), 1L) + expect_no_warning(georef_one(src8, collar_ring(), tempfile(fileext = ".tif"), rotation = 270)) +})