Let covariate_columns() report the source columns that travel with the data

Both resamplers were written expecting a <var>_source column to ride along.
resolve_methods() says so outright - "a source column riding along should not
force the caller to enumerate every column" - and each carries a non-numeric
column as a categorical: the commonest value when aggregating, the nearest or a
step when interpolating, which is what a source tag needs and an average is not.

But covariate_columns() is the default `vars` for both, and it excluded them.
So a source column never rode along. Regridding or retiming a gap-filled object
dropped the record of which source each value came from, at exactly the point
that record matters most - the values have just been combined.

The reasoning behind the old exclusion still holds for the other two cases, and
they stay out. <var>_depth is a level index, and the mean of two depths is not
the depth any value came from. .datamatch_source is internal bookkeeping that
matchData() consumes. is_provenance_column() becomes is_bookkeeping_column(),
which is what it now describes.

Nothing downstream had to change. plot_series() already filtered to numeric
columns, and both resamplers already handled categoricals. taupatch's
test-prejoin.R has been failing on this since a newer datamatch was installed
there - it expected covariate_columns() to return CHL_source, and it was right.
It passes now with no change to taupatch.

datamatch: 1,440 tests pass, check is OK. taupatch: 1,279 pass, none failing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: chross22

NEWS.md

@@ -1,5 +1,26 @@
 # datamatch 0.2.0
 
+## Breaking changes
+
+* **`covariate_columns()` now reports `<var>_source` columns.** They are
+  provenance, but they travel with the variable they describe rather than being
+  left behind by it, and both resamplers were already written on that
+  assumption: `upscale_grid()` and `upscale_time()` carry a non-numeric column
+  as a categorical - the commonest value when aggregating, the nearest or a step
+  when interpolating - and `resolve_methods()` says in as many words that "a
+  source column riding along should not force the caller to enumerate every
+  column".
+
+  Because `covariate_columns()` is the default `vars` for both, a source column
+  never actually rode along. Regridding or retiming a gap-filled object dropped
+  the record of which source each value came from, at the point it matters most.
+
+  `<var>_depth` is still excluded, since the mean of two depths is not the depth
+  any value came from, as is the internal per-row source tag.
+
+  Callers relying on `covariate_columns()` to return only measurements should
+  filter on `is.numeric()`, as `plot_series()` does.
+
 ## New features
 
 * **A date that has not happened yet is refused before anything is fetched.**

R/gapfill.R

@@ -1,6 +1,12 @@
 #' Covariate column names in an environmental data object
 #'
-#' Everything that is not a time column or the geometry.
+#' Everything that is not a time column, the geometry, or bookkeeping.
+#'
+#' `<var>_source` columns are included. They are provenance, but they travel
+#' with the variable they describe: [upscale_grid()] and [upscale_time()] carry
+#' a non-numeric column as a categorical, so a resampled object keeps the record
+#' of which source each value came from. `<var>_depth` is excluded, because the
+#' mean of two depths is not the depth any value came from.
 #'
 #' @param env_dat an `sf` POINT object from any access function -
 #'   [accessCopernicus()], [accessFVCOM()], [accessHYCOM()], [accessCCMP()] or
@@ -14,9 +20,7 @@
 covariate_columns <- function(env_dat) {
   candidates <- setdiff(names(env_dat),
                         c(time_columns(), attr(env_dat, "sf_column")))
-  # Provenance columns describe the data rather than measuring anything, so they
-  # must not be aggregated, regridded or plotted as though they were covariates.
-  candidates[!is_provenance_column(candidates)]
+  candidates[!is_bookkeeping_column(candidates)]
 }
 
 #' Fill satellite gaps with the model equivalent

R/matchData.R

@@ -66,8 +66,12 @@
 #' `<var>_source` column [fill_satellite_gaps()] writes. Pass
 #' `record_source = FALSE` to omit them.
 #'
-#' These are provenance rather than data: [covariate_columns()] excludes them, so
-#' they are not aggregated, regridded or plotted as though they were measurements.
+#' These are provenance rather than measurements, but they travel with the
+#' variable they describe: [covariate_columns()] reports them, and resampling
+#' carries them as a categorical - the commonest value when aggregating, the
+#' nearest when interpolating - so a regridded or retimed object still says
+#' which source each value came from. [plot_series()] skips them, since a source
+#' tag has no mean.
 #'
 #' @param dat <sf object> the points to add columns to: observations, stations,
 #'   tag positions, anything with coordinates and time. Needs year and month

R/provenance.R

