Reimplement plotTIS()/plotRIC() on the Set classes; add single-sample methods - #54
Merged
Merged
Conversation
…atogramSet plotTIS() and plotRIC() had near-identical, duplicated code to melt a matrix into long format and build a ggplot with SampleID coloring. Now both build a GCIMSSpectrumSet/GCIMSChromatogramSet from the (possibly sample-subsetted) matrix and delegate the actual plotting to plot.GCIMSSpectrumSet/plot.GCIMSChromatogramSet, removing the duplication. This also lets both gain a color_by parameter (defaulting to "SampleID", preserving prior behavior) to color by any pData() column, matching the annotation-aware plotting GCIMSChromatogramSet/ GCIMSSpectrumSet were built for. getTIS()/getRIC() themselves are untouched: they keep returning interpolated matrices via the joint per-sample extraction (extract_RIC_and_TIS), since that's still the right representation for a matrix API and is relied on directly by align-GCIMSDataset.R. Updated the existing plotTIS/plotRIC tests: plot.*Set() attaches data at the top-level ggplot object (ggplot(df) + geom_line(mapping = ...)), not per-layer like the old geom_line(data = ...) did, so tests now read p$data instead of p$layers[[1]]$data. Added a test for the new color_by parameter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
plotTIS/plotRIC previously only had GCIMSDataset methods, unlike every other TIS/RIC-adjacent generic (getChromatogram, getSpectrum, smooth, baseline, findPeaks), which work on both GCIMSSample and GCIMSDataset. Add the GCIMSSample methods, mirroring the dataset-level ones: wrap the existing getTIS()/getRIC() output in a GCIMSSpectrum/GCIMSChromatogram (populated the same way getSpectrum()/getChromatogram() do) and delegate to their existing plot() methods. RIC is extracted from a single drift-time row (the RIP position), not aggregated across a range, so its chromatogram's drift_time_idx/ drift_time_ms are set to that one accurate value (via dt_rt_range_normalization()'s existing single-index support) rather than a misleading range or NA. That RIP-position computation (which.max(rowSums(intensity(object)))) was previously duplicated in both getRIC() and alignDt() (the latter reaching it via getTIS() instead). Factored it into one unexported rip_position() helper used by both, plus the new plotRIC(), so there's a single definition instead of three. Simplified the vignette's two hand-rolled single-sample TIS/RIC plots (manual data.frame + ggplot, including a vestigial no-op dplyr::filter) into plotTIS()/plotRIC() calls now that they exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
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.
Summary
plotTIS()/plotRIC()forGCIMSDatasetare reimplemented on top ofGCIMSSpectrumSet/GCIMSChromatogramSet(added in Add GCIMSDataset methods for getChromatogram()/getSpectrum() #53), removing the duplicatedreshape2::melt()+ggplot()code between the two functions.getTIS()/getRIC()still return matrices unchanged (used internally byalign()and by any external callers), and the joint per-sample RIC+TIS extraction optimization is untouched.color_byparameter (defaults to"SampleID", preserving current behavior) that lets you color by anypData()column, not justSampleID.plotTIS()/plotRIC()now also haveGCIMSSamplemethods, closing a gap: every other TIS/RIC-adjacent generic (getChromatogram,getSpectrum,smooth,baseline,findPeaks) already worked on bothGCIMSSampleandGCIMSDataset, but these two didn't. RIC's chromatogram carries an accurate single-value drift time (the RIP position), not a misleading range orNA.which.max(rowSums(intensity(object)))) was independently duplicated ingetRIC()andalignDt()(the latter viagetTIS()). Factored into one unexportedrip_position()helper used by both, plus the newplotRIC().data.frame()+ggplot(), including a vestigial no-opdplyr::filter()) are now one-lineplotTIS()/plotRIC()calls.Test plan
testthatsuite passes, including new tests forplotTIS/plotRICon bothGCIMSSampleandGCIMSDataset(default behavior, range restriction,color_by, empty-set error handling)align-GCIMSDataset.R/align-GCIMSSample.Rtests (the actual consumers ofgetRIC/getTIS/alignDt) pass unchanged, confirming the matrix-returning contract and RIP-position refactor didn't change numerical behaviorrcmdcheckagainst the pinnedroxygen2version — no new WARNINGs; the two vignette-build WARNINGs seen locally are an artifact of skipping vignette rebuilding to save time and don't occur in CI's actual build stepGenerated by Claude Code