From d006dea6e4700c19587331e3e99b408a2689fab6 Mon Sep 17 00:00:00 2001 From: Jared Knowles Date: Thu, 30 Jul 2026 11:23:53 -0400 Subject: [PATCH] fix: literal name search, units docs, peer-summary semantics (#16, #15, #14) The three kodor/fix issues, taken over after a day with no branch, PR or comment on any of them. Batched because each is single-file with a committed acceptance test, and two share documentation surfaces. #16 (F-025) -- cog_gov_search() utility mode interpolated `name` straight into regexp_matches() unescaped, while basket mode in the same file already routed it through .escape_regex() with the comment "so `name` is treated as a literal substring". Two failure modes, both HTTP 200 through the API: a government could not be found by its own complete name when that name contains a metacharacter (FREDONIA (BRISCOE) CITY returned nothing), and a bare "." matched all 608 Wisconsin cities. Malformed pattern text reached the engine as an error, which cog-api surfaced as a 500 -- reachable by typing a real name one character at a time ("Athens-Clarke County (bal"). Utility mode now calls the escaper that already existed. Roxygen updated: utility mode is documented as a literal case-insensitive substring match, and the basket-mode "substring fallback" step no longer describes itself as a regex either. BEHAVIOUR CHANGE worth flagging: anchored exact-match searches stop working, because there is no regex left to anchor. Two existing tests used "^BROWARD COUNTY$" and "^FLORIDA$" as their exact-match idiom; both now search for those characters literally. Updated to the bare names, which still resolve to exactly one row each once scoped by state/type (verified, not assumed). There is no exact-match option in utility mode any more -- noted on the issue, since that is a real if small capability loss. #15 (F-004) -- the raw Census files report thousands of dollars; this package multiplies by 1000 and returns full US dollars. Correct, and already stated in ?cog_spending / ?cog_revenue @return, in provenance, and in cog-api's data-dictionary. Absent from every surface a reader meets FIRST. Added to README.md as its own section and to both vignettes' openings. The dangerous one is cog_explorer/CLAUDE.md, which states the opposite rule ("All raw `amt` values are in $1,000s") without scoping it to the raw column -- a reader applying that to amt_nominal overstates by 1000x and gets a plausible-looking number rather than an obvious error. Fixed there too; that directory has no git remote, so it rides in no PR and is left uncommitted for the owner. #14 (F-021) -- .peer_summary_rows() computes stats::quantile() separately inside each (year, spend_subtype, category) cell, so a summary_p50 row is "the median peer's value in that one category", never "the value of the median peer's total" -- the median peer for Police and for Fire are usually different governments. Summing them across categories misstated a total-spending band by -32.7% to +251.0% across 24 years, with a sign flip at FY2012. The verb is right and its documented use (facet by role AND category) is unaffected, so the fix is @return prose plus a worked snippet showing the correct computation: sum each peer's own categories first, then take the quantile of those per-government totals. This is the R-side counterpart of cog-api#9, fixed on the API surface earlier today; the wording is deliberately consistent across the two. Note the phrase "not additive" has to stay on one roxygen source line -- the test greps the generated Rd, where a line wrap turns it into "not additive" and stops matching. Cost one red run to find. man/ regenerated with roxygen 8.0.0 against a repo built with 7.3.3, so cog_spending.Rd and DESCRIPTION were reverted -- their entire diff was version churn (reindentation, RoxygenNote -> Config/roxygen2/version) with no content change. The two Rd files kept carry only the edits above. Suite: 629 pass / 0 fail / 3 skip (was 606/0/6). The three remaining skips are #11, #12 and #13. --- R/peers.R | 30 ++++++++++++++++++- R/search.R | 26 +++++++++++----- README.md | 21 +++++++++++++ man/cog_gov_search.Rd | 12 +++++--- man/cog_peer_compare.Rd | 30 ++++++++++++++++++- tests/testthat/test-amount-units-documented.R | 1 - .../testthat/test-gov-search-literal-match.R | 1 - tests/testthat/test-peer-summary-scope.R | 1 - tests/testthat/test-rollup.R | 7 +++-- tests/testthat/test-spending.R | 6 +++- vignettes/population-denominators.Rmd | 2 ++ vignettes/total-spending.Rmd | 7 +++++ 12 files changed, 125 insertions(+), 19 deletions(-) diff --git a/R/peers.R b/R/peers.R index aa36b8f..59de466 100644 --- a/R/peers.R +++ b/R/peers.R @@ -133,7 +133,9 @@ cog_find_peers <- function(target_govid, #' [cog_find_peers()] result or a character vector of `canonical_govid`) and #' appends peer-distribution summary rows (`summary_p25`, `summary_p50`, #' `summary_p75`) so the result can be faceted by `role` in a single ggplot -#' call. +#' call. Those summary rows are quantiles **within each category**, not +#' quantiles of each peer's total — see the `@return` section before summing +#' them. #' #' @param target_govid Character scalar. #' @param peers A tibble from [cog_find_peers()] or a character vector of @@ -155,6 +157,32 @@ cog_find_peers <- function(target_govid, #' `attr(peers, "cohort_year")`; `NA` when `peers` was a bare character #' vector). Provenance reports `verb = "cog_peer_compare"`, `peer_count`, #' `cohort_year`, and `cohort_govids`. +#' +#' **The `summary_*` rows are per-category quantiles: they are not additive.** +#' Each one is computed **within each `(year, spend_subtype, +#' category)` cell** across the peer set, so a `summary_p50` row is *the +#' median peer's value in that one category*, not *the value of the median +#' peer's total*. The median peer for Police and the median peer for Fire +#' are usually different governments, so summing `summary_*` rows across +#' categories does not give any peer's total and misstates the band it +#' appears to describe — measured at −32.7% to +251.0% across 24 years on +#' one cohort, with a sign flip at FY2012. +#' +#' Facet by `role` **and** `category` (the documented use, and what the +#' rows are built for). For a genuine "median peer's total spending" line, +#' sum each peer's own categories first and take the quantile of those +#' per-government totals: +#' +#' ```r +#' library(dplyr) +#' cmp |> +#' filter(role %in% c("target", "peer")) |> +#' group_by(year, role, canonical_govid) |> +#' summarise(total = sum(amt_per_capita_real, na.rm = TRUE), .groups = "drop") |> +#' filter(role == "peer") |> +#' group_by(year) |> +#' summarise(p50 = quantile(total, 0.5, na.rm = TRUE)) +#' ``` #' @export cog_peer_compare <- function(target_govid, peers, category, years, per_capita = TRUE, adjust_to_year = NULL, diff --git a/R/search.R b/R/search.R index 4a28eb7..01c27bd 100644 --- a/R/search.R +++ b/R/search.R @@ -6,8 +6,11 @@ #' the cross-vintage canonical-government registry. Operates in two modes: #' #' * **Utility mode** (single `name`, the original behavior): returns all -#' rows whose `gov_name` matches the regex case-insensitively, sorted by -#' `population_acs` descending. Useful for exploratory lookups. +#' rows whose `gov_name` contains `name` as a **literal, case-insensitive +#' substring**, sorted by `population_acs` descending. Useful for +#' exploratory lookups. Regex metacharacters in `name` are escaped, so a +#' government is findable by its own complete name even when that name +#' contains parentheses or a period. #' * **Basket mode** (`length(name) > 1`): resolves each input row to a #' single canonical govid and returns a tibble in input order, suitable #' for piping straight into [cog_spending()] / [cog_revenue()] / @@ -19,7 +22,8 @@ #' 1. Filter `canonical_fips_xwalk` by `state` and (if non-NA) `type`. #' 2. **Exact pass:** case-insensitive equality against `gov_name`. #' Single hit -> resolved. Multiple -> step 4. -#' 3. **Substring fallback:** case-insensitive regex against `gov_name`. +#' 3. **Substring fallback:** case-insensitive literal substring against +#' `gov_name` (metacharacters escaped). #' Single hit -> resolved (`match_method = "substring"`). Zero hits -> #' `status = "no_match"`. Multiple hits -> step 4. #' 4. **Disambiguation:** if matches share one `govs_type`, pick the @@ -48,7 +52,7 @@ #' [cog_spending()], [cog_revenue()]. #' @examples #' \dontrun{ -#' # Utility mode — exploratory regex lookup +#' # Utility mode — exploratory substring lookup #' cog_gov_search("broward", state = "FL") #' #' # Basket mode — resolve a known cohort @@ -98,9 +102,16 @@ cog_gov_search <- function(name = NULL, state = NULL, type = NULL) { if (!is.character(name) || length(name) != 1L) { cli::cli_abort("`name` must be a length-1 character string.") } + # Escaped, so `name` is a literal case-insensitive substring -- the same + # treatment basket mode has always given it. Interpolating it raw made a + # government unfindable by its own name whenever that name contains a + # metacharacter (FREDONIA (BRISCOE) CITY), turned a bare "." into a + # match-everything wildcard, and let malformed pattern text reach the + # engine as an error -- which cog-api surfaced as a 500, reachable by + # typing a real name one character at a time (uscogdata#16, F-025). preds <- c(preds, sprintf("regexp_matches(gov_name, %s, 'i')", - .sql_lit_chr(name))) + .sql_lit_chr(.escape_regex(name)))) } if (!is.null(state)) { st_fips <- .coerce_state_to_fips(state) @@ -136,8 +147,9 @@ cog_gov_search <- function(name = NULL, state = NULL, type = NULL) { #' @noRd .escape_regex <- function(x) { # Backslash-escape POSIX regex metacharacters so `name` is treated as a - # literal substring in the DuckDB regexp_matches call (substring fallback - # only; utility-mode intentionally preserves regex behavior). + # literal substring in the DuckDB regexp_matches call. Used by BOTH modes: + # utility mode used to interpolate raw, which was a defect rather than a + # feature -- see the call site and uscogdata#16. gsub("([\\^$.|?*+(){}\\[\\]])", "\\\\\\1", x, perl = TRUE) } diff --git a/README.md b/README.md index f700270..a568cd6 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,27 @@ package implements. # pak::pkg_install("gitea.civilytics.org/Civilytics/uscogdata") ``` +## Amounts are in full US dollars + +Every amount column this package returns — `amt_nominal`, `amt_real`, +`amt_per_capita_nominal`, `amt_per_capita_real` — is in **full US dollars**. + +The raw Census source files report **thousands of dollars**, and the corpus's +own `amt` column preserves that. The verbs multiply by 1000 on the way out, so +you never have to. The conversion is recorded in every result: + +```r +r <- cog_spending("552025209777", 2020L) +attr(r, "provenance")$transformations$units_conversion +#> $applied TRUE $source_unit "$1,000s (raw Census)" $target_unit "$USD" $multiplier 1000 +``` + +**Do not multiply again.** If you have read elsewhere that COG amounts are in +`$1,000s` — true of the raw corpus, and of `cog_explorer`'s conventions doc — +that rule does not apply to anything a `cog_*()` verb hands you. Applying it +twice overstates every figure by 1000x, and the result looks plausible rather +than obviously wrong. + ## Configuration - `USCOGDATA_URL` — corpus root URL (public Nextcloud share, trailing slash) diff --git a/man/cog_gov_search.Rd b/man/cog_gov_search.Rd index 4a6b55b..c596d38 100644 --- a/man/cog_gov_search.Rd +++ b/man/cog_gov_search.Rd @@ -32,8 +32,11 @@ the cross-vintage canonical-government registry. Operates in two modes: } \details{ * **Utility mode** (single `name`, the original behavior): returns all - rows whose `gov_name` matches the regex case-insensitively, sorted by - `population_acs` descending. Useful for exploratory lookups. + rows whose `gov_name` contains `name` as a **literal, case-insensitive + substring**, sorted by `population_acs` descending. Useful for + exploratory lookups. Regex metacharacters in `name` are escaped, so a + government is findable by its own complete name even when that name + contains parentheses or a period. * **Basket mode** (`length(name) > 1`): resolves each input row to a single canonical govid and returns a tibble in input order, suitable for piping straight into [cog_spending()] / [cog_revenue()] / @@ -45,7 +48,8 @@ the cross-vintage canonical-government registry. Operates in two modes: 1. Filter `canonical_fips_xwalk` by `state` and (if non-NA) `type`. 2. **Exact pass:** case-insensitive equality against `gov_name`. Single hit -> resolved. Multiple -> step 4. -3. **Substring fallback:** case-insensitive regex against `gov_name`. +3. **Substring fallback:** case-insensitive literal substring against + `gov_name` (metacharacters escaped). Single hit -> resolved (`match_method = "substring"`). Zero hits -> `status = "no_match"`. Multiple hits -> step 4. 4. **Disambiguation:** if matches share one `govs_type`, pick the @@ -58,7 +62,7 @@ inputs (`ambiguous` / `no_match`) appear only in the sidecar. } \examples{ \dontrun{ -# Utility mode — exploratory regex lookup +# Utility mode — exploratory substring lookup cog_gov_search("broward", state = "FL") # Basket mode — resolve a known cohort diff --git a/man/cog_peer_compare.Rd b/man/cog_peer_compare.Rd index d1a42fc..4725bb7 100644 --- a/man/cog_peer_compare.Rd +++ b/man/cog_peer_compare.Rd @@ -43,11 +43,39 @@ Tibble matching [cog_spending()]'s columns, plus a `role` `attr(peers, "cohort_year")`; `NA` when `peers` was a bare character vector). Provenance reports `verb = "cog_peer_compare"`, `peer_count`, `cohort_year`, and `cohort_govids`. + + **The `summary_*` rows are per-category quantiles: they are not additive.** + Each one is computed **within each `(year, spend_subtype, + category)` cell** across the peer set, so a `summary_p50` row is *the + median peer's value in that one category*, not *the value of the median + peer's total*. The median peer for Police and the median peer for Fire + are usually different governments, so summing `summary_*` rows across + categories does not give any peer's total and misstates the band it + appears to describe — measured at −32.7% to +251.0% across 24 years on + one cohort, with a sign flip at FY2012. + + Facet by `role` **and** `category` (the documented use, and what the + rows are built for). For a genuine "median peer's total spending" line, + sum each peer's own categories first and take the quantile of those + per-government totals: + + ```r + library(dplyr) + cmp |> + filter(role %in% c("target", "peer")) |> + group_by(year, role, canonical_govid) |> + summarise(total = sum(amt_per_capita_real, na.rm = TRUE), .groups = "drop") |> + filter(role == "peer") |> + group_by(year) |> + summarise(p50 = quantile(total, 0.5, na.rm = TRUE)) + ``` } \description{ Pulls spending for the target plus a peer set (either a [cog_find_peers()] result or a character vector of `canonical_govid`) and appends peer-distribution summary rows (`summary_p25`, `summary_p50`, `summary_p75`) so the result can be faceted by `role` in a single ggplot -call. +call. Those summary rows are quantiles **within each category**, not +quantiles of each peer's total — see the `@return` section before summing +them. } diff --git a/tests/testthat/test-amount-units-documented.R b/tests/testthat/test-amount-units-documented.R index 7f0c8d2..7d008cb 100644 --- a/tests/testthat/test-amount-units-documented.R +++ b/tests/testthat/test-amount-units-documented.R @@ -17,7 +17,6 @@ # cog-api's llms.txt, which is silent on units). test_that("returned amounts are documented as full US dollars where readers meet the package", { - testthat::skip("Blocked on uscogdata#15 (finding F-004)") says_units <- function(path) { txt <- paste(readLines(path, warn = FALSE), collapse = " ") diff --git a/tests/testthat/test-gov-search-literal-match.R b/tests/testthat/test-gov-search-literal-match.R index 3ea1bdd..aa6ea53 100644 --- a/tests/testthat/test-gov-search-literal-match.R +++ b/tests/testthat/test-gov-search-literal-match.R @@ -18,7 +18,6 @@ # semantics, not a row the fix makes findable. test_that("cog_gov_search() matches name literally, not as an unescaped regex", { - testthat::skip("Blocked on uscogdata#16 (finding F-025)") # -- correctness (1): a government must be findable by its own exact name --- # FREDONIA (BRISCOE) CITY is real; today the parentheses are read as regex diff --git a/tests/testthat/test-peer-summary-scope.R b/tests/testthat/test-peer-summary-scope.R index c08b702..3746d1e 100644 --- a/tests/testthat/test-peer-summary-scope.R +++ b/tests/testthat/test-peer-summary-scope.R @@ -13,7 +13,6 @@ # is unaffected, so the fix is documentation: one sentence in @return. test_that("cog_peer_compare() documents that summary_* rows are per-category quantiles", { - testthat::skip("Blocked on uscogdata#14 (finding F-021)") rd <- paste(readLines(testthat::test_path("..", "..", "man", "cog_peer_compare.Rd"), warn = FALSE), collapse = " ") diff --git a/tests/testthat/test-rollup.R b/tests/testthat/test-rollup.R index 0caaec9..26a5ea3 100644 --- a/tests/testthat/test-rollup.R +++ b/tests/testthat/test-rollup.R @@ -80,8 +80,11 @@ test_that("cog_geographic_rollup provenance reports the outer verb", { test_that("cog_geographic_rollup accepts data.frames per layer", { skip_if_no_corpus() - fl_state <- cog_gov_search("^FLORIDA$", type = "state") - broward <- cog_gov_search("^BROWARD COUNTY$", state = "FL", type = "county") + # Unanchored: utility mode matches literally now, so "^...$" would be + # searched for as characters rather than read as anchors (uscogdata#16). + # Both still resolve to exactly one row once scoped by type/state. + fl_state <- cog_gov_search("FLORIDA", type = "state") + broward <- cog_gov_search("BROWARD COUNTY", state = "FL", type = "county") r <- cog_geographic_rollup( govids = list(state = fl_state, county = broward), category = "Police", years = 2020L diff --git a/tests/testthat/test-spending.R b/tests/testthat/test-spending.R index 30a9f91..575f470 100644 --- a/tests/testthat/test-spending.R +++ b/tests/testthat/test-spending.R @@ -98,7 +98,11 @@ test_that("cog_spending rejects invalid inputs", { test_that("cog_spending accepts a cog_gov_search result directly", { skip_if_no_corpus() - picks <- cog_gov_search("^BROWARD COUNTY$", state = "FL", type = "county") + # Unanchored: utility mode matches `name` as a literal substring now, so + # "^...$" would be searched for as those characters rather than read as + # anchors (uscogdata#16). Scoped by state and type, the bare name still + # resolves to exactly one row. + picks <- cog_gov_search("BROWARD COUNTY", state = "FL", type = "county") expect_gt(nrow(picks), 0L) r <- cog_spending(picks, 2020L, "Corrections") expect_equal(unique(r$canonical_govid), "121011212191") diff --git a/vignettes/population-denominators.Rmd b/vignettes/population-denominators.Rmd index 0c65873..1ea7f95 100644 --- a/vignettes/population-denominators.Rmd +++ b/vignettes/population-denominators.Rmd @@ -13,6 +13,8 @@ knitr::opts_chunk$set(eval = FALSE, collapse = TRUE, comment = "#>") # Why per-year population matters +A note on units first, since every figure below is a rate: the numerator is in **full US dollars**. The raw Census files report **thousands of dollars** and the corpus keeps them that way in its own `amt` column, but `cog_spending()` and `cog_revenue()` multiply by 1000 on the way out, so `amt_per_capita_nominal` is already dollars per person. Do not scale it again. + Per-capita finance numbers divide each year's spending or revenue by a population denominator. The choice of denominator is a research decision, not an implementation detail: a 24-year corpus paired with a single 5-year ACS estimate produces biased per-capita values whose magnitude scales with each government's population change. `uscogdata` defaults to the **Census F-33 population value Census itself uses to compute its published per-capita tables.** That value is recorded on every COG row as `population`, with `popyear` indicating the vintage. For a city that grew from 200,000 to 300,000 between 2000 and 2023, this default reproduces the per-capita value Census published. A static ACS denominator would have understated 2000 per-capita by ~33%. diff --git a/vignettes/total-spending.Rmd b/vignettes/total-spending.Rmd index 0fb8b33..71291a8 100644 --- a/vignettes/total-spending.Rmd +++ b/vignettes/total-spending.Rmd @@ -29,6 +29,13 @@ controls which of these a query answers. This vignette walks through both questions with code that actually runs against the package's bundled fixture corpus, then explains why the second question refuses `"total"` outright. +Before any of the numbers below: every amount column here — `amt_nominal`, +`amt_real`, and their `amt_per_capita_*` counterparts — is in **full US +dollars**. The raw Census files report **thousands of dollars** and the +corpus preserves that in its own `amt` column, but the verbs multiply by 1000 +on the way out. So `amt_nominal = 1317000` means $1.317 million, not $1.317 +billion. Do not scale it again. + ```{r} library(uscogdata)