Fixed control flow bugs : three bugs in how the ML pipeline finds and validates its input files

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) {

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"
+  )
+})