Fix as.sir() data.frame: preserve already-<sir> columns, exclude metadata

Issue #278: two related bugs in the column-detection / type-assignment pipeline.

Bug 1 – already-<sir> columns deleted on re-run
  Line 886 excluded already-sir columns from the type assignment (they
  stayed type "") causing the result loop to do x[,col] <- NULL, deleting
  them.  Fix: drop the !is.sir() guard so all untyped columns fall through
  to type "sir" and are re-processed correctly.

Bug 2 – metadata columns treated as antibiotics
  as.ab("patient") -> OXY, as.ab("ward") -> PRU.  The column detector
  accepted any column whose name matched an antibiotic code, regardless of
  content.  Fix: for name-matched columns that do not already carry an AMR
  class, also verify content looks like AMR data (all_valid_mics, all-
  numeric, or any SIR-like string).  all_valid_disks() is intentionally
  avoided here because it strips letters from strings (as.disk("Pt_1")==1).

Also adds tools/benchmark_parallel.R: a standalone script that times
sequential vs parallel as.sir() across n=20/200/2000/20000 rows and
saves a ggplot2 PNG to tools/benchmark_parallel.png.

https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR

NEWS.md

@@ -35,6 +35,8 @@
 * Fixed SIR and MIC coercion of combined values, e.g. `as.sir("<= 0.002; S") ` or `as.mic("S; 0.002")` (#252)
 * Fixed translation of foreign languages in `sir_df()` (#272)
 * Fixed BRMO classification by including bacterial complexes (#275)
+* Fixed `as.sir()` for data frames silently deleting columns whose AB class was already `<sir>` when called a second time (re-running on already-converted data) (#278)
+* Fixed `as.sir()` for data frames incorrectly treating metadata columns (e.g. `patient`, `ward`) as antibiotic columns when their names coincidentally matched an antibiotic code; column content is now validated against AMR data patterns before inclusion
 
 ### Updates
 * Extensive `cli` integration for better message handling and clickable links in messages and warnings (#191, #265)

R/sir.R

@@ -852,7 +852,6 @@ as.sir.data.frame <- function(x,
   i <- 0
   ab_cols <- colnames(x)[vapply(FUN.VALUE = logical(1), x, function(y) {
     i <<- i + 1
-    check <- is.mic(y) | is.disk(y)
     ab <- colnames(x)[i]
     if (!is.null(col_mo) && ab == col_mo) {
       return(FALSE)
@@ -861,13 +860,30 @@ as.sir.data.frame <- function(x,
       return(FALSE)
     }
     if (length(sel) == 0 || (length(sel) > 0 && ab %in% sel)) {
+      # columns already carrying an AMR class are always included
+      y_bak <- x.bak[, ab, drop = TRUE]
+      if (is.mic(y_bak) || is.disk(y_bak) || is.sir(y_bak)) {
+        return(TRUE)
+      }
       ab_coerced <- suppressWarnings(as.ab(ab, info = FALSE))
       if (is.na(ab_coerced) || (length(sel) > 0 & !ab %in% sel)) {
         # not even a valid AB code
         return(FALSE)
-      } else {
-        return(TRUE)
       }
+      # Name matches an antibiotic; also verify column content resembles AMR
+      # data. This prevents false positives on metadata columns whose names
+      # happen to match a drug code (e.g. 'patient' -> OXY, 'ward' -> PRU).
+      # Note: all_valid_disks() is intentionally avoided here because it strips
+      # non-numeric characters (as.disk("Pt_1") == 1), accepting patient IDs.
+      y_char <- tryCatch(as.character(y), error = function(e) character(0))
+      y_valid <- y_char[!is.na(y_char) & nzchar(trimws(y_char))]
+      if (length(y_valid) == 0L) {
+        return(FALSE)
+      }
+      y_numeric <- suppressWarnings(as.numeric(y_valid))
+      all_valid_mics(y) ||
+        all(!is.na(y_numeric)) ||
+        any(y_valid %in% c("S", "SDD", "I", "R", "NI"))
     } else {
       return(FALSE)
     }
@@ -883,7 +899,7 @@ as.sir.data.frame <- function(x,
   types[vapply(FUN.VALUE = logical(1), x.bak[, ab_cols, drop = FALSE], is.mic)] <- "mic"
   types[types == "" & vapply(FUN.VALUE = logical(1), x[, ab_cols, drop = FALSE], all_valid_disks)] <- "disk"
   types[types == "" & vapply(FUN.VALUE = logical(1), x[, ab_cols, drop = FALSE], all_valid_mics)] <- "mic"
-  types[types == "" & !vapply(FUN.VALUE = logical(1), x.bak[, ab_cols, drop = FALSE], is.sir)] <- "sir"
+  types[types == ""] <- "sir"
   if (any(types %in% c("mic", "disk"), na.rm = TRUE)) {
     # now we need an mo column
     stop_if(is.null(col_mo), "{.arg col_mo} must be set")

tests/testthat/test-sir.R

@@ -406,6 +406,37 @@ test_that("test-sir.R", {
   expect_equal(out3, as.sir(c("NWT", "WT", "NWT")))
   expect_equal(out4, as.sir(c("NWT", "WT", "NWT")))
 
+  # Issue #278: re-running as.sir() on already-<sir> data must preserve columns
+  df_already_sir <- data.frame(
+    mo  = "B_ESCHR_COLI",
+    AMC = as.mic(c("1", "2", "4")),
+    GEN = sample(c("S", "I", "R"), 3, replace = TRUE),
+    stringsAsFactors = FALSE
+  )
+  first_pass  <- suppressMessages(as.sir(df_already_sir, col_mo = "mo", info = FALSE))
+  second_pass <- suppressMessages(as.sir(first_pass,    col_mo = "mo", info = FALSE))
+  expect_equal(ncol(first_pass), ncol(second_pass))
+  expect_true(is.sir(second_pass[["AMC"]]))
+  expect_true(is.sir(second_pass[["GEN"]]))
+  expect_identical(first_pass[["AMC"]], second_pass[["AMC"]])
+  expect_identical(first_pass[["GEN"]], second_pass[["GEN"]])
+
+  # Issue #278: metadata columns whose names coincidentally match antibiotic
+  # codes (e.g. 'patient' -> OXY, 'ward' -> PRU) must not be processed
+  df_meta <- data.frame(
+    mo      = "B_ESCHR_COLI",
+    patient = paste0("Pt_", 1:20),
+    ward    = rep(c("ICU", "Surgery", "Outpatient", "ED"), 5),
+    AMC     = as.mic(rep(c("1", "2", "4", "8"), 5)),
+    stringsAsFactors = FALSE
+  )
+  df_meta_sir <- suppressMessages(as.sir(df_meta, col_mo = "mo", info = FALSE))
+  expect_true("patient" %in% colnames(df_meta_sir))
+  expect_true("ward"    %in% colnames(df_meta_sir))
+  expect_false(is.sir(df_meta_sir[["patient"]]))
+  expect_false(is.sir(df_meta_sir[["ward"]]))
+  expect_true(is.sir(df_meta_sir[["AMC"]]))
+
   # Parallel computing ----------------------------------------------------
   # Tests must pass even when only 1 core is available; parallel = TRUE then
   # silently falls back to sequential, but results must still be identical.

tools/benchmark_parallel.R

@@ -0,0 +1,71 @@
+# Benchmark: sequential vs parallel as.sir() across data-set sizes
+#
+# Run from the repo root with:
+#   Rscript tools/benchmark_parallel.R
+# or from inside an R session:
+#   source("tools/benchmark_parallel.R")
+#
+# Requires ggplot2 for the output plot; uses devtools::load_all() so the
+# package does not need to be installed.
+
+devtools::load_all(".", quiet = TRUE)
+
+sizes <- c(20, 200, 2000, 20000)
+n_ab  <- 6   # number of antibiotic columns
+
+make_df <- function(n) {
+  set.seed(42)
+  mics <- lapply(seq_len(n_ab), function(j) {
+    as.mic(sample(c("0.25", "0.5", "1", "2", "4", "8", "16", "32"), n, TRUE))
+  })
+  names(mics) <- c("AMC", "GEN", "CIP", "TZP", "IPM", "MEM")
+  data.frame(mo = "B_ESCHR_COLI", mics, stringsAsFactors = FALSE)
+}
+
+results <- do.call(rbind, lapply(sizes, function(n) {
+  df <- make_df(n)
+
+  t_seq <- system.time(
+    suppressMessages(as.sir(df, col_mo = "mo", info = FALSE, parallel = FALSE))
+  )[["elapsed"]]
+
+  t_par <- system.time(
+    suppressMessages(as.sir(df, col_mo = "mo", info = FALSE, parallel = TRUE))
+  )[["elapsed"]]
+
+  message(sprintf("n = %6d  seq = %.3fs  par = %.3fs  speedup = %.1fx",
+                  n, t_seq, t_par, t_seq / t_par))
+
+  data.frame(n = n, mode = c("sequential", "parallel"),
+             seconds = c(t_seq, t_par))
+}))
+
+if (requireNamespace("ggplot2", quietly = TRUE)) {
+  p <- ggplot2::ggplot(results, ggplot2::aes(x = n, y = seconds,
+                                             colour = mode, group = mode)) +
+    ggplot2::geom_line(linewidth = 1) +
+    ggplot2::geom_point(size = 3) +
+    ggplot2::scale_x_log10(
+      breaks = sizes,
+      labels = format(sizes, big.mark = ",", scientific = FALSE)
+    ) +
+    ggplot2::scale_colour_manual(
+      values = c(sequential = "#E05C5C", parallel = "#2E86AB")
+    ) +
+    ggplot2::labs(
+      title    = "as.sir() throughput: sequential vs parallel",
+      subtitle = sprintf("%d antibiotic columns, E. coli, EUCAST 2025", n_ab),
+      x        = "Number of rows (log scale)",
+      y        = "Wall-clock time (seconds)",
+      colour   = NULL
+    ) +
+    ggplot2::theme_minimal(base_size = 13) +
+    ggplot2::theme(legend.position = "top")
+
+  out_file <- "tools/benchmark_parallel.png"
+  ggplot2::ggsave(out_file, p, width = 7, height = 5, dpi = 150)
+  message("Plot saved to ", out_file)
+} else {
+  message("Install ggplot2 to get a plot; raw results:")
+  print(results)
+}