[BUG] Error in the simulation outlier step #11

Activity

1. jgarces02

   added

   bugSomething isn't workingSomething isn't working

   on Aug 31, 2026
2. Arvindiyer

   self-assigned this

   on Sep 1, 2026
3. Arvindiyer

   changed the title [-][BUG][/-] [+][BUG] Error in the simulation outlier step[/+] on Sep 1, 2026
4. Arvindiyer

   added the

   Bug

   issue type on Sep 1, 2026
5. Arvindiyer commented on Sep 1, 2026

   

   Arvindiyer

   on Sep 1, 2026

   Collaborator

   More actions

   @jgarces02

   Thanks for the detailed report — the full str() dump plus the exact error and warning text made this easy to track down. I reproduced it with a synthetic dataset shaped like yours, and it turns out there are two separate bugs bundled together here:

   1. The crash itself comes from sample.class being a factor (Factor w/ 2 levels "BM", "PCT"). Internally, get.blocks() builds per-block sample lists using which(sample.class == x), and R silently drops the names attribute when you do this comparison on a named factor (it keeps the names fine for a named character vector — that inconsistency is the actual bug). Once the names are gone, the code that slices the mutation/weight matrices by sample name ends up indexing with NULL, which produces empty matrices a few steps downstream, and that's what finally surfaces as replacement has length zero in retrieveOutliers(). I confirmed this by running your workflow on synthetic data with sample.class as a factor (crashes identically) vs. as a character vector (runs to completion) with nothing else changed.

   2. Your two warnings are a separate issue: it's a row-count mismatch. Nonsense/Missense in your tmb list only include samples with ≥1 mutation of that type (775 and 75 rows respectively, out of 884 samples total), but the code that builds the total TMB per sample just does plain vector addition assuming every tmb table already has one row per sample in matching order. When the lengths don't match, R recycles instead of erroring, so it adds positionally rather than matching by sample. That's the "longer object length is not a multiple of shorter object length" warning. This one doesn't crash, but it does silently produce incorrect per-sample TMB totals and sample weights, so it's worth fixing even though it's not what threw your error.

   Workaround for now, no package changes needed on your end — just two small tweaks to m_selectsim before calling selectX():

   # 1. Convert sample.class from factor to character, keeping the sample names
   m_selectsim$sample.class <- setNames(as.character(m_selectsim$sample.class),
                                         names(m_selectsim$sample.class))
   
   # 2. Give each tmb table one row per sample (0 where a sample has none of
   #    that mutation type), matching the column order of M
   all_samples <- colnames(m_selectsim$M[[1]])
   for (nm in names(m_selectsim$tmb)) {
     tb <- m_selectsim$tmb[[nm]]
     full <- data.frame(sample = all_samples,
                         mutation = 0L,
                         row.names = all_samples)
     full[as.character(tb$sample), "mutation"] <- tb$mutation
     m_selectsim$tmb[[nm]] <- full
   }
   
   aux <- SelectSim::selectX(
     M = m_selectsim, min.freq = 2,
     sample.class = m_selectsim$sample.class,
     alteration.class = m_selectsim$alteration.class)

   That should let you run selectX() on your data today without hitting either issue. Let me know if it doesn't behave as expected on your full dataset.

   I've tracked down the root cause in get.blocks() and new.AL.general() and I will be putting a patch together for both.

   I'm happy to help 👍🏽. Thanks again for the clear write-up; it made this a quick diagnosis.
6. Arvindiyer commented on Sep 1, 2026

   

   Arvindiyer

   on Sep 1, 2026

   Last edited by Arvindiyer

   Collaborator

   More actions

   TODO (tracking checklist to close it)

   ### Fix tracking
   
   - [X] Fix `get.blocks()` (`R/selectX_create.r`) so a `factor` `sample.class`/`alteration.class` doesn't silently lose sample names via `which(sample.class == x)` (coerce to `character`, or use a name-safe split).
   - [X] Fix `new.AL.general()` (`R/selectX_create.r`) so `al$tmb$total` is computed by matching each `tmb` table to samples **by name**, not by raw vector position/recycling.
   - [X] Add validation in `selectX()` / `new.AL.general()` that errors clearly (instead of warning + silently corrupting/crashing) when:
     - [X] `sample.class` or `alteration.class` is a factor
     - [X] any `tmb` table doesn't have exactly one row per sample matching `colnames(M[[i]])`
   - [X] Add regression tests covering both cases above (factor `sample.class`; partial/sparse `tmb` tables)
   - [X] Update docs (`@param` in `selectX_run.R`) to state the required shape for `sample.class`, `alteration.class`, and `tmb`
   - [X] Verify fix against the reporter's original reproduction (factor `sample.class` + sparse `tmb`) — should run cleanly or fail with a clear error
7. jgarces02 commented on Sep 1, 2026

   

   jgarces02

   on Sep 1, 2026

   Author

   More actions

   Nice. All checked the factor type in all tables except in that variable 😓. Now it works perfectly.
   And good catch with the different sizes, I didn't pay too much attention while the error was happening.
   Thanks a lot for your help, @Arvindiyer!

   (I won't close it as I see you're still doing things in regard this issue)

   👍React with 👍1Reacted by Arvind
8. Arvindiyer commented on Sep 1, 2026

   

   Arvindiyer

   on Sep 1, 2026

   Collaborator

   More actions

   @jgarces02 Thanks again for bringing this to my attention. I’ll work on it and fix the bug. Glad to hear the suggested workaround worked well 👍🏽😊

   No need to close the issue on your end—I’ll take care of that once the fix is in. And if anything else comes up, feel free to open another issue anytime. Happy to help!
9. Arvindiyer

   added a commit that references this issue on Sep 10, 2026

   Fix crash from factor sample.class and sparse tmb tables (#11)

   ...

   0fa321e