Skip to content

fix(profMat): correct .insertColumn boundary handling for empty spectra (fixes #366) - #840

Open
stanstrup wants to merge 1 commit into
sneumann:develfrom
stanstrup:fix/insertcolumn-empty-spectra
Open

stanstrup wants to merge 1 commit into
sneumann:develfrom
stanstrup:fix/insertcolumn-empty-spectra

Conversation

@stanstrup

@stanstrup stanstrup commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Fix: profMat() errors (subscript out of bounds) when a file has trailing / adjacent empty spectra

AI-assisted contribution. This report and the accompanying code fix were
prepared with the assistance of Claude Code. The reproducible example and every
quoted output were produced by actually running the code against the xcms
source; the change is covered by a regression test.

Summary

.insertColumn() (R/functions-utils.R) is used by the profMat() methods to
re-insert zero columns for empty spectra so that the returned profile matrix
has one column per spectrum (ncol(profMat) == length(rtime); the fix for
issue #312). The helper mis-handles two boundary cases:

  1. Insert position past the current last column (pos > ncol(x)) — happens
    whenever the empty spectrum is the last spectrum, or a run of empty
    spectra sits at the end. The else branch evaluates x[, pos[i]:ncol(x)]
    with pos[i] == ncol(x) + 1, i.e. x[, (ncol+1):ncol], which is out of
    bounds → error.

  2. Insert position exactly at the last column (pos == ncol(x)) — the code
    takes a special branch that appends the column at the end
    (cbind(x, val)) instead of inserting it at pos. The new (empty) column
    therefore lands one position too far to the right → silent column
    misalignment
    between the profile matrix and the retention times.

Empty spectra are common (scans with no peaks in the requested m/z range, after
centroiding, or after filterMz()), so profMat() can crash on ordinary data.

Related GitHub issues

Reproducible example

End-to-end crash of the public profMat() method on faahKO, restricting to an
m/z window that leaves several spectra — including the last one — empty:

library(xcms)
library(MSnbase)
library(faahKO)
library(BiocParallel)
register(SerialParam())

fl  <- dir(system.file("cdf", package = "faahKO"), recursive = TRUE,
           full.names = TRUE)[1]                       # ko15.CDF
raw <- readMSData(fl, mode = "onDisk")

sub <- filterMz(raw, mz = c(599, 600))
sum(lengths(mz(sub)) == 0)                 #> 189 empty spectra
lengths(mz(sub))[length(mz(sub))] == 0     #> TRUE   (last spectrum is empty)

profMat(sub, step = 1)
#> Error in x[, pos[i]:ncol(x)] : subscript out of bounds

Both failure modes can also be reproduced directly on the helper (current
source):

## trailing empty spectrum -> pos == ncol + 1 -> hard error
xcms:::.insertColumn(matrix(c(11, 13), nrow = 1), c(2, 4), 0)
#> Error in x[, pos[i]:ncol(x)] : subscript out of bounds

## pos == ncol -> column silently appended at the end instead of inserted
xcms:::.insertColumn(matrix(c(1, 2, 3), nrow = 1), 3, 0)
#>      [,1] [,2] [,3] [,4]
#> [1,]    1    2    3    0     # WRONG: want 1 2 0 3

With this fix, profMat(sub, step = 1) returns a matrix with one column per
spectrum (1278), and the two helper calls above return 11 0 13 0 and
1 2 0 3 respectively.

…rash

.insertColumn() mishandled two boundary cases used when re-inserting zero
columns for empty spectra in profMat():

- pos == ncol(x) took an "append at end" branch, placing the new column
  one position too far right -> silent misalignment of profile-matrix
  columns with retention times.
- a trailing empty spectrum yields pos == ncol(x) + 1, and the interior
  branch evaluated x[, pos:ncol(x)] out of bounds -> "subscript out of
  bounds" error.

The second case is a hard crash in the public profMat() method (and thus
adjustRtime(ObiwarpParam())) on ordinary data with empty trailing/adjacent
spectra, e.g. after filterMz() (reported in sneumann#366, worked around with
filterEmptySpectra()).

Rewrite the branches: pos == 1 prepend, pos > ncol(x) append, otherwise
insert *at* pos (with drop = FALSE for single-row robustness). Extend the
unit tests with the boundary cases.

Fixes sneumann#366

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NfgjUvLj79rbf3J4xX2wr9
@jorainer jorainer added the AI label Aug 7, 2026
@stanstrup

Copy link
Copy Markdown
Contributor Author

An example with adjustRtime instead of directly profMat:

library(xcms)
library(MsExperiment)
library(Spectra)
library(faahKO)

fls <- dir(system.file("cdf", package = "faahKO"), recursive = TRUE, full.names = TRUE)[1:2]

mse <- readMsExperiment(spectraFiles = fls, sampleData = data.frame(sample_name = basename(fls)))

# to create some empty spectra
spectra(mse) <- filterIntensity(spectra(mse), intensity = c(1e4, Inf))

xmse <- findChromPeaks(mse, CentWaveParam(noise = 10000, snthresh = 40, prefilter = c(3, 10000)))

adjustRtime(xmse, ObiwarpParam(binSize = 1))

--> Error in x[, pos[i]:ncol(x)] : subscript out of bounds

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

adjustRtime subscript out of bounds error

2 participants