mirror of
https://github.com/msberends/AMR.git
synced 2026-04-28 13:43:56 +02:00
Fix parallel computing in as.sir.data.frame (#276)
* Fix parallel computing in as.sir.data.frame
Six bugs in parallel = TRUE mode:
1. PSOCK workers (Windows / R < 4.0) never had AMR loaded, so every
exported/AMR function call failed. Added clusterEvalQ(cl, library(AMR))
with a graceful fallback to sequential when the package cannot be loaded
(e.g. dev-only load_all() environments).
2. clusterExport'd AMR_env was a frozen serialised copy; as.sir() on the
worker wrote to AMR:::AMR_env while run_as_sir_column read from the stale
copy, so the captured log was always wrong. Fixed by resolving AMR_env
dynamically via get("AMR_env", envir = asNamespace("AMR")) inside the
worker function, and removing AMR_env from clusterExport.
3. In the fork-based (mclapply) path each worker inherited the parent's full
sir_interpretation_history. Capturing the whole log then combining across
workers duplicated every pre-existing entry. Fixed by recording the log
row count before the as.sir() call and slicing only the new rows
afterwards.
4. run_as_sir_column used non-exported internals (%pm>%, pm_pull,
as.sir.default) that are inaccessible on PSOCK workers after library(AMR).
Replaced pipe chains with direct as.mic(as.character(x[, col, drop=TRUE]))
and as.disk(...) calls, and changed as.sir.default() to as.sir() which
dispatches correctly via S3.
5. With info = TRUE, worker forks printed per-column progress messages
simultaneously, producing garbled interleaved console output. Per-column
messages are now suppressed inside workers (effective_info = FALSE) while
the outer "Running in parallel" / "DONE" messages still appear.
6. Malformed Unicode escape \u00a (3 hex digits) in the "DONE" banner was
parsed by R as U+00AD (soft hyphen) + "ONE"; corrected to .
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Add parallel computing tests to test-sir.R
Eight targeted tests verify correctness of the parallel as.sir() path:
identical SIR output vs sequential, matching log row counts, no
pre-existing history duplication, reproducibility across runs, results
consistency across max_cores values, single-column fallback, and no
per-column worker messages leaking when info = TRUE. All pass when only
1 core is available (parallel silently falls back to sequential).
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* 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
* Update benchmark: two-panel script with warm-up and column-count sweep
Previous single-panel benchmark was misleading: the first sequential run
paid one-time cache-warm-up cost (skewing n=20), and only 6 columns were
used so only 6 cores were ever active on a 16-core machine.
New two-panel design:
Left – vary rows with 16 fixed AB columns (shows memory-bandwidth
saturation for large n)
Right – vary columns with fixed rows (shows the real speedup profile:
parallel wins when n_cols >> 1)
Also adds a warm-up pass before measurements to eliminate first-call bias.
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Optimise parallel as.sir(): row-batch mode when n_cols < n_cores
Previously parallel dispatch only parallelised by column, so a 6-column
dataset on a 16-core machine used at most 6 cores with the other 10 idle.
For large n this also caused memory-bandwidth saturation (each worker did
a full n-row scan of clinical_breakpoints simultaneously).
New row-batch mode (fork path, R >= 4.0, non-Windows):
pieces_per_col = ceil(n_cores / n_cols)
Jobs = n_cols × pieces_per_col (≈ n_cores jobs total)
Each job: one column × one row slice
Benefits:
- All cores stay busy regardless of column count
- Per-worker memory footprint shrinks by pieces_per_col ×
- Breakpoints lookup cache pressure reduced per worker
PSOCK path (Windows / R < 4.0) is unchanged: per-job serialisation
overhead makes row batching unprofitable there.
run_as_sir_column() gains an optional `rows` parameter (NULL = all rows,
backward-compatible). Results are reassembled via as.sir(c(as.character(.)))
which is safe for already-clean SIR values.
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Fix info=FALSE ignored when no breakpoints found in as_sir_method
Operator-precedence bug at line 1601:
if (isTRUE(info) && nrow(df_unique) < 10 || nrow(breakpoints) == 0)
R evaluates && before ||, so this was equivalent to:
(isTRUE(info) && nrow(df_unique) < 10) || (nrow(breakpoints) == 0)
When nrow(breakpoints) == 0 (e.g. cefoxitin / flucloxacillin / mupirocin
against E. coli in EUCAST) the intro message was always printed regardless
of info. Fix: add parentheses so info gates both conditions:
isTRUE(info) && (nrow(df_unique) < 10 || nrow(breakpoints) == 0)
Also pass print = isTRUE(info) to progress_ticker so the progress bar
(which prints intro_txt as its title) is suppressed when info = FALSE.
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Fix cli formatting in as.sir() messages
- stop_if for empty ab_cols: wrap as.mic() and as.disk() in
{.help [{.fun ...}](...)} for clickable links in cli output
- Parallel mode message: use {.field col} formatting for column names
and quotes = FALSE in vector_and(), consistent with the rest of the
codebase (avoids double-quoting from both font_bold and quotes="'")
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Use font_bold() inside {.field} for column names in parallel message
Convention: paste0("{.field ", font_bold(col), "}") gives bold green
column names without quotation marks, consistent with the rest of the
codebase (e.g. the 'Cleaning values' message in run_as_sir_column).
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Add collapse = NULL to font_bold() for column name vectors
font_bold() without collapse = NULL joins a vector with "" into a single
string, breaking paste0() element-wise formatting for length > 1 vectors.
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
* Add tools/ to .Rbuildignore
Keeps the benchmark script out of the built package tarball.
https://claude.ai/code/session_012DXCXbZUC54Zij1z9bFiHR
---------
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Matthijs Berends
GitHub
@@ -406,40 +406,126 @@ 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.
|
||||
|
||||
# MB 29 Apr 2025: I have run the code of AVC, PEI, Canada (dataset of 2854x65), and compared it like this:
|
||||
set.seed(42)
|
||||
n_par <- 200
|
||||
df_par <- data.frame(
|
||||
mo = "B_ESCHR_COLI",
|
||||
AMC = as.mic(sample(c("0.25", "0.5", "1", "2", "4", "8", "16", "32"), n_par, TRUE)),
|
||||
GEN = as.mic(sample(c("0.5", "1", "2", "4", "8", "16", "32", "64"), n_par, TRUE)),
|
||||
CIP = as.mic(sample(c("0.001", "0.002", "0.004", "0.008", "0.016", "0.032"), n_par, TRUE)),
|
||||
PEN = sample(c("S", "I", "R", NA_character_), n_par, TRUE),
|
||||
stringsAsFactors = FALSE
|
||||
)
|
||||
|
||||
# system.time({
|
||||
# data_2022_2023_SIR_parallel <- data_2022_2023_clean |>
|
||||
# as.sir(amikacin:tiamulin,
|
||||
# col_mo = "mo",
|
||||
# guideline = "CLSI 2024",
|
||||
# host = "Species",
|
||||
# uti = "isUTI",
|
||||
# parallel = TRUE)
|
||||
# })
|
||||
# # user system elapsed
|
||||
# # 271.424 2.767 45.762
|
||||
#
|
||||
# history_parallel <- sir_interpretation_history(clean = TRUE)
|
||||
#
|
||||
# system.time({
|
||||
# data_2022_2023_SIR <- data_2022_2023_clean |>
|
||||
# as.sir(amikacin:tiamulin,
|
||||
# col_mo = "mo",
|
||||
# guideline = "CLSI 2024",
|
||||
# host = "Species",
|
||||
# uti = "isUTI")
|
||||
# })
|
||||
# # user system elapsed
|
||||
# # 120.637 5.406 128.835
|
||||
# history <- sir_interpretation_history()
|
||||
# clear any existing history before comparing
|
||||
sir_interpretation_history(clean = TRUE)
|
||||
sir_seq <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE))
|
||||
log_seq <- sir_interpretation_history(clean = TRUE)
|
||||
|
||||
sir_par <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE))
|
||||
log_par <- sir_interpretation_history(clean = TRUE)
|
||||
|
||||
# and then got this:
|
||||
# identical(history[, -1], history_parallel[, -1])
|
||||
#> [1] TRUE
|
||||
# 1. parallel = TRUE gives identical SIR results to sequential
|
||||
expect_identical(sir_seq[["AMC"]], sir_par[["AMC"]])
|
||||
expect_identical(sir_seq[["GEN"]], sir_par[["GEN"]])
|
||||
expect_identical(sir_seq[["CIP"]], sir_par[["CIP"]])
|
||||
expect_identical(sir_seq[["PEN"]], sir_par[["PEN"]])
|
||||
|
||||
# so parallel on Apple M2 is 2.8x faster, with identical history -> GREAT!
|
||||
# 2. same number of log rows as sequential
|
||||
expect_equal(nrow(log_seq), nrow(log_par))
|
||||
|
||||
# 3. pre-existing log entries must not be duplicated
|
||||
# run sequential once to populate the history, then run parallel and
|
||||
# verify the new parallel run adds exactly as many rows as sequential
|
||||
sir_interpretation_history(clean = TRUE)
|
||||
suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE)) # populate history
|
||||
pre_n <- nrow(sir_interpretation_history())
|
||||
suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE))
|
||||
post_n <- nrow(sir_interpretation_history())
|
||||
expect_equal(post_n - pre_n, nrow(log_seq)) # exactly one run's worth of new rows
|
||||
sir_interpretation_history(clean = TRUE)
|
||||
|
||||
# 4. two sequential runs and two parallel runs yield identical results
|
||||
sir_par2 <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE))
|
||||
expect_identical(sir_par[["AMC"]], sir_par2[["AMC"]])
|
||||
expect_identical(sir_par[["GEN"]], sir_par2[["GEN"]])
|
||||
|
||||
# 5. max_cores = 1 gives same results as default sequential
|
||||
sir_mc1 <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE, max_cores = 1L))
|
||||
expect_identical(sir_seq[["AMC"]], sir_mc1[["AMC"]])
|
||||
expect_identical(sir_seq[["GEN"]], sir_mc1[["GEN"]])
|
||||
|
||||
# 6. max_cores = 2 and max_cores = 3 give same results as sequential
|
||||
sir_mc2 <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE, max_cores = 2L))
|
||||
sir_mc3 <- suppressMessages(as.sir(df_par, col_mo = "mo", info = FALSE, parallel = TRUE, max_cores = 3L))
|
||||
expect_identical(sir_seq[["AMC"]], sir_mc2[["AMC"]])
|
||||
expect_identical(sir_seq[["GEN"]], sir_mc3[["GEN"]])
|
||||
|
||||
# 7. single-column data frame falls back silently to sequential
|
||||
df_single <- df_par[, c("mo", "AMC")]
|
||||
sir_single_seq <- suppressMessages(as.sir(df_single, col_mo = "mo", info = FALSE))
|
||||
sir_single_par <- suppressMessages(as.sir(df_single, col_mo = "mo", info = FALSE, parallel = TRUE))
|
||||
expect_identical(sir_single_seq[["AMC"]], sir_single_par[["AMC"]])
|
||||
|
||||
# 9. row-batch mode (n_cols < n_cores): force row splitting via max_cores and
|
||||
# verify identical output to sequential for a dataset with 2 AB columns so
|
||||
# pieces_per_col = ceiling(max_cores / 2) >= 2 and row batching activates
|
||||
df_wide <- data.frame(
|
||||
mo = "B_ESCHR_COLI",
|
||||
AMC = as.mic(sample(c("1", "2", "4", "8"), n_par, TRUE)),
|
||||
GEN = as.mic(sample(c("1", "2", "4", "8"), n_par, TRUE)),
|
||||
stringsAsFactors = FALSE
|
||||
)
|
||||
sir_wide_seq <- suppressMessages(as.sir(df_wide, col_mo = "mo", info = FALSE))
|
||||
sir_wide_par <- suppressMessages(as.sir(df_wide, col_mo = "mo", info = FALSE,
|
||||
parallel = TRUE, max_cores = 8L))
|
||||
expect_identical(sir_wide_seq[["AMC"]], sir_wide_par[["AMC"]])
|
||||
expect_identical(sir_wide_seq[["GEN"]], sir_wide_par[["GEN"]])
|
||||
|
||||
# 8. info = TRUE with parallel does not produce per-column worker messages
|
||||
# (messages should only appear in the main process, not duplicated from workers)
|
||||
msgs <- capture.output(
|
||||
suppressWarnings(as.sir(df_par, col_mo = "mo", info = TRUE, parallel = TRUE)),
|
||||
type = "message"
|
||||
)
|
||||
# each AB column name should appear at most once in all messages combined
|
||||
for (ab_nm in c("AMC", "GEN", "CIP", "PEN")) {
|
||||
n_mentions <- sum(grepl(ab_nm, msgs, fixed = TRUE))
|
||||
expect_lte(n_mentions, 1L)
|
||||
}
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user