diff --git a/R/run_ML.R b/R/run_ML.R index bb9e534..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,9 +209,16 @@ 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'.") - # } + # 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, `stratify_by` must be NULL ", + "(leave-one-drug-out), '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.") @@ -503,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( @@ -579,7 +617,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 +1042,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..b7851b2 --- /dev/null +++ b/tests/testthat/test-run-ml-models.R @@ -0,0 +1,113 @@ +# 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) +}) + +# `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 = "bananas"), + "stratify_by" + ) +}) + +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 + ) + file.create(file.path( + paths$matrix_path, + "Sfl_drug_leaveout_AMP_gene_binary_sparse.parquet" + )) + + expect_error( + createMLinputList( + tmp, + stratify_by = NULL, LOO = TRUE, cross_test = TRUE, MDR = FALSE + ), + "not supported yet" + ) +})