Skip to content

fly_footprint() silently returns 5x the rows when handed its own output #37

Description

@NewGraphEnvironment

Problem

fly_footprint() accepts its own output and silently returns five times as many
rows
as it was given.

centroids <- sf::st_read(system.file("testdata/photo_centroids.gpkg", package = "fly"))
nrow(centroids)                              # 20
nrow(fly_footprint(centroids))               # 20
nrow(fly_footprint(fly_footprint(centroids))) # 100   <- no error, no warning

Tibble-backed input errors instead of returning wrong data, which is the safer
half of the same bug:

Error: Can't recycle input of size 100 to size 20.

Cause

fly_footprint() calls sf::st_coordinates(pts_3005) to get centroid positions.
On POINT input that is one row per feature. On POLYGON input it is one row per
vertex — five per closed rectangle — so fly_rectangles() builds 5n geometries
from n attribute rows.

sf::st_sf() then sets row.names = seq_along(sfc) = 1:100, and the
cbind(data.frame(row.names = ...), ...) branch recycles the 20-row attribute
frame up to 100. Nothing warns, and the result is a valid sf object whose
attributes are wrong for 80 of its rows.

Why it matters

The function's own documented output is polygons, so feeding it back is a natural
mistake — a pipeline that re-derives footprints after a filter, or any code that
cannot tell whether it holds centroids or footprints. There is no guard: the
function requires only an sf object with a scale column, and its output has
both.

The 5x is specific to closed rectangles. Any polygon input produces
sum(vertices) rows, so the factor varies with the geometry.

Not a regression

Present before and after #35fly_footprint() has always done this. Found
while a reviewer was disproving a claim I had written into the v0.5.1 NEWS entry
(that overwriting a colliding footprint_basis made the function idempotent; it
does not). Pre-existing and unrelated to the tibble bug, so it was deliberately
kept out of that patch release rather than folded into a PR about something else.

Suggested fix

Reject non-POINT input, rather than trying to make it work:

if (!all(sf::st_geometry_type(centroids_sf) %in% c("POINT", "MULTIPOINT"))) {
  stop("`centroids_sf` must be points. Ground coverage is estimated *from* a ",
       "centroid; passing footprints back in silently multiplies the rows by ",
       "the vertex count.", call. = FALSE)
}

An error is right here rather than a st_centroid() coercion: taking the
centroid of an estimated footprint and re-estimating from it is not a
meaningful operation, and quietly doing it would hide the caller's real mistake.

Worth checking the same input assumption in fly_bearing(), which also calls
st_coordinates() on centroids_sf.

Test

The guard needs a fixture that reaches it — the whole suite currently passes
points only, so this is the same fixture blind spot as #35 on a different axis:

test_that("fly_footprint refuses its own output", {
  centroids <- sf::st_read(testdata_path("photo_centroids.gpkg"), quiet = TRUE)
  fp <- fly_footprint(centroids)
  expect_equal(nrow(fp), nrow(centroids))      # premise
  expect_error(fly_footprint(fp), "must be points")
})

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions