Skip to content

Commit cada8a3

Browse files
author
Mike Johnson
committed
invariants: guard mainstem_id across all stages (merge + ngen + aggregated)
mainstem_id is a global reference levelpath id that must propagate intact through every stage / derivative layer -- not a minted per-stage id. Add a shared .hf_mainstem_check() (flags >5% NULL = dropped or mis-mapped) and wire it into hf_check_invariants for the aggregated and ngen stages AND into hf_check_merge_invariants. This regression (Stage-4 merge remapped mainstem_id through the flowpath-id table -> 78% NULL) shipped silently because nothing checked it; now any stage that drops it fails fast. Tests cover dropped + clean at both the per-stage and merge entry points.
1 parent 78cff38 commit cada8a3

3 files changed

Lines changed: 53 additions & 0 deletions

File tree

R/hf_invariants.R

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ hf_check_invariants <- function(stage, ..., strict = TRUE,
234234
sum(duplicated(ids)) == 0L,
235235
sprintf("aggregated flowpath_id has %d duplicate(s)",
236236
sum(duplicated(ids))))
237+
checks$mainstem_id_populated <- .hf_mainstem_check(flowpaths)
237238
}
238239

239240
if (!is.null(divides)) {
@@ -263,6 +264,7 @@ hf_check_invariants <- function(stage, ..., strict = TRUE,
263264
n_bad <- sum(!is.na(ids) & !startsWith(ids, "fp-"))
264265
checks$ngen_fp_prefix <- .hf_ok(n_bad == 0L,
265266
sprintf("%d ngen flowpath_id(s) do not start with 'fp-'", n_bad))
267+
checks$mainstem_id_populated <- .hf_mainstem_check(flowpaths)
266268
}
267269

268270
if (!is.null(divides)) {
@@ -327,6 +329,21 @@ hf_check_invariants <- function(stage, ..., strict = TRUE,
327329

328330
.hf_info <- function(msg) list(ok = TRUE, msg = msg, kind = "info")
329331

332+
# Shared mainstem_id population check. mainstem_id is a global reference
333+
# levelpath id that must propagate intact through every stage / derivative
334+
# layer -- it is NOT a minted per-stage id. A high NULL fraction means it was
335+
# dropped or mis-mapped (e.g. remapped through an id table it does not belong
336+
# to). Returns a check result, or .hf_info when the column is absent.
337+
.hf_mainstem_check <- function(flowpaths) {
338+
if (!"mainstem_id" %in% names(flowpaths)) {
339+
return(.hf_info("no mainstem_id column on flowpaths"))
340+
}
341+
frac <- mean(is.na(flowpaths$mainstem_id))
342+
.hf_ok(frac <= 0.05,
343+
sprintf("%.1f%% of flowpaths have NULL mainstem_id%s", 100 * frac,
344+
if (frac > 0.05) " -- dropped/mis-mapped?" else ""))
345+
}
346+
330347
.hf_ring_counts <- function(geoms) {
331348
vapply(geoms, function(g) {
332349
if (length(g) == 0L || sf::st_is_empty(g)) return(0L)
@@ -493,6 +510,9 @@ hf_check_merge_invariants <- function(merged, expected = NULL,
493510
if (frac_accum < 0.10) " -- WARNING: looks unaccumulated (topology resolved?)" else ""))
494511
} else checks$drainage_area_populated <- .hf_ok(FALSE, "total_dasqkm column absent from flowpaths")
495512

513+
# mainstem_id (a global reference levelpath id) must survive the merge intact.
514+
checks$mainstem_id_populated <- .hf_mainstem_check(fp)
515+
496516
# single CRS across all merged layers
497517
crs_of <- function(x) if (inherits(x, "sf")) sf::st_crs(x)$epsg else NA
498518
crs <- unique(stats::na.omit(c(crs_of(fp), crs_of(dv), crs_of(nx))))

tests/testthat/test-invariants.R

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,20 @@ test_that("ngen stage enforces fp-/cat- id prefixes", {
6060
"do not start with 'fp-'")
6161
})
6262

63+
test_that("ngen stage flags dropped/mis-mapped mainstem_id", {
64+
drop_fp <- data.frame(flowpath_id = paste0("fp-", 1:5),
65+
flowpath_toid = "0", mainstem_id = c(10, rep(NA_real_, 4)), # 80% NULL
66+
stringsAsFactors = FALSE)
67+
res <- suppressMessages(hf_check_invariants("ngen", flowpaths = drop_fp,
68+
strict = FALSE))
69+
expect_false(res$checks$mainstem_id_populated$ok)
70+
71+
full_fp <- transform(drop_fp, mainstem_id = 11:15)
72+
res2 <- suppressMessages(hf_check_invariants("ngen", flowpaths = full_fp,
73+
strict = FALSE))
74+
expect_true(res2$checks$mainstem_id_populated$ok)
75+
})
76+
6377
test_that("aggregated stage catches duplicate flowpath_id", {
6478
fp <- data.frame(flowpath_id = c("1", "1"), stringsAsFactors = FALSE)
6579
expect_error(

tests/testthat/test-merge-invariants.R

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,22 @@ test_that("hf_check_merge_invariants strict stops on failure", {
3939
hf_check_merge_invariants(list(flowpaths = fp, divides = dv, nexus = NULL),
4040
strict = TRUE)))
4141
})
42+
43+
test_that("hf_check_merge_invariants flags dropped/mis-mapped mainstem_id", {
44+
# mostly-NULL mainstem_id -- the merge-drop regression this guards against
45+
fp <- data.frame(flowpath_id = paste0("fp-", 1:10), vpuid = "01",
46+
total_dasqkm = 1:10, areasqkm = rep(0.5, 10),
47+
mainstem_id = c(101, rep(NA_real_, 9))) # 90% NULL
48+
dv <- data.frame(divide_id = paste0("cat-", 1:10), vpuid = "01")
49+
res <- suppressMessages(
50+
hf_check_merge_invariants(list(flowpaths = fp, divides = dv, nexus = NULL),
51+
strict = FALSE))
52+
expect_false(res$checks$mainstem_id_populated$ok)
53+
54+
# fully-populated mainstem_id passes
55+
fp$mainstem_id <- 100 + (1:10)
56+
res2 <- suppressMessages(
57+
hf_check_merge_invariants(list(flowpaths = fp, divides = dv, nexus = NULL),
58+
strict = FALSE))
59+
expect_true(res2$checks$mainstem_id_populated$ok)
60+
})

0 commit comments

Comments
 (0)