Add GCIMSDataset methods for getChromatogram()/getSpectrum() - #53
Merged
Conversation
getChromatogram()/getSpectrum() are now S4 generics (previously plain functions working only on GCIMSSample). The new GCIMSDataset methods extract one chromatogram/spectrum per sample and wrap them in a new GCIMSChromatogramSet/GCIMSSpectrumSet object, together with a copy of pData(dataset) so plots can use the dataset's annotations. Each sample keeps its own native retention/drift time axis inside the Set (no interpolation onto a shared grid), and the new plot() methods combine them by row-binding rather than requiring a common axis, so mismatched sample axes are never silently distorted. plot(..., color_by = <any pData column>) lets you color by group instead of just SampleID. The vignette now demonstrates getChromatogram(dataset, ...)/ getSpectrum(dataset, ...) right after smoothing the whole dataset, as a simpler alternative to manually picking one sample to visualize. Regenerated NAMESPACE/man with roxygen2 7.3.1 (matching RoxygenNote), which also picked up previously stale docs for DelayedDatasetBase's dropSolePendingOp() method (added in an earlier PR, never regenerated). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
…uction GCIMSChromatogramSet/GCIMSSpectrumSet stored the sample identity in two places (the names of the chromatograms/spectra list, and pData$SampleID) with nothing enforcing they referred to the same set of samples. A mismatched pData built this way constructed silently, and plot()'s left_join(by = "SampleID") would then produce misleading NA/mismatched annotations instead of failing loudly. initialize() now aborts with a clear message when pData is missing a SampleID column, or its SampleID values don't match the sample names as a set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
sampleNames() previously always returned names(chromatograms)/ names(spectra), ignoring pData entirely. Now, when pData is present, it is treated as the source of truth for sample identity and order (mirrors GCIMSDataset, where sampleNames<- keeps pData$SampleID in sync). This also exposed a latent bug in plot(): it zipped x@chromatograms/x@spectra together with sample_names positionally via purrr::map2(), which was only correct because sampleNames() previously always matched the underlying list's own order. Switched both plot() methods to look up each sample by name (x[[sample_id]]) instead, so they stay correct regardless of how pData's row order relates to the list's order. Added regression tests for both the reordering and the plot() lookup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
Replace the previous fix (sampleNames() preferring pData, plot() doing name-based lookups) with what was actually being asked for: enforce the invariant structurally, at the points where it can be violated. - initialize() now reorders pData's rows to match the chromatograms/ spectra list order right after validating they refer to the same samples, so the two are positionally aligned from construction onward (matched by SampleID, not blindly reordered, so annotations stay attached to the right sample). - Added a sampleNames<- replacement method for both classes, mirroring GCIMSDataset's own setter: renaming updates both the list names and pData$SampleID together, so they can never drift apart afterwards. sampleNames() goes back to simply returning names(chromatograms)/ names(spectra), since alignment is now guaranteed rather than special-cased in the getter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
…xist CI (PR #53) failed R CMD check with: checking Rd \usage sections ... WARNING Undocumented arguments in Rd file 'GCIMSChromatogramSet-class.Rd': '...' Documented arguments not in \usage in Rd file 'GCIMSChromatogramSet-class.Rd': 'y' (same for GCIMSSpectrumSet-class.Rd) plot()'s roxygen block was copy-pasted from plot-GCIMSChromatogram.R, which documents a `y` param inherited from the base plot generic's signature. But that method is function(x, ...), so roxygen renders its \usage using the generic's own (x, y, ...) signature, whereas my method is function(x, color_by = "SampleID", ...) -- an actual extra parameter, which makes roxygen use the method's own formals verbatim, with no y in it. Replaced the stale `@param y` with `@param ... Ignored`, which now matches the generated \S4method{plot}{...}(x, color_by = "SampleID", ...) usage exactly. Verified locally with rcmdcheck: `checking Rd \usage sections` is back to a NOTE (the one pre-existing, unrelated findPeaksImpl.Rd issue that already exists on master), not a WARNING. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getChromatogram()/getSpectrum() are now S4 generics (previously plain
functions working only on GCIMSSample). The new GCIMSDataset methods
extract one chromatogram/spectrum per sample and wrap them in a new
GCIMSChromatogramSet/GCIMSSpectrumSet object, together with a copy of
pData(dataset) so plots can use the dataset's annotations.
Each sample keeps its own native retention/drift time axis inside the
Set (no interpolation onto a shared grid), and the new plot() methods
combine them by row-binding rather than requiring a common axis, so
mismatched sample axes are never silently distorted. plot(..., color_by
= ) lets you color by group instead of just SampleID.
The vignette now demonstrates getChromatogram(dataset, ...)/
getSpectrum(dataset, ...) right after smoothing the whole dataset, as a
simpler alternative to manually picking one sample to visualize.
Regenerated NAMESPACE/man with roxygen2 7.3.1 (matching RoxygenNote),
which also picked up previously stale docs for DelayedDatasetBase's
dropSolePendingOp() method (added in an earlier PR, never regenerated).
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56