diff --git a/DESCRIPTION b/DESCRIPTION index e69b278..831cf9d 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,7 @@ Package: fly -Title: Airphoto Footprint Estimation and Coverage Selection -Version: 0.5.0 +Title: Historic Airphoto Footprints, Selection and Georeferencing for + British Columbia +Version: 0.5.1 Date: 2026-08-29 Authors@R: c( person("Allan", "Irvine", , "al@newgraphenvironment.com", role = c("aut", "cre"), @@ -8,8 +9,9 @@ Authors@R: c( person("New Graph Environment", role = "cph") ) Description: Estimate ground footprints from airphoto centroids and scale, - compute coverage of areas of interest, and select minimum photo sets - using greedy set-cover. + compute coverage of areas of interest, select minimum photo sets using + greedy set-cover, fetch scans and flight metadata from the BC Data + Catalogue, and georeference the images onto their estimated footprints. License: MIT + file LICENSE URL: https://github.com/NewGraphEnvironment/fly, https://newgraphenvironment.github.io/fly/ diff --git a/NEWS.md b/NEWS.md index 049536f..66cde2a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,15 @@ # fly (development version) +## 0.5.1 (2026-08-29) + +- Fix `fly_footprint()` silently dropping `footprint_basis`, `footprint_terrain`, `height_agl` and `dem_coverage` whenever its input carried the `tbl_df` class ([#35](https://github.com/NewGraphEnvironment/fly/issues/35)). `bcdata::collect()` returns exactly that class, so every caller querying `WHSE_IMAGERY_AND_BASE_MAPS.AIMG_PHOTO_CENTROIDS_SP` — the documented source for this package — lost the whole reporting surface 0.4.0 and 0.5.0 added, and the documented "filter on `footprint_basis`" and "filter on `dem_coverage`" workflows were unreachable from it +- Geometry and every downstream number were always correct; what was lost was the audit trail, which is what made it invisible. `sf::st_sf()` keeps only its first argument when that argument is a tibble, discarding every trailing named column — a general R trap rather than a `fly` one +- Every class the input carries is carried through, so a tibble-backed sf comes back tibble-backed. The order is not preserved: `sf::st_transform()` moves `sf` to the front, so a `bcdc_sf` input returns `sf, bcdc_sf, ...` — as it always has +- Where the input already carried a column named `footprint_basis` (or one of the other three), the old code kept the caller's value and appended a duplicate `footprint_basis.1` — so the documented `footprint_basis != "unknown_format"` filter read the caller's column while fly's own answer sat unread beside it. The computed value now wins, and there is one column. Plain `sf` callers were affected by this too, though never by the tibble bug +- `fly_footprint()` on an input matching no frames now returns `footprint_basis` and `footprint_terrain` as `character` rather than `logical`, so an empty result binds to a populated one. Assembling a per-AOI ledger across queries previously failed on the column type the first time an area returned nothing +- Tests sweep the input-class axis (plain / tibble / grouped) rather than adding cases along the one axis the bundled fixture could present. Every fixture in the package reads back as plain `sf, data.frame`, so the suite was structurally incapable of seeing this +- Widen the `DESCRIPTION` Title and Description, which described roughly half the package — they predated `fly_fetch()`, `fly_georef()` and `fly_bearing()` ([#31](https://github.com/NewGraphEnvironment/fly/issues/31)) + ## 0.5.0 (2026-08-29) - `fly_footprint()` gains a `dem` argument, sizing each frame from its height above ground instead of the reported scale ([#9](https://github.com/NewGraphEnvironment/fly/issues/9)). On the bundled Upper Bulkley AOI the reported scale understates footprint **area by a median 14%, ranging to 26%** — and always in the same direction, because the scale is referenced to an elevation above the valley floor the photos cover. This is a datum offset, not the slope effect the issue described diff --git a/R/fly_footprint.R b/R/fly_footprint.R index 32cdf93..e828878 100644 --- a/R/fly_footprint.R +++ b/R/fly_footprint.R @@ -149,7 +149,11 @@ fly_rectangles <- function(coords, half_side) { #' `height_agl` giving the metres above ground each footprint was sized from, #' and `dem_coverage` giving the fraction of each footprint the DEM actually #' covered (`0` where it covered none, `NA` only where there is no footprint). -#' Frames whose format could not be resolved get an empty geometry. +#' Frames whose format could not be resolved get an empty geometry. Every +#' class the input carries is carried through, so a tibble-backed sf — which +#' is what `bcdata::collect()` returns — comes back tibble-backed. The order +#' is not preserved: `sf::st_transform()` moves `sf` to the front, so a +#' `bcdc_sf` input returns `sf, bcdc_sf, ...`, as it always has. #' #' @details #' Ground coverage is computed as `negative_size * scale_number * 0.0254` metres @@ -359,7 +363,12 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL, } else { media <- as.character(centroids_sf$media) width_in <- unname(formats[media]) - basis <- ifelse(is.na(width_in), "unknown_format", media) + # `as.character()` is load-bearing on empty input: `ifelse(logical(0), ...)` + # returns `logical(0)`, so a query that matched no frames would report its + # basis as a logical column. Binding that to a populated result — the + # per-AOI ledger this reporting surface exists for — fails on the type + # rather than yielding an empty block of rows. + basis <- as.character(ifelse(is.na(width_in), "unknown_format", media)) } unresolved <- is.na(width_in) @@ -379,7 +388,7 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL, # Keyed on half_side, not width_in: an unparseable `scale` also leaves a frame # with no footprint, and a frame with no footprint has had no terrain # treatment to report. - terrain <- ifelse(is.na(half_side), NA_character_, "nominal_scale") + terrain <- as.character(ifelse(is.na(half_side), NA_character_, "nominal_scale")) height_agl <- rep(NA_real_, n) dem_coverage <- rep(NA_real_, n) @@ -479,14 +488,20 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL, terrain[unusable] <- "nominal_scale" } - result <- sf::st_sf( - sf::st_drop_geometry(pts_3005), - footprint_basis = basis, - footprint_terrain = terrain, - height_agl = height_agl, - dem_coverage = dem_coverage, - geometry = fly_rectangles(coords, half_side) - ) + # Assign the reporting columns onto the attribute frame rather than passing + # them to `st_sf()` as trailing arguments. `st_sf()` builds its attribute + # frame with `else if (inherits(x[[1]], c("tbl_df", "tbl"))) x[[1]]`, so a + # tibble first argument means every trailing named column is silently + # discarded — and `bcdata::collect()` returns a tibble, which is to say the + # package's own documented data source (#35). Inside `x[[1]]` they survive, + # and the caller keeps the class they passed in. + attrs <- sf::st_drop_geometry(pts_3005) + attrs$footprint_basis <- basis + attrs$footprint_terrain <- terrain + attrs$height_agl <- height_agl + attrs$dem_coverage <- dem_coverage + + result <- sf::st_sf(attrs, geometry = fly_rectangles(coords, half_side)) sf::st_transform(result, input_crs) } diff --git a/man/fly-package.Rd b/man/fly-package.Rd index 6ffc9ce..7922534 100644 --- a/man/fly-package.Rd +++ b/man/fly-package.Rd @@ -4,11 +4,11 @@ \name{fly-package} \alias{fly} \alias{fly-package} -\title{fly: Airphoto Footprint Estimation and Coverage Selection} +\title{fly: Historic Airphoto Footprints, Selection and Georeferencing for British Columbia} \description{ \if{html}{\figure{logo.png}{options: style='float: right' alt='logo' width='120'}} -Estimate ground footprints from airphoto centroids and scale, compute coverage of areas of interest, and select minimum photo sets using greedy set-cover. +Estimate ground footprints from airphoto centroids and scale, compute coverage of areas of interest, select minimum photo sets using greedy set-cover, fetch scans and flight metadata from the BC Data Catalogue, and georeference the images onto their estimated footprints. } \seealso{ Useful links: diff --git a/man/fly_footprint.Rd b/man/fly_footprint.Rd index 1cc4574..9d236b5 100644 --- a/man/fly_footprint.Rd +++ b/man/fly_footprint.Rd @@ -32,7 +32,11 @@ rectangles, a \code{footprint_basis} column recording how each was sized, a \code{height_agl} giving the metres above ground each footprint was sized from, and \code{dem_coverage} giving the fraction of each footprint the DEM actually covered (\code{0} where it covered none, \code{NA} only where there is no footprint). -Frames whose format could not be resolved get an empty geometry. +Frames whose format could not be resolved get an empty geometry. Every +class the input carries is carried through, so a tibble-backed sf — which +is what \code{bcdata::collect()} returns — comes back tibble-backed. The order +is not preserved: \code{sf::st_transform()} moves \code{sf} to the front, so a +\code{bcdc_sf} input returns \verb{sf, bcdc_sf, ...}, as it always has. } \description{ Creates rectangular polygons representing the estimated ground coverage diff --git a/planning/archive/2026-08-issue-35-footprint-tibble-columns/README.md b/planning/archive/2026-08-issue-35-footprint-tibble-columns/README.md new file mode 100644 index 0000000..506b918 --- /dev/null +++ b/planning/archive/2026-08-issue-35-footprint-tibble-columns/README.md @@ -0,0 +1,28 @@ +# #35 — fly_footprint() dropped its reporting columns on tibble input + +**Outcome:** fixed and released as v0.5.1 in [PR #36](https://github.com/NewGraphEnvironment/fly/pull/36), which also closed #31. + +`sf::st_sf()` keeps only its first argument when that argument is a tibble, so the four +reporting columns `fly_footprint()` attaches as trailing arguments were discarded for +every `bcdata::collect()` caller — which is to say for the package's own documented data +source. Nothing errored: geometry and every downstream number stayed correct and only the +audit trail went missing, which is how it survived two releases. The fix assigns the +columns onto the attribute frame before `st_sf()` sees it. + +The durable lesson is the fixture one, and it is the seventh instance in this package +(see `inst/notes/terrain-correction.md` for the six from #9): **every fixture here reads +back as plain `sf, data.frame`, so no case added along the existing axis could have found +this.** The tests now sweep the input-class axis instead. Two further defects fell out of +review — a colliding `footprint_basis` column that was never overwritten, so the +documented filter read the caller's column instead of fly's answer; and `logical` columns +on zero-row input, which broke binding an empty result to a populated one. + +Two claims of mine were falsified by review and corrected before merge: that the input +class is *preserved* (the set survives, the order does not — `st_transform()` moves `sf` +to the front), and that the overwrite change made `fly_footprint()` idempotent (it does +not; a plain 20-row sf returns 100 rows silently). + +**Closing commits:** `a566074` (sweep), `ae628bc` (fix), `28c4f85` (#31 retitle), `3d902fa` (release). +**Verified:** suite 225 → 338, `R CMD check` Status OK, and end to end against the live +catalogue — 1405 real centroids, all four columns present, 151 frames excluded by the +documented filter that were invisible before. diff --git a/planning/archive/2026-08-issue-35-footprint-tibble-columns/findings.md b/planning/archive/2026-08-issue-35-footprint-tibble-columns/findings.md new file mode 100644 index 0000000..ed33f44 --- /dev/null +++ b/planning/archive/2026-08-issue-35-footprint-tibble-columns/findings.md @@ -0,0 +1,209 @@ +# Findings — fly_footprint() drops footprint_basis and friends on tibble input (#35) + +## Reproduced on `main` at `8585fd5`, sf 1.1.2 + +Against `inst/testdata/photo_centroids.gpkg` with the class overridden: + +``` +sf,data.frame -> footprint_basis footprint_terrain height_agl dem_coverage +sf,tbl_df,tbl,data.frame -> (all four absent) +``` + +## Mechanism — an `sf::st_sf()` branch, not a `fly` quirk + +`st_sf()` builds its attribute frame with a chain that keeps only the **first** +argument when that argument is a tibble: + +```r +df = if (inherits(x, c("tbl_df", "tbl"))) x + else if (length(x) == 1) data.frame(row.names = row.names) + else if (!sfc_last && inherits(x, "data.frame")) x + else if (sfc_last && inherits(x, "data.frame")) x[-all_sfc_columns] + else if (inherits(x[[1]], c("tbl_df", "tbl"))) x[[1]] # <-- here + else cbind(data.frame(row.names = row.names), as.data.frame(x[-all_sfc_columns], ...)) +``` + +With six arguments, `x` is a plain list of length 6, so the first four branches +are not taken. `x[[1]]` is the dropped-geometry tibble, so `df` becomes that +tibble alone — `footprint_basis`, `footprint_terrain`, `height_agl` and +`dem_coverage` are discarded before the sfc column is reattached. A plain +data.frame first argument falls through to the final `cbind()`, which keeps +everything. That is the whole difference. + +Confirmed in isolation: + +``` +st_sf(tbl, extra = "hello", geometry = g) -> "extra" present: FALSE +st_sf(df, extra = "hello", geometry = g) -> "extra" present: TRUE +``` + +## Scope — one call site + +`grep -rn "st_sf(" R/` returns two hits. The claim is specifically about columns +attached **through `st_sf()`** — columns are attached to user-supplied data in +four other places (`fly_coverage.R:34`, `fly_select.R:118,207-208`, +`fly_bearing.R:66`, `fly_georef.R:133`), all via `$<-`, which is the very +mechanism this fix adopts and none of which are affected. `R/fly_footprint.R:64` builds a bare +geometry frame (`st_sf(geometry = rects[ok])`) and attaches nothing to +user-supplied data. `R/fly_footprint.R:482` is the defect. Every other frame in +the package is a `dplyr::tibble()` built from scratch — `fly_fetch.R:84,93,104`, +`fly_overlap.R:38,71,82`, `fly_georef.R:142`, `fly_coverage.R:66` — so none +inherit the input's class and none are affected. + +Nothing downstream reads the four columns, so this is pure loss of reporting +rather than breakage: geometry and every downstream number are correct with a +`tbl_df` input today. That is what makes it dangerous rather than obvious. + +## Why the suite cannot see it + +Every fixture in the package is `sf, data.frame`: + +- `inst/testdata/photo_centroids.gpkg` reads back plain via `sf::st_read()` +- `mixed_media_fixture()` and `terrain_fixture()` in `tests/testthat/setup.R` + are built by `sf::st_sf()` on plain vectors + +No number of added cases along the existing axis would find this. Seventh +instance of the pattern in `inst/notes/terrain-correction.md` and the `CLAUDE.md` +gotcha — *ask which shapes the fixture can never present*. + +Two adjacent axes were checked and are **already covered**, so class shape is the +single gap: + +- **geometry column name** — the bundled fixture's geometry column is named + `geom`, not `geometry`, so the `st_drop_geometry()` path is exercised +- **row subsets** — existing tests pass `centroids[centroids$scale == ..., ]` + +## Fix shapes measured + +| shape | four columns | output class for a `tbl_df` input | +|---|---|---| +| `st_sf(as.data.frame(drop_geom(x)), basis =, ..., geometry =)` | present | `sf, data.frame` — **downgraded** | +| assign onto `attrs`, then `st_sf(attrs, geometry =)` | present | `sf, tbl_df, tbl, data.frame` — preserved | + +The second was verified across three input shapes, in each case with `geometry` +last and `attr(, "sf_column")` correct: + +``` +sf,data.frame -> cols ok, class preserved +sf,tbl_df,tbl,data.frame -> cols ok, class preserved +sf,grouped_df,tbl_df,tbl,data.frame-> cols ok, class preserved +``` + +It works because a two-element `x` still reaches the `inherits(x[[1]], "tbl_df")` +branch — but by then the four columns are *inside* `x[[1]]`, so surviving as +`x[[1]]` is exactly what is wanted. Chosen over the issue's suggested +`as.data.frame()` because it is the smaller delta: a tibble caller's class is +unchanged from today, only the missing columns appear. + +## `sf::st_read(path, as_tibble = TRUE)` gives an honest tibble fixture + +No class hacking is needed to build the regression fixture: + +``` +sf::st_read(p, quiet = TRUE) -> sf,data.frame +sf::st_read(p, quiet = TRUE, as_tibble = TRUE) -> sf,tbl_df,tbl,data.frame +dplyr::group_by(tbl, scale) -> sf,grouped_df,tbl_df,tbl,data.frame +``` + +Per the negative-fixture rule in `CLAUDE.md`, the helper asserts that premise +inline, so a future `sf` change fails on the premise rather than on the +behaviour under test. + +## Column vectors are already full length + +`basis`, `terrain`, `height_agl` and `dem_coverage` are each length `nrow()` +(`rep()` / `ifelse()` over length-`n` inputs, `R/fly_footprint.R:353-382`), so +assigning them with `$<-` introduces no recycling that `st_sf()` was not already +doing. + +## Baseline + +`devtools::test()` on `8585fd5`: **FAIL 0 | WARN 0 | SKIP 0 | PASS 225**. + +CI on `main` at session start: all recent runs green (pkgdown + pages). + +## Errors Encountered + +| Error | Resolution | +|-------|------------| + +## A/B against `8585fd5` on the edge cases the fix could plausibly move + +Prior implementation extracted with `git show 8585fd5:R/fly_footprint.R` and run +side by side with the new one (not reconstructed from memory): + +| input | names | nrow | row.names | geometry | `sf_column` | +|---|---|---|---|---|---| +| full 20-row plain sf | identical | 20/20 | identical | identical | `geometry`, last | +| zero-row | identical | 0/0 | identical | identical | `geometry`, last | +| non-sequential subset `[c(5,3,11), ]` | identical | 3/3 | identical | identical | `geometry`, last | +| single row | identical | 1/1 | identical | identical | `geometry`, last | + +So `$<-` recycling, the `row.names` argument the old `st_sf()` path used, and +column ordering are all non-issues. + +**One deliberate behaviour change, and it is wider than first measured.** When +the input already carries a column named `footprint_basis`, the old path kept the +caller's value and discarded the computed one. Corrected measurement — the first +probe counted exact name matches and so missed the duplicate on the plain path: + +| input | old | new | +|---|---|---| +| plain sf | 22 cols: `footprint_basis`, `footprint_basis.1`; `$footprint_basis` = `"PRE"` | 21 cols: `footprint_basis` = `"Film - BW"` | +| tibble sf | 18 cols: `footprint_basis` = `"PRE"` (four columns dropped) | 21 cols: `footprint_basis` = `"Film - BW"` | + +So a **plain-sf** caller with a colliding column — a shape #35 never broke — sees +a column-count and value change too. The new direction is the correct one: +`fly_footprint()` must report how *it* sized each frame, not echo back a +same-named column it was handed, and `fly_footprint(fly_footprint(x))` is now +idempotent rather than sticky. But it is a second fix landing alongside the +first, so it gets its own test and its own NEWS line rather than riding in +silently. + +## Restore-the-bug verification + +Prior function loaded from `8585fd5` into both `asNamespace("fly")` and +`as.environment("package:fly")` — patching only the namespace gives a false green +for anything test code calls directly. Proof-of-patch printed before asserting: + +``` +PATCH PROOF - footprint_basis on tbl input: NULL (broken code running) +``` + +The sweep then failed on the `tbl` and `grouped` shapes, naming the four missing +columns. The guard is real. + +## Suite and lint after the fix + +- `devtools::test()`: **FAIL 0 | WARN 0 | SKIP 0 | PASS 275** (baseline 225) +- `lintr` on `R/fly_footprint.R`: 0 at `8585fd5`, 0 on branch. Package total 16, + all pre-existing (`data-raw/`, `fly_fetch`, `fly_georef`, `test-fly_fetch`, + `test-fly_footprint.R:391`, vignette) +- `devtools::document()` wrote only `fly_footprint.Rd`; `NAMESPACE` unchanged at + 9 exports, so no roxygen block rebound + +## End-to-end against the live BC Data Catalogue + +The case the fixture structurally cannot reach. `bcdc_query_geodata()` on +`WHSE_IMAGERY_AND_BASE_MAPS.AIMG_PHOTO_CENTROIDS_SP`, bbox-filtered to the +bundled AOI, then `collect()`: + +``` +rows: 1405 +input class : bcdc_sf,sf,tbl_df,tbl,data.frame +output class: sf,bcdc_sf,tbl_df,tbl,data.frame +four columns present: TRUE +basis values: Film - BW | Film - Colour | unknown_format +documented filter: 1254 of 1405 +``` + +Three things this establishes that no fixture could: + +- the real input class is exactly the shape #35 named, and the four columns + arrive on it +- the class **reordering** is real rather than an artefact of the hand-set + `bcdc_sf` in `centroid_shapes()` — `sf` leads on the way out, and it is + `st_transform()` that does it +- the documented `footprint_basis != "unknown_format"` filter actually excludes + something on live data — 151 of 1405 frames — which is the digital-frame + exclusion #30 built and #35 had made unreachable from this source diff --git a/planning/archive/2026-08-issue-35-footprint-tibble-columns/progress.md b/planning/archive/2026-08-issue-35-footprint-tibble-columns/progress.md new file mode 100644 index 0000000..0e10df9 --- /dev/null +++ b/planning/archive/2026-08-issue-35-footprint-tibble-columns/progress.md @@ -0,0 +1,33 @@ +# Progress — fly_footprint() drops footprint_basis and friends on tibble input (#35) + +## Session 2026-08-29 + +- Plan-mode exploration — reproduced the defect, located the mechanism in + `sf::st_sf()`, confirmed one call site, established the 225-test baseline +- User approved: preserve-input-class fix, class-shape sweep, v0.5.1 with #31 + folded in +- Created branch `35-fly-footprint-drops-footprint-basis-and` off main +- Scaffolded PWF baseline from issue #35 with approved phases +- Next: Phase 1 — regression tests that fail on unmodified source + +### Phase 1 — regression tests (complete) + +- `centroid_shapes()` added to `tests/testthat/setup.R`: plain / `as_tibble = TRUE` + / `group_by()` shapes of the bundled fixture, premise asserted inline +- Four tests added to `test-fly_footprint.R` — nominal sweep, `dem` sweep, + class contract, downstream pass-through +- **Confirmed red on unmodified source**: the `tbl` and `grouped` shapes return + `NULL` for all four columns where the `plain` shape returns the vectors +- The class-contract test passes on the broken code by design and is commented + as such — it guards against the coercing fix, not against #35 + +### Phases 2-3 — fix, contract, review findings (complete) + +- Fix applied at `R/fly_footprint.R`; suite FAIL 0, PASS 338 (baseline 225) +- Restore-the-bug confirmed for all three new guards, each with a proof-of-patch + line printed before asserting, patching both bindings, prior code pulled from + `8585fd5` rather than reconstructed +- Plan review (12 findings) and code-check round 1 (5 findings) both landed; + every load-bearing claim reproduced before acting. Two corrected a claim I had + written: the class-preservation over-claim, and a false idempotence line in NEWS +- `R CMD check` Status OK; NAMESPACE byte-identical at 9 exports; lint unchanged diff --git a/planning/archive/2026-08-issue-35-footprint-tibble-columns/review-round1.md b/planning/archive/2026-08-issue-35-footprint-tibble-columns/review-round1.md new file mode 100644 index 0000000..9966fc5 --- /dev/null +++ b/planning/archive/2026-08-issue-35-footprint-tibble-columns/review-round1.md @@ -0,0 +1,220 @@ +# Review round 1 — #35 `st_sf()` drops trailing columns on tibble input + +Branch `35-fly-footprint-drops-footprint-basis-and`. Reviewed `git diff main...HEAD` +(tests) + `git diff --cached` (fix + Rd), and the full current text of +`R/fly_footprint.R`, `tests/testthat/setup.R`, `tests/testthat/test-fly_footprint.R`. + +Everything below was **measured by running R**, not reasoned about. Scripts are in +`/tmp/flyrev/`. The prior implementation was pulled with +`git show 8585fd5:R/fly_footprint.R` and sourced into an env parented on +`asNamespace("fly")` — not reconstructed from memory. + +## Verdict + +The fix itself is correct and, on every input I could construct, **byte-identical to +`8585fd5` for plain `data.frame` callers** while adding the four columns for tibble +callers. The five findings below are all in the surrounding material — one false +claim in the in-flight `NEWS.md`, two guard/fixture gaps, one pre-existing type +trap the fix newly exposes, and one process note. + +## Findings + +- **[fragile]** `NEWS.md:10` (unstaged, written by the concurrent session) — the claim + that the overwrite change *"makes `fly_footprint(fly_footprint(x))` idempotent"* is + **false**, and false in the dangerous direction. Measured + (`/tmp/flyrev/10_idem.R`, `11_idem_tbl.R`): + + | input | `fly_footprint(fly_footprint(x))` | + |---|---| + | plain `sf` (20 rows) | **100 rows, silently** | + | tibble-backed `sf` (20 rows) | error, `Can't recycle input of size 100 to size 20` | + + Cause is pre-existing and identical before and after the fix: + `sf::st_coordinates()` on POLYGON input returns 5 rows per feature, so + `fly_rectangles()` builds `5n` geometries; in the `cbind(data.frame(row.names = ...))` + branch `row.names` is `seq_along(sfc)` = `1:100`, which recycles the 20-row attribute + frame up to 100. Nothing warns. The overwrite change is a genuine improvement and is + worth the NEWS line — but the idempotence sentence should be cut or replaced, because + a reader will take it as licence to re-run the function on its own output. + +- **[fragile]** `R/fly_footprint.R:493-496` / `tests/testthat/test-fly_footprint.R` — + the behaviour change on an input that **already carries** one of the four names has + no test, in either direction. Measured (`/tmp/flyrev/03_checks.R`, input given + `footprint_basis = "PRE-EXISTING"` and `dem_coverage = -99`): + + ``` + old (8585fd5): ... footprint_basis, dem_coverage, footprint_basis.1, footprint_terrain, height_agl, dem_coverage.1, geometry (23 cols) + new: ... footprint_basis, dem_coverage, footprint_terrain, height_agl, geometry (21 cols) + old footprint_basis[1] = "PRE-EXISTING" new footprint_basis[1] = "Film - BW" + ``` + + The new behaviour is the better of the two — under the old code the documented + workflow `footprints$footprint_basis != "unknown_format"` read the *caller's* column + and fly's real answer sat unnoticed in `footprint_basis.1`. But it is (a) silent data + loss of a caller-supplied column and (b) completely unguarded: a future + "simplification" back to trailing `st_sf()` arguments would reinstate the `.1` + duplicate for `data.frame` callers with **nothing in the suite failing**, since the + class sweep only asserts the columns are present and equal across shapes. Since NEWS + now advertises this as a deliberate second change, it wants an assertion behind it. + +- **[fragile]** `tests/testthat/test-fly_footprint.R` (class-contract test) + + `R/fly_footprint.R:152-154` (`@return`) — the contract asserted is stronger than the + one the code provides, and the fixture is structurally unable to show it. Measured + (`/tmp/flyrev/08_bcdc.R`) on the actual documented source shape: + + ``` + in : bcdc_sf, sf, tbl_df, tbl, data.frame + out: sf, bcdc_sf, tbl_df, tbl, data.frame identical(class(out), class(in)) == FALSE + ``` + + So `expect_identical(class(fly_footprint(x)), class(x))` holds for `plain`/`tbl`/ + `grouped` and **fails for a real `bcdata::collect()` result** — which is the caller + #35 exists for, and which the unchecked "End-to-end against a real `bcdata::collect()` + result" box in `task_plan.md` is the last defence for. The reordering is pre-existing + (`sf::st_transform()` does it, not `st_sf()` — verify: `class(st_transform(b, 3005))` + is already `sf, bcdc_sf, ...` before any of this code runs) and is identical old vs + new, so it is not a regression. Two consequences worth handling: + - the new `@return` sentence *"The input's class is preserved"* over-claims for + exactly that caller — the class *set* survives, the order does not; + - `NEWS.md:12` attributes the reordering to `sf::st_sf()`. It is `sf::st_transform()`. + + The four columns themselves **do** arrive correctly for `bcdc_sf` (verified TRUE), + so the fix works; only the stated contract is too strong. + +- **[fragile]** `R/fly_footprint.R:384` (pre-existing, newly exposed) — on a **0-row** + input, `footprint_basis` and `footprint_terrain` come back **`logical`**, not + `character`, because `ifelse(logical(0), ...)` returns `logical(0)`. Old and new are + identical here (`all.equal(old(x[0,]), new(x[0,]))` is `TRUE`), so this is not a + regression — but the fix makes the columns reach tibble callers for the first time, + and the type is load-bearing for the obvious downstream move + (`/tmp/flyrev/12_bindrows.R`): + + ``` + dplyr::bind_rows(fly_footprint(x[0, ]), fly_footprint(x)) + #> Error: Can't combine `..1$footprint_basis` and `..2$footprint_basis` + ``` + + Anyone assembling a per-AOI ledger across queries — the `stac_airphoto_bc` use case + in the issue — hits this the first time an AOI returns no frames. One line + (`character(0)` / `NA_character_` seeds, or `rep(NA_character_, n)` before the + `ifelse`) closes it, and it is cheap to fold in here since the release is already + about this function's reporting columns. + +- **[process]** working tree — a **concurrent session is editing this checkout** + (`DESCRIPTION`, `NEWS.md`, `planning/active/findings.md` changed *during* this + review; they were clean at the start). Per CLAUDE.md "Two agent sessions must not + share one git working tree". Also: I ran `devtools::document()` as part of the + verification, which regenerated `man/fly-package.Rd` to match their new + `DESCRIPTION` Title. That file is now modified and unstaged. The regeneration is + correct — do not revert it — but it is my write, not theirs. + +- **[note, not a defect]** `planning/active/task_plan.md` Phase 1 marks + *"Downstream pass-through: ... `fly_coverage()` / `fly_overlap()` / `fly_filter()` / + `fly_select()`"* complete, but the implemented test covers only the first three; + `fly_select()` is not in it. I exercised `fly_select()` (all three modes), + `fly_summary()` and `fly_bearing()` across plain/tibble/grouped myself + (`/tmp/flyrev/09_select.R`) — **all pass**, so there is no bug hiding behind the + checkbox, only PWF drift. + +## What I verified clean, and how + +Each of the seven questions asked, answered by measurement rather than reading. + +1. **Zero-row input** (`/tmp/flyrev/03_checks.R`). Old and new both return without + error and `all.equal(old, new)` is `TRUE` — same 21 names, same column types, + same everything. `$<-` on a 0-row tibble with a length-0 RHS is accepted (0 + matches `nrow`), and the four vectors are always exactly `nrow()` long + (`rep()`/`ifelse()` over length-`n` inputs), so no recycling difference exists to + find. The only 0-row wrinkle is the `logical` typing above, which is unchanged. + +2. **Input already carrying `footprint_basis`** — see finding 2. New behaviour is the + better one; the risk is that it is untested, not that it is wrong. + +3. **Non-sequential row names**, `centroids[c(5, 3, 11), ]` (`/tmp/flyrev/03_checks.R`). + No reorder, no drop, no misalignment, and old/new `all.equal` `TRUE`: + + ``` + input row.names 5,3,11 + old out row.names 1,2,3 airp_id 699365,699426,697358 + new out row.names 1,2,3 airp_id 699365,699426,697358 geometry identical: TRUE + ``` + + The `row.names` reset to `1:n` is `st_sf()`'s own `row.names = seq_along(x[[sf_column]])` + default and is present in both implementations. Alignment checked independently: + each output rectangle's centroid equals its input point (`all.equal`, tol 1e-6). + Repeated on the tibble subset — same geometry, four columns present. + +4. **`sf_column` and column ordering** (`/tmp/flyrev/03_checks.R`, `07_odd.R`). + `attr(, "sf_column")` is `"geometry"` and last in both; `names()`, `row.names()`, + `class()`, `agr` and `st_crs()` all identical to `8585fd5`, and `all.equal(old, new)` + is `TRUE`. Re-ran that comparison across seven awkward inputs to make sure the + `cbind(...)` fall-through branch had not shifted: non-syntactic column name, list + column, factor column, matrix column, single row, and an input whose sfc is named + `geom` (which the bundled fixture is — the output is renamed to `geometry` by both + implementations alike). **All seven: names identical, `all.equal` TRUE.** + +5. **Can the sweep pass vacuously?** No. Restored the bug properly — patched **both** + `asNamespace("fly")` and `as.environment("package:fly")` (the search-path copy is + what the test file resolves), from the extracted `8585fd5` bytes, and printed a + proof line before running (`/tmp/flyrev/04_restore.R`): + + ``` + patching 2 environments; search has package:fly: TRUE + PROOF broken: footprint_basis in names(fly_footprint(tbl)) == FALSE (must be FALSE) + -> test-fly_footprint.R goes red, 10-failure cap reached ("... and 13 more") + ``` + + On the specific sub-questions asked: + - The `plain` iteration of `expect_identical(names(out[[nm]]), names(out$plain))` + **is** a self-comparison that cannot fail, as are the per-column and + `st_geometry()` comparisons for `nm == "plain"`. This does **not** make the test + vacuous, because the guard is carried by the separate absolute assertion + `expect_true(all(reported %in% names(out[[nm]])))`, which is a real check for + every shape including `plain`. Three no-op assertions out of ~30; worth knowing, + not worth changing. + - No comparison is over an empty set. Measured (`/tmp/flyrev/05_vacuity.R`): + `fly_filter` 20 rows, `fly_coverage` 1 row with `covered_km2 = 24.8`, + `fly_overlap` 61 pairs, and in the `dem` sweep `any(footprint_terrain == "dem_agl")` + `TRUE` with 20 non-`NA` `dem_coverage` — the premise assertions in that test are + real and do reach the terrain code. + - The `stopifnot()` premise in `centroid_shapes()` **is** reachable (evaluated on + every one of the four calls) and rots in the safe direction: if `sf` ever makes + `st_read()` tibble-backed by default, `!inherits(plain, "tbl_df")` fires and names + the premise instead of the behaviour. One asymmetry — it asserts the `plain`/`tbl` + premises but not `inherits(grouped, "grouped_df")`, so the third shape has no + premise of its own. + - The class-contract test and the consumer pass-through test both pass on the broken + code (confirmed in the restore run). The first is explicitly commented as such; the + second is a forward guard rather than a #35 regression guard and is not labelled, + which is a smaller version of the same thing. + +6. **`.data$scale` in `dplyr::group_by()`** — safe. Verified two ways rather than by + reasoning about imports. In `R_DEFAULT_PACKAGES=base Rscript --vanilla` with + **nothing attached but base** (`/tmp/flyrev/06_data_pronoun.R`), `group_by(df, .data$scale)` + succeeds — `.data` comes from rlang's data mask, not the search path + (`exists(".data")` in globalenv is `FALSE`). And the full `R CMD check` runs + `testthat.R` clean: **Status: OK, 0 errors | 0 warnings | 0 notes**, tests `[78s/84s] OK`. + +7. **Other `st_sf()` sites** — `grep -rn "st_sf(" R/` gives exactly two. + `R/fly_footprint.R:64` is `st_sf(geometry = rects[ok])`, a bare geometry frame that + attaches nothing to user data and cannot hit the branch. The other is the fixed one. + I also swept the adjacent mechanisms rather than trusting the `st_sf` grep: + `cbind` / `bind_cols` / `st_set_geometry` have no hits; columns are attached to + user-supplied objects by `$<-` at `fly_coverage.R:34`, `fly_select.R:118,207-208`, + plus `fly_bearing.R` and `fly_georef.R` — which is the same mechanism the fix + adopts, so none are affected. Confirmed empirically by running `fly_select()` + (minimal / all / component_ensure), `fly_summary()` and `fly_bearing()` against all + three class shapes: no errors, correct classes out. + +## Other checks + +- Full suite: **FAIL 0 | WARN 0 | SKIP 0 | PASS 275** (baseline in `findings.md` was 225). +- `lintr::lint("R/fly_footprint.R")`: no lints, and no lints on the `HEAD` baseline + either — no change in lint count. +- `devtools::document()` printed no `Writing '.Rd'` for an unexpected file; + `NAMESPACE` unchanged, `grep -c "^export("` still 9, `export(fly_footprint)` present + — the roxygen block did not rebind. +- The staged `man/fly_footprint.Rd` is in sync with the roxygen source (re-running + `document()` produced no further diff to it). Its non-ASCII em-dashes match 18 + already present in that file at `HEAD` and in four other `.Rd` files, and + `R CMD check` is clean. diff --git a/planning/archive/2026-08-issue-35-footprint-tibble-columns/task_plan.md b/planning/archive/2026-08-issue-35-footprint-tibble-columns/task_plan.md new file mode 100644 index 0000000..b7af8f7 --- /dev/null +++ b/planning/archive/2026-08-issue-35-footprint-tibble-columns/task_plan.md @@ -0,0 +1,107 @@ +# Task: fly_footprint() drops footprint_basis and friends when input is a tibble (as bcdata returns) (#35) + +## Problem + +`fly_footprint()` silently drops `footprint_basis`, `footprint_terrain`, +`height_agl` and `dem_coverage` when its input carries the `tbl_df` class. + +`bcdata::collect()` returns exactly that class, so every caller querying +`WHSE_IMAGERY_AND_BASE_MAPS.AIMG_PHOTO_CENTROIDS_SP` — the documented source for +this package — loses the whole reporting surface 0.4.0 and 0.5.0 added. The +warning still fires, so the count is visible in the console and nowhere else. + +Mechanism: `sf::st_sf()` keeps only its **first** argument when that argument is +a tibble, discarding every trailing named column +(`else if (inherits(x[[1]], c("tbl_df", "tbl"))) x[[1]]`). One call site, +`R/fly_footprint.R:482`. Baseline before any change: FAIL 0 | PASS 225. + +## Phase 1: Regression tests first (must fail on unmodified source) + +- [x] Add `centroid_shapes()` to `tests/testthat/setup.R` — plain / tibble / + grouped shapes of the bundled fixture, via `sf::st_read(as_tibble = TRUE)` + rather than class hacking, with the premise asserted inline +- [x] Class-shape sweep in `test-fly_footprint.R`: all four columns present in + every shape; `names()` identical across shapes; the four columns' **values** + identical across shapes; output class equals input class with `sf` present +- [x] Repeat the sweep with `dem = testdata_path("dem.tif")` behind + `skip_if_no_terra()` — all four columns come from the same `st_sf()` call +- [x] Downstream pass-through: a tibble-backed footprint still flows through + `fly_coverage()` / `fly_overlap()` / `fly_filter()` / `fly_select()` +- [x] **Confirm the sweep fails on unmodified `R/`** before touching the source + +## Phase 2: The fix + +- [x] `R/fly_footprint.R:482` — assign the four columns onto the attribute frame, + then one-arg `sf::st_sf(attrs, geometry = ...)`. Preserves the caller's + class; chosen over `as.data.frame()`, which additionally downgrades a + bcdata caller's tibble +- [x] Comment names the `st_sf()` branch and #35, so the next reader does not + "simplify" it back +- [x] Restore the bug and confirm the sweep goes red — patching **both** + `asNamespace("fly")` and `as.environment("package:fly")`, from + `git show 8585fd5:R/fly_footprint.R` rather than from memory +- [x] Full suite green, PASS above the 225 baseline + +## Phase 2b: Review findings folded in + +Both reviewers ran independently against the branch; every claim below was +reproduced before acting on it. + +- [x] `@return` and the class test over-claimed — `identical(class(out), class(in))` + is FALSE for `bcdc_sf`, because `st_transform()` (not `st_sf()`) moves `sf` + to the front. Assert the class *set*, add the `bcdc` shape the fixture + lacked, and correct the `@return` and NEWS wording +- [x] The non-terra sweep compared constants and all-`NA`, so only *absence* made + it fail. Sweep the mixed-media fixture too, where `footprint_basis` varies + and reaches `unknown_format` +- [x] The collision behaviour was unguarded: the old code appended + `footprint_basis.1` and left the caller's value under the documented name, + so the prescribed filter read the wrong column. Now asserted +- [x] 0-row input returned `footprint_basis`/`footprint_terrain` as `logical` + (`ifelse(logical(0), ...)`), so an empty result would not bind to a + populated one — the per-AOI ledger in the issue. Seeded with + `as.character()` and guarded +- [x] Downstream test used `by = "photo_year"`, which is one group on this + fixture. Switched to `by = "scale"` (two), added non-degeneracy premises, + and added the `fly_select()` the checkbox claimed +- [x] Cut a false NEWS claim: `fly_footprint(fly_footprint(x))` is **not** + idempotent — a plain 20-row sf returns 100 rows silently, because + `st_coordinates()` on polygons yields 5 rows per feature. Pre-existing and + unchanged, but the sentence invited the round trip + +## Phase 3: Document the contract + +- [x] `@return` in `R/fly_footprint.R` states the input's class is preserved +- [x] `devtools::document()` — read its output; an unexpected `Writing '.Rd'` + or a falling `grep -c "^export(" NAMESPACE` means a roxygen block rebound +- [x] `lintr::lint_package()` compared against the `HEAD` baseline per file + +## Phase 4: DESCRIPTION Title (#31, folded in) + +- [x] Widen `Title:` and `Description:` to cover fetch and georeferencing +- [x] `devtools::document()`; `devtools::check()` for the title-case rules + +## Phase 5: Release v0.5.1 + +- [x] `NEWS.md` — name the class, the data source it breaks, and that geometry + and downstream numbers were always correct (reporting loss only) +- [x] Bump `DESCRIPTION` to `0.5.1` as the **final** commit of the branch +- [~] Tag `v0.5.1` — **deliberately not done on the branch.** `/gh-pr-merge` + does release bookkeeping after the merge, and a tag on a branch tip that + review may amend moves silently. Tag on `main`, after merge + +## Validation + +- [x] Tests pass +- [x] `/code-check` round 1 run on the fix; 5 findings, all folded in +- [x] End-to-end against a real `bcdata::collect()` result — the case the + fixture structurally cannot reach +- [ ] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion +- [ ] Post-merge, confirm the pkgdown deploy commit is the new `HEAD` + +## Out of scope — file as follow-ups + +- The `st_sf()` trailing-column behaviour as a `conventions/code-check.md` entry + in `soul`, plus a grep across the other NGE packages. General R trap, belongs + in `soul`, not on this branch. diff --git a/tests/testthat/setup.R b/tests/testthat/setup.R index 3d69915..d286b46 100644 --- a/tests/testthat/setup.R +++ b/tests/testthat/setup.R @@ -56,3 +56,54 @@ terrain_fixture <- function() { ) ) } + + +# The same bundled centroids in every class shape a real caller can supply. +# +# `bcdata::collect()` returns `bcdc_sf, sf, tbl_df, tbl, data.frame`, and #35 +# measured that `tbl_df` is the discriminating member — stripping the bcdata +# class alone does not change the outcome — so these shapes bound the real +# inputs without fly taking a dependency on bcdata to build a fixture. +# +# Read honestly rather than by overwriting `class()`: `st_read(as_tibble =)` +# produces the tibble-backed sf a caller would actually hold. The premise is +# asserted here so that a future `sf` change fails on the premise, naming the +# real cause, rather than on the behaviour under test. +centroid_shapes <- function() { + p <- testdata_path("photo_centroids.gpkg") + plain <- sf::st_read(p, quiet = TRUE) + tbl <- sf::st_read(p, quiet = TRUE, as_tibble = TRUE) + grouped <- dplyr::group_by(tbl, .data$scale) + # `bcdc_sf` is set by hand rather than by querying: bcdata is not a dependency + # of fly, and #35 measured that `tbl_df` is what selects the failing branch — + # so this shape exists to pin the *class contract* for the documented caller, + # not to reach the bug. `st_transform()` moves `sf` to the front of the class + # vector, so this is the one shape that shows the order is not preserved. + bcdc <- tbl + class(bcdc) <- c("bcdc_sf", class(bcdc)) + stopifnot( + !inherits(plain, "tbl_df"), + inherits(tbl, "tbl_df"), + inherits(grouped, "grouped_df"), + identical(class(bcdc)[1:2], c("bcdc_sf", "sf")) + ) + list(plain = plain, tbl = tbl, grouped = grouped, bcdc = bcdc) +} + + +# The bundled centroids are one film stock at one terrain treatment, so with no +# `dem` all four reporting columns are constant or all-`NA` — a value comparison +# across class shapes there is nearly vacuous, and only their *absence* makes it +# fail. The mixed-media fixture varies `footprint_basis` across four rows and +# reaches the `unknown_format` branch, so the sweep compares something. +mixed_media_shapes <- function() { + mm <- mixed_media_fixture() + tbl <- sf::st_as_sf(dplyr::as_tibble(mm)) + stopifnot(!inherits(mm, "tbl_df"), inherits(tbl, "tbl_df")) + list(plain = mm, tbl = tbl) +} + +# The columns #30 and #9 added, which #35 found were reaching no tibble caller. +fly_reported_cols <- function() { + c("footprint_basis", "footprint_terrain", "height_agl", "dem_coverage") +} diff --git a/tests/testthat/test-fly_footprint.R b/tests/testthat/test-fly_footprint.R index ee6f37e..e17bb64 100644 --- a/tests/testthat/test-fly_footprint.R +++ b/tests/testthat/test-fly_footprint.R @@ -647,3 +647,164 @@ test_that("fly_footprint reports coverage of the footprint it actually returns", expect_lt(truth, 0.95) # the fixture must reach the failure mode expect_equal(fp$dem_coverage, truth, tolerance = 1e-6) }) + + +test_that("fly_footprint reports the same columns whatever class the input carries", { + # #35: `sf::st_sf()` keeps only its first argument when that argument is a + # tibble, so the four reporting columns were discarded for every caller whose + # input carried `tbl_df` — which is what `bcdata::collect()` returns, and so + # what the package's own documented data source hands back. Every fixture in + # the package is plain `sf, data.frame`, so no case added along the existing + # axis could have found this. Sweep the axis instead. + shapes <- centroid_shapes() + reported <- fly_reported_cols() + + out <- lapply(shapes, fly_footprint) + + for (nm in names(out)) { + expect_true(all(reported %in% names(out[[nm]])), info = nm) + } + + # The bundled centroids give a constant basis and all-NA terrain columns, so + # the value comparison below would hold for any implementation that got the + # names right. Sweep the mixed-media fixture too, where `footprint_basis` + # varies across rows and reaches `unknown_format`. + mixed <- lapply(mixed_media_shapes(), function(x) suppressWarnings(fly_footprint(x))) + expect_gt(length(unique(mixed$plain$footprint_basis)), 1) # premise + for (nm in names(mixed)) { + expect_true(all(reported %in% names(mixed[[nm]])), info = nm) + expect_identical(mixed[[nm]]$footprint_basis, mixed$plain$footprint_basis, + info = nm) + } + + # Not merely present: identical, column for column, to what the plain shape + # gets. A fix that supplied the names and lost the values would pass the + # check above. + for (nm in names(out)) { + expect_identical(names(out[[nm]]), names(out$plain), info = nm) + for (col in reported) { + expect_identical(out[[nm]][[col]], out$plain[[col]], + info = paste(nm, col)) + } + expect_identical(sf::st_geometry(out[[nm]]), sf::st_geometry(out$plain), + info = nm) + } +}) + + +test_that("fly_footprint reports the same columns on the dem path too", { + skip_if_no_terra() + # All four columns are attached by the one `st_sf()` call, so the terrain path + # fails the same way — and it is `dem_coverage`, documented as the filter for + # partially-covered frames, that goes missing there. + dem <- terra::rast(testdata_path("dem.tif")) + shapes <- centroid_shapes() + reported <- fly_reported_cols() + + out <- lapply(shapes, function(x) suppressWarnings(fly_footprint(x, dem = dem))) + + # The fixture must reach the terrain code, or this sweep says nothing about it. + expect_true(any(out$plain$footprint_terrain == "dem_agl", na.rm = TRUE)) + expect_true(any(!is.na(out$plain$dem_coverage))) + + for (nm in names(out)) { + expect_true(all(reported %in% names(out[[nm]])), info = nm) + for (col in reported) { + expect_identical(out[[nm]][[col]], out$plain[[col]], + info = paste(nm, col)) + } + } +}) + + +test_that("fly_footprint carries every class the input had", { + # Contract, not the #35 regression guard: the broken code carried the class + # correctly and dropped the columns. This guards the other direction — a fix + # that coerced the frame to `data.frame` would hand a bcdata caller back + # something narrower than they passed in. + # + # Set, not sequence. `sf::st_transform()` moves `sf` to the front, so a + # `bcdc_sf` input comes back `sf, bcdc_sf, ...` — measured, and true of the + # prior implementation too. An `identical(class(out), class(in))` here would + # pass on the three shapes the fixture used to have and fail on the one + # caller the issue was filed about, which is the trap this branch exists to + # close rather than repeat. + shapes <- centroid_shapes() + expect_true("bcdc" %in% names(shapes)) # premise: the shape is present + for (nm in names(shapes)) { + out <- fly_footprint(shapes[[nm]]) + expect_true(all(class(shapes[[nm]]) %in% class(out)), info = nm) + expect_s3_class(out, "sf") + } +}) + + +test_that("fly_footprint overwrites a colliding reporting column", { + # Before #35 the trailing-argument form appended `footprint_basis.1` for a + # data.frame caller and kept the caller's value under the documented name — + # so `footprints$footprint_basis != "unknown_format"`, the filter the docs + # prescribe, read the caller's column and fly's real answer sat unread. The + # computed value must win. Unguarded, a revert to trailing `st_sf()` + # arguments would reinstate the duplicate with nothing failing. + shapes <- centroid_shapes() + for (nm in names(shapes)) { + x <- shapes[[nm]] + x$footprint_basis <- "PRE-EXISTING" + out <- fly_footprint(x) + expect_equal(sum(grepl("^footprint_basis", names(out))), 1L, info = nm) + expect_false(any(out$footprint_basis == "PRE-EXISTING"), info = nm) + } +}) + + +test_that("an empty result binds to a populated one", { + # `ifelse(logical(0), ...)` returns `logical(0)`, so a query matching no + # frames reported its basis as a logical column. Assembling a per-AOI ledger + # across queries — the use the reporting columns exist for — then fails on + # the type the first time an AOI returns nothing, rather than contributing no + # rows. Reachable only from the empty input, which no other test supplies. + shapes <- centroid_shapes() + for (nm in names(shapes)) { + empty <- fly_footprint(shapes[[nm]][0, ]) + expect_identical(nrow(empty), 0L, info = nm) + expect_type(empty$footprint_basis, "character") + expect_type(empty$footprint_terrain, "character") + expect_type(empty$height_agl, "double") + expect_type(empty$dem_coverage, "double") + } + full <- fly_footprint(shapes$plain) + bound <- dplyr::bind_rows(fly_footprint(shapes$plain[0, ]), full) + expect_identical(nrow(bound), nrow(full)) +}) + + +test_that("a tibble-backed footprint still flows through the consumers", { + # The four columns are new on this path, so check they do not disturb the + # functions that take a footprint. Numbers come from the plain shape, which + # the rest of the suite already pins. + shapes <- centroid_shapes() + aoi <- sf::st_read(testdata_path("aoi.gpkg"), quiet = TRUE) + + # `by = "scale"` because the fixture is a single photo year — grouping on it + # gives one group, and a one-row-against-one-row comparison is the weakest + # available. Premises assert each reference result is non-degenerate, since + # every comparison below passes when both sides are empty. + ref_filter <- fly_filter(shapes$plain, aoi) + ref_cover <- fly_coverage(shapes$plain, aoi, by = "scale") + ref_overlap <- fly_overlap(shapes$plain) + ref_select <- suppressMessages(fly_select(shapes$plain, aoi)) + expect_gt(nrow(ref_filter), 0) + expect_gt(nrow(ref_cover), 1) + expect_gt(nrow(ref_overlap), 0) + expect_gt(nrow(ref_select), 0) + + for (nm in names(shapes)) { + x <- shapes[[nm]] + expect_equal(nrow(fly_filter(x, aoi)), nrow(ref_filter), info = nm) + expect_equal(fly_coverage(x, aoi, by = "scale")$covered_km2, + ref_cover$covered_km2, info = nm) + expect_equal(nrow(fly_overlap(x)), nrow(ref_overlap), info = nm) + expect_equal(nrow(suppressMessages(fly_select(x, aoi))), nrow(ref_select), + info = nm) + } +})