@@ -78,18 +78,27 @@ stamp_source <- function(x, source, detail) {
   x
 }
 
-#' Columns that record where a value came from rather than what it is
+#' Columns that are bookkeeping rather than data
 #'
-#' `<var>_source` columns written by [matchData()] and [fill_satellite_gaps()],
-#' and the `<var>_depth` column a derived bottom variable returns. They describe
-#' the data rather than measuring anything, so aggregating or interpolating them
-#' is meaningless — a mean of two source tags is not a source, and the mean of
-#' two depths is not the depth any value came from.
+#' `<var>_depth` records which model level a derived bottom value was taken
+#' from, and `.datamatch_source` is the per-row source tag a fetch spanning
+#' several archives carries. Neither measures anything: the mean of two depths
+#' is not the depth any value came from, and the internal tag is consumed by
+#' [matchData()] rather than kept. So [covariate_columns()] leaves both out, and
+#' nothing resamples or plots them as though they were covariates.
+#'
+#' @section Why `<var>_source` is not here:
+#' It is provenance too, but it travels with the variable it describes rather
+#' than being left behind by it. [upscale_grid()] and [upscale_time()] carry a
+#' non-numeric column as a categorical - the commonest value when aggregating,
+#' the nearest when interpolating - which is exactly what a source tag needs,
+#' and both were written expecting one to ride along. Excluding it here meant it
+#' never did, so the record of which source a value came from was lost at the
+#' first resample, which is the point at which it matters most.
 #'
 #' @param names <char> column names to inspect
 #' @return <logical> one per name
 #' @keywords internal
-is_provenance_column <- function(names) {
-  grepl("_source$", names) | grepl("_depth$", names) |
-    names == ".datamatch_source"
+is_bookkeeping_column <- function(names) {
+  grepl("_depth$", names) | names == ".datamatch_source"
 }

man/covariate_columns.Rd

@@ -134,3 +134,51 @@ test_that("missing columns are reported with what is available", {
   expect_error(fill_satellite_gaps(sources$satellite, sources$model,
                                     c(CHL = "MISSING")), "Model has")
 })
+
+# `<var>_source` is provenance, but it travels with the variable it describes
+# rather than being left behind by it. Both resamplers were written expecting a
+# source column to ride along - they carry a non-numeric column as a categorical
+# - but covariate_columns() used to strip it, so it never arrived and the record
+# of which source a value came from was lost at the first resample.
+
+test_that("covariate_columns reports <var>_source but not bookkeeping", {
+  d <- sf::st_as_sf(
+    data.frame(x = c(1, 2), y = c(1, 2), SST = c(4, 5),
+               SST_source = c("satellite", "model"), BOTS_depth = c(80, 90),
+               YEAR = 2010L, MONTH = 6L, DAY = 1L),
+    coords = c("x", "y"), crs = 4326)
+
+  cols <- covariate_columns(d)
+  expect_true("SST_source" %in% cols)
+  expect_true("SST" %in% cols)
+  # The mean of two depths is not the depth any value came from.
+  expect_false("BOTS_depth" %in% cols)
+  # Time and geometry are never covariates.
+  expect_false(any(c("YEAR", "MONTH", "DAY", "geometry") %in% cols))
+})
+
+test_that("the internal per-row source tag stays out of covariate_columns", {
+  d <- sf::st_as_sf(
+    data.frame(x = c(1, 2), y = c(1, 2), SST = c(4, 5),
+               YEAR = 2010L, MONTH = 6L, DAY = 1L),
+    coords = c("x", "y"), crs = 4326)
+  d <- stamp_source(d, "hycom", c("GLBv53X", "GLBy930"))
+
+  expect_true(".datamatch_source" %in% names(d))
+  expect_false(".datamatch_source" %in% covariate_columns(d))
+})
+
+test_that("a source column survives upscaling, as the commonest value", {
+  sources <- gappy_sources(gap_fraction = 0.3)
+  filled <- fill_satellite_gaps(sources$satellite, sources$model,
+                                c(CHL = "CHL_MODEL"))
+
+  # No `vars`: the default is covariate_columns(), which is the whole point -
+  # a caller should not have to enumerate columns to keep provenance.
+  coarser <- upscale_grid(filled, to = 0.5, min_coverage = 0)
+
+  expect_true("CHL_source" %in% names(coarser))
+  expect_true(all(coarser$CHL_source %in% c("satellite", "model")))
+  # Carried as a category, not averaged into something that is neither.
+  expect_false(any(is.na(coarser$CHL_source) & !is.na(coarser$CHL)))
+})