perf(dataProcessPlots): Flat-layer QC boxplots for PDF and no print()…

[Rudhik1904]

Motivation and Context

dataProcessPlots() slows down as the number of MS runs grows, mainly in QCPlot. We profiled it on synthetic data (12–768 runs) and on a real 270-run, 4,715-protein experiment, and found two causes:

1. QCPlot to PDF scales linearly with runs. geom_boxplot builds a separate grob tree for every box inside ggplot_gtable(), at about 5 ms per box, with one box per run in every plot. The cost depends on the number of boxes, not the number of data points: on real data, a protein with 1,058× more rows was only 20% slower in this step. With 270 runs and 4,715 proteins, QCPlot alone was projected at about 2.5 hours.
2. Plotly mode (isPlotly = TRUE) renders every plot twice. Each plot was print()ed onto a graphics device and then rebuilt from scratch by ggplotly(). With no device open, R creates a stray Rplots.pdf in the working directory. For QCPlot this wasted rendering was most of the runtime. Separately, .plotQC called dev.off() even in plotly mode, which closed the user's open graphics device.

Solution:

• Plotly mode: skip print() and guard .plotQC's dev.off().
• PDF mode: compute the box statistics once and draw the QC boxplots as a few vectorized layers. ggplotly() does not pay the per-box cost, so plotly mode keeps geom_boxplot unchanged, along with its native plotly box traces and tooltips.

Results on a real dataset (20-protein subset, 270 runs, median of 3):

	Before	After	Speedup
QCPlot to PDF, 20 proteins	39.3 s	5.5 s	7.2×
QCPlot to PDF, 1 protein	1.88 s	0.42 s	4.5×
QCPlot to PDF, "All proteins" plot only	3.74 s	1.39 s	2.7×
QCPlot in plotly mode, 20 proteins	64.9 s	27.3 s	2.4×
QCPlot in plotly mode, 1 protein	2.05 s	0.58 s	3.5×
ProfilePlot in plotly mode, 1 protein	1.13 s	0.68 s	1.7×
ConditionPlot in plotly mode, 20 proteins	4.94 s	3.38 s	1.5×

• Other output is unchanged: ProfilePlot and ConditionPlot PDF output keeps byte-identical file sizes, and plotly output keeps the same number of plots and traces.
• Smaller QC PDFs: they are about 14% smaller (765 → 661 KB for 20 proteins), because rectangles are written more compactly than polygons. They look the same.

Changes

R/dataProcessPlots.R

• Wrapped the five per-plot print() calls in if (!isPlotly): two in .plotProfile, two in .plotQC and one in .plotCondition. In plotly mode the plots are no longer rendered to a device, and no Rplots.pdf is created.
• Changed .plotQC's dev.off() guard from address != FALSE to address != FALSE & !isPlotly, matching .plotProfile and .plotCondition. Plotly mode no longer closes the user's graphics device, and does not error once no device is opened.
• .plotQC passes isPlotly to .makeQCPlot.

R/utils_dataprocess_plots.R

• .makeQCPlot takes a new isPlotly argument (default FALSE):
  • isPlotly = TRUE: unchanged geom_boxplot.
  • isPlotly = FALSE: the boxplots are drawn as four vectorized layers: whiskers (geom_segment), boxes (geom_rect, filled by LABEL), medians (geom_segment) and outliers (geom_point).
  • Matching appearance: colour #333333, line width 0.5 (1 for the median), box width 0.75, outlier shape 1 / size 1.5, and the same x-axis range (0.4 to n + 0.6) and tick labels as the discrete axis.
  • Shared layers: facets, fill scale, y-limits, vlines, group labels and theme are as before.
• New internal .qcBoxStats() computes per-run, per-label box statistics with data.table, using the same rules as ggplot2::stat_boxplot:
  • quantile type 7;
  • whiskers at the most extreme values within 1.5 × IQR;
  • y-axis limits applied before the statistics, as scale_y_continuous(limits) does;
  • x positions as indices among the run levels present in the data, so runs with no values keep their slot.

man/

• Regenerated with devtools::document(): dot-makeQCPlot.Rd documents isPlotly, and dot-qcBoxStats.Rd is new.

Housekeeping

• Removed a stray Rplots.pdf that had been committed by accident.

Verified on real data (the "All proteins" plot plus the smallest, median and largest protein): identical box statistics (difference 0), identical outlier counts (up to 75,143), the same axis range and labels, and visually identical images.

Testing

Added to inst/tinytest/test_dataProcessPlots.R:

Test	Checks
10	With isPlotly = TRUE, ProfilePlot, QCPlot and ConditionPlot open no graphics device and write no Rplots.pdf
11	QCPlot with isPlotly = TRUE and a saving address leaves the user's open device open
12	.qcBoxStats() matches stat_boxplot (via layer_data()) on SRMRawData output: box positions, all five statistics, and outliers
13	Same, with user-set y-limits (values outside the limits are dropped before the statistics)
14	Same, with a run that has no values and a run with a single value
15	.makeQCPlot(isPlotly = TRUE) contains a GeomBoxplot layer; isPlotly = FALSE does not
16	Both versions produce the same x-axis range

• Tests 10 and 11 catch the original bug: all three of their checks fail on the original code and pass on the new code.
• The statistics tests detect wrong statistics: each of three deliberately broken versions of .qcBoxStats() (ignoring the y-limits, quantile type 6, whiskers at 2 × IQR) fails at least two of them.
• Full tinytest suite: 531/532, up from 516/517 before the change.
• R CMD check (--no-manual --ignore-vignettes): 1 ERROR, 3 WARNINGs, 5 NOTEs, identical to the parent commit a100dcf. No warning or note mentions the changed code.

