From d85b6aeac6ebc7f4f5fbec4c9e1bc89a2ecae577 Mon Sep 17 00:00:00 2001 From: Emily Boyer Date: Wed, 12 Aug 2026 17:21:57 -0600 Subject: [PATCH 1/2] Fixed control flow bugs. --- R/run_ML.R | 25 ++++++++++--- tests/testthat/test-run-ml-models.R | 58 +++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 tests/testthat/test-run-ml-models.R diff --git a/R/run_ML.R b/R/run_ML.R index bb9e534..e743463 100644 --- a/R/run_ML.R +++ b/R/run_ML.R @@ -201,9 +201,17 @@ createMLinputList <- function(path, path <- normalizePath(path) -# if (isTRUE(LOO) && (is.null(stratify_by) || !(stratify_by %in% c("year", "country")))) { - # stop("For Leave-One-Out (LOO) models, stratify_by must be 'year' or 'country'.") - # } + # Leave-one-drug-out cross-testing (LOO + cross_test) is a separate mode + # that doesn't stratify by year/country - see the `cross_test && LOO` + # branch below. Stratified LOO (leave-one-year/country-out) always needs + # stratify_by, whether or not it's also cross-tested. + if (isTRUE(LOO) && !isTRUE(cross_test) && + (is.null(stratify_by) || !(stratify_by %in% c("year", "country")))) { + stop( + "For Leave-One-Out (LOO) models without cross-testing, ", + "stratify_by must be 'year' or 'country'." + ) + } if (isTRUE(MDR) && (!is.null(stratify_by) || LOO || cross_test)) { stop("MDR can only run when stratify_by = NULL, LOO = FALSE, cross_test = FALSE.") @@ -579,7 +587,9 @@ parsed_drugs <- parsed |> out_top = paths$ML_top_features, out_models = paths$ML_models, out_pred = paths$ML_prediction - ) + ) + + return(out) } # LOO requires special directory structure resolution test_path <- file.path(path, stringr::str_remove(basename(paths$matrix_path), "^LOO_")) @@ -1002,7 +1012,12 @@ runMLmodels <- function(path, MDR = FALSE, cross_test = cross_test ) - + + if (nrow(files) == 0) { + message("No files found to process. Exiting.") + return(invisible(NULL)) + } + .findNonRanPrefixes <- function(files, seed, shuffle_labels = FALSE) { diff --git a/tests/testthat/test-run-ml-models.R b/tests/testthat/test-run-ml-models.R new file mode 100644 index 0000000..bfe0080 --- /dev/null +++ b/tests/testthat/test-run-ml-models.R @@ -0,0 +1,58 @@ +# Regression tests for control-flow bugs in +# createMLinputList()/runMLmodels(). + +test_that("runMLmodels() exits cleanly instead of crashing on no files", { + tmp <- withr::local_tempdir() + # No matrix parquet files are created, so createMLinputList() returns an + # empty tibble. runMLmodels() must message and return early instead of + # trying to use the empty result (which previously crashed with + # "a character vector argument expected"). + result <- NULL + expect_message( + result <- runMLmodels( + path = tmp, stratify_by = NULL, LOO = FALSE, cross_test = FALSE + ), + "No files found" + ) + expect_null(result) +}) + +test_that("createMLinputList() rejects LOO w/o year/country (no cross-test)", { + tmp <- withr::local_tempdir() + expect_error( + createMLinputList(tmp, LOO = TRUE, cross_test = FALSE, stratify_by = NULL), + "stratify_by must be" + ) +}) + +test_that("createMLinputList() allows LOO+cross-test with stratify_by = NULL", { + # LOO + cross_test with stratify_by = NULL is a distinct mode + # (leave-one-drug-out cross-testing, "Case A" in the cross_test && LOO + # branch) and is intentionally exempt from the year/country requirement + # above. + tmp <- withr::local_tempdir() + paths <- createMLResultDir( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE + ) + + # A single matrix filename that satisfies both the general filename parser + # and the LOO-specific parser used inside Case A. File content is + # irrelevant - createMLinputList() only inspects filenames. + file.create(file.path( + paths$matrix_path, + "Csp_drug_leaveout_leaveout_genes_binary_sparse.parquet" + )) + + out <- createMLinputList( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE + ) + + # Before the return() fix, Case A built the right result but never + # returned it, so execution fell through into the stratify_by != NULL + # branch, which self-joins the single file against itself and always + # filters it out (ref_file != test_file), silently returning 0 rows. + expect_equal(nrow(out), 1) + expect_true(grepl("_drug_leaveout_", out$output_prefix)) +}) From 6c7e7826618ebc4755d037a3d27a1a31e9844e6e Mon Sep 17 00:00:00 2001 From: Alexander McKim Date: Thu, 27 Aug 2026 15:34:17 -0600 Subject: [PATCH 2/2] updating tests and correctly finding LOOD matrices --- R/run_ML.R | 50 +++++++++++--- tests/testthat/test-run-ml-models.R | 101 +++++++++++++++++++++------- 2 files changed, 118 insertions(+), 33 deletions(-) diff --git a/R/run_ML.R b/R/run_ML.R index e743463..5ecf4ba 100644 --- a/R/run_ML.R +++ b/R/run_ML.R @@ -112,9 +112,17 @@ createMLResultDir <- function(path, ) } + # There is no stratify_by = "drug", so leave-one-drug-out arrives as NULL + # and `suffix` is "", making the path "LOO_matrix", which nothing writes. + # Those matrices live in LOO_matrix_drug/. Result directories keep the + # plain `suffix`, giving LOO_ML_performance/ not LOO_ML_drug_performance/. + # TODO: retire this by adding "drug" to the switch above, which also makes + # LOO with stratify_by = NULL an error again. + matrix_suffix <- if (isTRUE(LOO) && identical(suffix, "")) "_drug" else suffix + # Build paths paths <- list( - matrix_path = file.path(path, paste0(half_prefix, "matrix", suffix)), + matrix_path = file.path(path, paste0(half_prefix, "matrix", matrix_suffix)), ML_performance = file.path(path, paste0(full_prefix, "ML", suffix, "_performance")), ML_top_features = file.path(path, paste0(full_prefix, "ML", suffix, "_top_features")), ML_models = file.path(path, paste0(full_prefix, "ML", suffix, "_models")), @@ -201,15 +209,14 @@ createMLinputList <- function(path, path <- normalizePath(path) - # Leave-one-drug-out cross-testing (LOO + cross_test) is a separate mode - # that doesn't stratify by year/country - see the `cross_test && LOO` - # branch below. Stratified LOO (leave-one-year/country-out) always needs - # stratify_by, whether or not it's also cross-tested. - if (isTRUE(LOO) && !isTRUE(cross_test) && - (is.null(stratify_by) || !(stratify_by %in% c("year", "country")))) { + # LOO has three kinds: by year, by country, and by drug. Only the first two + # have a stratify_by value, so leave-one-drug-out arrives as NULL. Validate + # the value when one is given rather than requiring one. + if (isTRUE(LOO) && !is.null(stratify_by) && + !(stratify_by %in% c("year", "country"))) { stop( - "For Leave-One-Out (LOO) models without cross-testing, ", - "stratify_by must be 'year' or 'country'." + "For Leave-One-Out (LOO) models, `stratify_by` must be NULL ", + "(leave-one-drug-out), 'year', or 'country'." ) } @@ -511,7 +518,30 @@ createMLinputList <- function(path, # ============================ } else if (cross_test && LOO) { if(is.null(stratify_by)) { - # Case A: stratify_by = NULL, pair across abx within same feature + prefix + # Leave-one-drug-out cross testing. NOT SUPPORTED YET: it needs its own + # test set, the LOO equivalent of cross_drug_test/, which + # generateMLInputs() does not produce. Without it the pairing below has + # nothing to join against and returns zero rows, so fail loudly. + # + # The pairing code is kept for when that lands. It needs three fixes: + # 1. These filenames carry a "leaveout" marker before the drug, e.g. + # Sfl_drug_leaveout_AMP_gene_binary_sparse.parquet. `parsed` reads + # drug_or_class as the token right after "drug", so it returns the + # marker "leaveout" instead of "AMP", and the join below + # (ref_drug == test_drug) never matches. + # 2. `loo_test` is hardcoded to LOO_matrix/, which nothing writes, so + # test_file is always empty. It must point at the new folder once + # that exists. Note the loo_test / parsed_loo_test naming assumes + # the LOO matrices are the test set, but they hold training data + # (see "## Training drugs" in .parquet2LOODrugMatrix()), which is + # what ref_file already reads them as. + # 3. This branch should key off stratify_by == "drug" once that is a + # real value, rather than treating NULL as "must mean drug". + stop( + "Leave-one-drug-out cross testing is not supported yet: ", + "generateMLInputs() does not produce a cross-drug test set for the ", + "leave-one-drug-out matrices." + ) paths$loo_test <- file.path(dirname(paths$matrix_path), "LOO_matrix/") loo_files_vec <- list.files( diff --git a/tests/testthat/test-run-ml-models.R b/tests/testthat/test-run-ml-models.R index bfe0080..b7851b2 100644 --- a/tests/testthat/test-run-ml-models.R +++ b/tests/testthat/test-run-ml-models.R @@ -17,42 +17,97 @@ test_that("runMLmodels() exits cleanly instead of crashing on no files", { expect_null(result) }) -test_that("createMLinputList() rejects LOO w/o year/country (no cross-test)", { +# `stratify_by = NULL` means "no stratification" when LOO is FALSE, and "leave +# one drug out" when LOO is TRUE. There is no stratify_by = "drug", so the drug +# case has to be spelled NULL. Every caller has to know that rule, and the ones +# that got it wrong are what the tests below cover. + +test_that("createMLResultDir() maps every LOO/stratify_by pair to a real dir", { + tmp <- withr::local_tempdir() + matrix_dir <- function(loo, strat) { + basename(createMLResultDir( + tmp, + stratify_by = strat, LOO = loo, cross_test = FALSE, MDR = FALSE + )$matrix_path) + } + + expect_equal(matrix_dir(FALSE, NULL), "matrix") + expect_equal(matrix_dir(FALSE, "year"), "matrix_year") + expect_equal(matrix_dir(FALSE, "country"), "matrix_country") + expect_equal(matrix_dir(TRUE, NULL), "LOO_matrix_drug") + expect_equal(matrix_dir(TRUE, "year"), "LOO_matrix_year") + expect_equal(matrix_dir(TRUE, "country"), "LOO_matrix_country") +}) + +test_that("createMLResultDir() keeps LOO result dirs unsuffixed", { + # Only matrix_path takes "_drug". Drug LOO results are documented as + # LOO_ML_performance/ and LOO_ML_top_features/, unlike their year/country + # siblings, so they must not become LOO_ML_drug_*. + tmp <- withr::local_tempdir() + paths <- createMLResultDir( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = FALSE, MDR = FALSE + ) + expect_equal(basename(paths$ML_performance), "LOO_ML_performance") + expect_equal(basename(paths$ML_top_features), "LOO_ML_top_features") +}) + +test_that("createMLinputList() accepts leave-one-drug-out and finds its files", { + # This used to hard-error: the LOO check demanded stratify_by be "year" or + # "country", which rejected the drug case outright. + tmp <- withr::local_tempdir() + paths <- createMLResultDir( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = FALSE, MDR = FALSE + ) + for (drug in c("AMP", "CIP")) { + file.create(file.path( + paths$matrix_path, + sprintf("Sfl_drug_leaveout_%s_gene_binary_sparse.parquet", drug) + )) + } + + out <- createMLinputList( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = FALSE, MDR = FALSE + ) + + expect_equal(nrow(out), 2) + # One model per left-out drug, named from the file so the two do not collide. + expect_setequal( + out$output_prefix, + c("Sfl_drug_leaveout_AMP_gene_binary", "Sfl_drug_leaveout_CIP_gene_binary") + ) + expect_true(all(grepl("LOO_matrix_drug", out$matrix_path))) +}) + +test_that("createMLinputList() still rejects a bad stratify_by under LOO", { + # NULL is valid (leave-one-drug-out), but a typo must still be caught. tmp <- withr::local_tempdir() expect_error( - createMLinputList(tmp, LOO = TRUE, cross_test = FALSE, stratify_by = NULL), - "stratify_by must be" + createMLinputList(tmp, LOO = TRUE, cross_test = FALSE, stratify_by = "bananas"), + "stratify_by" ) }) -test_that("createMLinputList() allows LOO+cross-test with stratify_by = NULL", { - # LOO + cross_test with stratify_by = NULL is a distinct mode - # (leave-one-drug-out cross-testing, "Case A" in the cross_test && LOO - # branch) and is intentionally exempt from the year/country requirement - # above. +test_that("leave-one-drug-out cross testing fails loudly, not silently", { + # This mode needs a test set generateMLInputs() does not produce yet, so it + # must error rather than quietly return zero rows. tmp <- withr::local_tempdir() paths <- createMLResultDir( tmp, stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE ) - - # A single matrix filename that satisfies both the general filename parser - # and the LOO-specific parser used inside Case A. File content is - # irrelevant - createMLinputList() only inspects filenames. file.create(file.path( paths$matrix_path, - "Csp_drug_leaveout_leaveout_genes_binary_sparse.parquet" + "Sfl_drug_leaveout_AMP_gene_binary_sparse.parquet" )) - out <- createMLinputList( - tmp, - stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE + expect_error( + createMLinputList( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE + ), + "not supported yet" ) - - # Before the return() fix, Case A built the right result but never - # returned it, so execution fell through into the stratify_by != NULL - # branch, which self-joins the single file against itself and always - # filters it out (ref_file != test_file), silently returning 0 rows. - expect_equal(nrow(out), 1) - expect_true(grepl("_drug_leaveout_", out$output_prefix)) })