Added chromatogramHeader test

[nilshoffmann]

First step to demonstrate issues with index access for chromatogramHeader vs chromatogram.

[jorainer]

Oh - so there is an issue with indexing? I hope you then also provide a PR with a fix :)

[nilshoffmann]

@jorainer @sneumann Please review and run the workflow. I have added unit tests to demonstrate the fixed indexing behavior between chromatogram and chromatogramHeader.

[jorainer]

Really great! thank you so much Nils!

quick question: there is not indexing issue with the getAllChromatogramsInfo(), right? All functions/methods in R for chromatogram extraction pass now R-style 1-based indices, right?

[jorainer]

to be on the (very) save side I would suggest to then call as.integer(chrom) below (i.e. getChromatogramHeaderInfo(as.integer(chrom)))

[jorainer]

could be that the Rcpp makes sure the correct type is passed, but I would rather play save...

[sneumann]

Done.

[nilshoffmann]

Yes, correct. The „All“ methods are already using a seq from 1:Nchrom so no need for fixing there. Am 05.02.2025 um 14:42 schrieb Johannes Rainer ***@***.***>: @jorainer approved this pull request. Really great! thank you so much Nils! quick question: there is not indexing issue with the getAllChromatogramsInfo(), right? All functions/methods in R for chromatogram extraction pass now R-style 1-based indices, right? In R/methods-mzRpwiz.R:

@@ -173,8 +173,7 @@ setMethod("chromatogram", "mzRpwiz",

chrom <- as.integer(chrom) if (min(chrom) < 1 | max(chrom) > n) stop("Index out of bound [", 1, ":", n, "].") - ## Update index to match original indices at the C-level - chrom <- chrom - 1L + # chromatogram index is adjusted in the backend function yes! agree. that's also what we do for the spectra data. In R/methods-mzRpwiz.R:

@@ -192,10 +191,11 @@ setMethod("chromatogramHeader", "mzRpwiz",

} else { stopifnot(is.numeric(chrom)) n <- nChrom(object) - chrom <- as.integer(chrom) - if (min(chrom) < 1 || max(chrom) > n) - stop("Index out of bound [", 1, ":", n, "]") - chrom <- chrom - 1L + cat("n=",n,"min(chrom)=",min(chrom)," max(chrom)=",max(chrom), "\n") can you please remove the debugging cat() calls here ;) In R/methods-mzRpwiz.R:

@@ -192,10 +191,11 @@ setMethod("chromatogramHeader", "mzRpwiz",

} else { stopifnot(is.numeric(chrom)) n <- nChrom(object) - chrom <- as.integer(chrom) to be on the (very) save side I would suggest to then call as.integer(chrom) below (i.e. getChromatogramHeaderInfo(as.integer(chrom))) In R/methods-mzRpwiz.R:

@@ -192,10 +191,11 @@ setMethod("chromatogramHeader", "mzRpwiz",

} else { stopifnot(is.numeric(chrom)) n <- nChrom(object) - chrom <- as.integer(chrom) could be that the Rcpp makes sure the correct type is passed, but I would rather play save... —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[sneumann]

Before merging, let me check that the cat() debug calls are removed in a later PR. Yours, Steffen
No, still there. Please remove. Yours, Steffen

[sneumann]

Done.

[sneumann]

Unsure whether I messed up merging, but I am getting a test failure:
checkIdentical(tic(x), chromatogram(x, 1L)) is FALSE, but instead
checkIdentical(tic(x), chromatogram(x, 2L)) is TRUE.
What I realised is that

mzR/R/methods-mzRpwiz.R

Line 153 in 30d1676

object@backend$getChromatogramsInfo(1L)

assumes the TIC is always the first chromatogram(). Is that guaranteed, quite common or just in our MRM file ?
Yours, Steffen

[jorainer]

AFAIK the TIC is always the first chromatogram in the chromatogram list entry of a mzML file - as far that is what I've seen so far. But Nils @nilshoffmann will definitely have more data files seen to give a better/more reliable answer.