The single test failure, which is also the check's one ERROR, is the existing stale snapshot in test_dataProcess.R:71-74 ("dataProcess FeatureLevelData for SRMRawData should be identical to previously saved output"). It fails identically without this change.

Checklist Before Requesting a Review

• I have read the MSstats contributing guidelines
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules (none needed: internal changes only; dataProcessPlots()'s arguments and outputs are unchanged)
• I have run the devtools::document() command after my changes and committed the added files

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 41 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4789d1c5-c34b-47ca-ad3e-2b2e1ff63cc3

📥 Commits

Reviewing files that changed from the base of the PR and between 00c9ba0 and 3cd24f9.

📒 Files selected for processing (1)

• R/dataProcessPlots.R

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d02a7e9e-08a0-4366-aa2d-645ba487a8f7

📥 Commits

Reviewing files that changed from the base of the PR and between f07978d and 00c9ba0.

📒 Files selected for processing (2)

• man/dot-makeQCPlot.Rd
• man/dot-qcBoxStats.Rd

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

QC plots use separate rendering paths for Plotly and non-Plotly output. Plot helpers suppress direct printing in Plotly mode, and QC device closure is also skipped in that mode.

Changes

Plot Rendering

Layer / File(s)	Summary
QC rendering and statistics R/utils_dataprocess_plots.R, inst/tinytest/test_dataProcessPlots.R, man/dot-makeQCPlot.Rd, man/dot-qcBoxStats.Rd	.makeQCPlot retains geom_boxplot for Plotly output. For non-Plotly output, it draws boxplot elements from statistics that exclude missing and out-of-limit values. Tests compare the statistics with ggplot2 references and check plot layers and x-axis ranges. The documentation describes isPlotly and .qcBoxStats.
Plot helper integration R/dataProcessPlots.R, inst/tinytest/test_dataProcessPlots.R	Profile, summary profile, QC, and condition plot helpers skip direct printing in Plotly mode. QC calls pass isPlotly, and QC device closure is skipped in that mode. Tests check graphics-device behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 00c9b

The reviewed rendering changes are mergeable; no actionable risk was identified.

Architecture Summary

Architecture risk: 🔵 Low · up to 00c9b

The change affects 3 systems.

Changed systems: man, R, inst

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — man (service) was modified; 2 changed files map to changed impact.
• observed — R (service) was modified; 2 changed files map to changed impact.
• observed — inst (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in R/dataProcessPlots.R: Original profile plots are printed only when isPlotly is false; the plots continue to be stored in output_plots.
• observed — Modified behavior in R/dataProcessPlots.R: Summary profile plots are printed only when isPlotly is false; the plots continue to be stored in output_plots.
• observed — Modified behavior in R/dataProcessPlots.R: The all-proteins QC plot call now passes isPlotly to .makeQCPlot, and printing is skipped when isPlotly is true.
• observed — Modified behavior in R/dataProcessPlots.R: Per-protein QC plot calls now pass isPlotly to .makeQCPlot and skip printing in Plotly mode. QC device closure now also requires !isPlotly; previously it ran whenever address != FALSE.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the performance change: flat-layer QC boxplots and reduced printing in Plotly mode.
Description check	✅ Passed	The description provides motivation, detailed changes, test results, and a completed checklist. It satisfies the repository template.

✨ Finishing Touches 💡 1

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch scale-data-processing-plots

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit plots with whiskers neat
Plotly keeps its boxes sweet
No stray device opens wide
QC stats stand side by side
The bunny hops through ranges bright}

_{Comment @coderabbitai help to get the list of available commands.}

[tonywu1999]

Can you simplify the documentation across the PR around the adjustment to QC plots? At the moment, I still need to read the code to understand what you did at a high level. Jargon like grob tree and few vectorized layers make it hard to understand.

From my understanding, you're basically computing some basic quantile metrics, rather than every little quantile, and then passing those metrics to be plotted. Please correct me if I'm wrong / did not elaborate enough.

[tonywu1999]

Did you test this on the dataset with file_path /projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds? Which dataset did you do your benchmark on? With /projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds, it seems to be taking forever still.

HPC code to reproduce:

library(MSstats)
quant_data_p2 <- readRDS("/projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds")
dataProcessPlots(quant_data_p2, "QCPlot", which.Protein = "allonly", address = FALSE, isPlotly = TRUE)

[Rudhik1904]

Did you test this on the dataset with file_path /projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds? Which dataset did you do your benchmark on? With /projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds, it seems to be taking forever still.

HPC code to reproduce:

library(MSstats)
quant_data_p2 <- readRDS("/projects/VitekLab/Data/MS/Talus/thp1/quant_data_combined_msstats_plus_fast.rds")
dataProcessPlots(quant_data_p2, "QCPlot", which.Protein = "allonly", address = FALSE, isPlotly = TRUE)

I did use a subset of the test file (quant_data_combined_cpu) mentioned in https://github.com/orgs/Vitek-Lab/projects/9/views/1?sliceBy%5Bvalue%5D=Rudhik1904&pane=issue&itemId=221133751

There is prob stilll more room for improvement. but this was giving me decent result, so want to commit at least that.