From 6ece73cb22965538ee2bc3c3c71032305439f1b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Apr 2026 21:30:21 +0000 Subject: [PATCH] Fix as.sir() data.frame: preserve already- columns, exclude metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #278: two related bugs in the column-detection / type-assignment pipeline. Bug 1 – already- 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 | 2 ++ R/sir.R | 24 ++++++++++--- tests/testthat/test-sir.R | 31 +++++++++++++++++ tools/benchmark_parallel.R | 71 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 tools/benchmark_parallel.R diff --git a/NEWS.md b/NEWS.md index e9b122393..ec7274c6b 100644 --- a/NEWS.md +++ b/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 `` 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) diff --git a/R/sir.R b/R/sir.R index c1c07b2c6..456f15fe0 100755 --- a/R/sir.R +++ b/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") diff --git a/tests/testthat/test-sir.R b/tests/testthat/test-sir.R index 31235ea45..68fbe467e 100644 --- a/tests/testthat/test-sir.R +++ b/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- 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. diff --git a/tools/benchmark_parallel.R b/tools/benchmark_parallel.R new file mode 100644 index 000000000..fa736a918 --- /dev/null +++ b/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) +}