Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new exported function Changes
Sequence DiagramsequenceDiagram
actor User
participant Function as MSstatsQualityMetricsPlot
participant Validation as Input Validation
participant Processing as Data Processing
participant ggplot2
participant Plotly
participant FileSystem as File System
User->>Function: input, metric, which.Protein, address, isPlotly
Function->>Validation: Validate columns (metric, Run, ProteinName)
Validation-->>Function: Validation passed
Function->>Processing: Filter by protein & derive Precursor
Processing->>Processing: Aggregate metric by Run & Precursor
Processing-->>Function: Aggregated data
Function->>ggplot2: Create plot with colored precursor lines
ggplot2-->>Function: ggplot object
alt isPlotly = TRUE
Function->>Plotly: Convert to interactive plotly
Plotly-->>Function: plotly object
alt address provided
Function->>FileSystem: Save as HTML
FileSystem-->>Function: Saved
end
Function-->>User: Return plotly object
else isPlotly = FALSE
alt address provided
Function->>FileSystem: Save as PDF
FileSystem-->>Function: Saved
end
Function-->>User: Return ggplot object
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
R/plot_quality_metrics.R (2)
82-87:aggregateon all-NA groups emitsNaNwith a warning.When every fragment-ion value for a given
Run × PrecursorisNA,mean(..., na.rm = TRUE)returnsNaNandmean.defaultwarns"argument is not numeric or logical"/ producesNaN. The plot still renders (missing points), but users will see a stream of confusing warnings. Consider suppressing the no-complete-cases warning, or filtering all-NA groups out before aggregation.♻️ Suggested tweak
- plot_df <- aggregate( - input_df[[metric]], - by = list(Run = input_df$Run, Precursor = input_df$Precursor), - FUN = mean, na.rm = TRUE - ) + plot_df <- suppressWarnings(aggregate( + input_df[[metric]], + by = list(Run = input_df$Run, Precursor = input_df$Precursor), + FUN = function(v) if (all(is.na(v))) NA_real_ else mean(v, na.rm = TRUE) + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/plot_quality_metrics.R` around lines 82 - 87, The aggregate call on input_df[[metric]] with FUN = mean, na.rm = TRUE produces NaN and warnings for groups where all values are NA; to fix, pre-filter those all-NA Run×Precursor groups before calling aggregate (e.g., remove rows or group keys where all fragment values for the given metric are NA) so aggregate only sees groups with at least one non-NA, then run the existing aggregate into plot_df and keep the colnames fix for "x" -> metric; reference input_df, metric, Run, Precursor, and plot_df when locating where to add the filter.
119-123: Useon.exit(dev.off())to protect againstprint(p)errors.If
print(p)throws (e.g. ggplot build error on an unexpected metric column type),dev.off()is never called and the PDF device leaks, which can cause subsequent plotting calls in the same session to write into the orphaned file or fail. Same concern doesn't apply to the plotly HTML branch sincesave_htmlmanages its own file handle.♻️ Suggested fix
if (!identical(address, FALSE)) { pdf(paste0(address, "QualityMetricsPlot.pdf")) + on.exit(dev.off(), add = TRUE) print(p) - dev.off() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/plot_quality_metrics.R` around lines 119 - 123, The PDF device opened in the block guarded by if (!identical(address, FALSE)) should be protected with on.exit(dev.off()) so that the device is closed even if print(p) throws; update the block around pdf(paste0(address, "QualityMetricsPlot.pdf")) / print(p) / dev.off() to call on.exit(dev.off(), add = TRUE) immediately after opening the device (before print(p)) so the device will always be closed, keeping the final dev.off() as a normal cleanup or removing the redundant one if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/plot_quality_metrics.R`:
- Line 97: The DESCRIPTION currently imports ggplot2 without a version
constraint but the code uses geom_line(linewidth = 0.6) which requires ggplot2
>= 3.4.0; update the DESCRIPTION Imports entry to declare ggplot2 (>= 3.4.0) so
installations pull a ggplot2 version that supports the linewidth argument and
avoid "unused argument (linewidth = 0.6)" errors.
- Around line 59-70: The function currently fails to validate required columns
and the which.Protein argument: first, add an explicit missing() check for
which.Protein and provide a clear error if it's not supplied; second, before any
use of input_df$ProteinName/PeptideSequence/PrecursorCharge (and Run/metric),
verify those column names exist in input_df (e.g., check "ProteinName",
"PeptideSequence", "PrecursorCharge", "Run" and the requested metric via
colnames(input_df)) and stop with descriptive messages if any are missing;
finally, only after these column-existence checks perform the membership test
for which.Protein in input_df$ProteinName so the error reflects a missing
protein rather than a missing column.
---
Nitpick comments:
In `@R/plot_quality_metrics.R`:
- Around line 82-87: The aggregate call on input_df[[metric]] with FUN = mean,
na.rm = TRUE produces NaN and warnings for groups where all values are NA; to
fix, pre-filter those all-NA Run×Precursor groups before calling aggregate
(e.g., remove rows or group keys where all fragment values for the given metric
are NA) so aggregate only sees groups with at least one non-NA, then run the
existing aggregate into plot_df and keep the colnames fix for "x" -> metric;
reference input_df, metric, Run, Precursor, and plot_df when locating where to
add the filter.
- Around line 119-123: The PDF device opened in the block guarded by if
(!identical(address, FALSE)) should be protected with on.exit(dev.off()) so that
the device is closed even if print(p) throws; update the block around
pdf(paste0(address, "QualityMetricsPlot.pdf")) / print(p) / dev.off() to call
on.exit(dev.off(), add = TRUE) immediately after opening the device (before
print(p)) so the device will always be closed, keeping the final dev.off() as a
normal cleanup or removing the redundant one if desired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c05809a-9219-4f93-b7b5-f7409f82d64b
📒 Files selected for processing (3)
NAMESPACER/plot_quality_metrics.Rman/MSstatsQualityMetricsPlot.Rd
1 participant
Description
Add
MSstatsQualityMetricsPlotvisualization helperPlot protein metrics across run order
Aggregate fragment ions to precursor means
Support PDF and plotly HTML export
Diagram Walkthrough
File Walkthrough
NAMESPACE
Export new quality metrics plotting functionNAMESPACE
MSstatsQualityMetricsPlotplot_quality_metrics.R
Add run-ordered quality metrics plotting utilityR/plot_quality_metrics.R
MSstatsQualityMetricsPlotfor QC metricsmetric,Run, andwhich.Proteinggplot2, plotly, PDF, and HTMLMSstatsQualityMetricsPlot.Rd
Document quality metrics plotting interfaceman/MSstatsQualityMetricsPlot.Rd