fix(dataProcessPlots, groupComparisonPlots): Migrate away from deprecated functions for ggplot #175
Conversation
WalkthroughReplaces ggplot2 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 1
🧹 Nitpick comments (1)
R/utils_dataprocess_plots.R (1)
237-237: Minor: prefer explicit data arg; otherwise looks goodBoxplot and text layers use
.datacorrectly. Consider making the ggplot call explicit for consistency.-ggplot(input, aes(x = .data$RUN, y = .data$ABUNDANCE)) + +ggplot(data = input, aes(x = .data$RUN, y = .data$ABUNDANCE)) +Also applies to: 240-240, 248-248
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
R/groupComparisonQCPlots.R(3 hunks)R/utils_dataprocess_plots.R(8 hunks)R/utils_groupcomparison_plots.R(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
R/utils_dataprocess_plots.R (4)
100-104: LGTM: label layer converted to tidy-eval correctly
.data$RUN/.data$ABUNDANCE/.data$Nameusage is correct and avoids NSE notes.
159-161: Verify analysis levels align with manual scalesMapping color/size/linetype to
.data$analysislooks good. Please confirminput$analysishas exactly the expected two levels and matches the values passed toscale_colour_manualandscale_size_manual, or you’ll get dropped/empty legends.Also applies to: 168-175, 195-195
212-213: LGTM: late point layer inherits tidy mappingsAdding the point layer with
.data$newABUNDANCEand.data$analysisis consistent with the top-level mapping.
271-273: LGTM: condition plot aesthetics migrated correctlyErrorbar/point mappings via
.dataare correct.R/utils_groupcomparison_plots.R (4)
191-196: Tidy-eval migration is correct; ensure rlang::.data is importedNice use of
.data[[log_adjp]]for dynamic column. Make sure the package imports rlang’s.data(see verification script in my other comment).
212-212: LGTM: text label mapping updated
geom_text_repel(..., aes(label = .data$Protein))is correct.
229-229: LGTM: dynamic y mapping in guide/threshold linesUsing
.data[[log_adjp]]across all threshold/guide layers keeps things consistent.Also applies to: 251-251, 257-257, 262-262
291-293: LGTM: comparison plot aesthetics migrated; verify Label orderingTidy-eval for
Label/logFC/ciwis correct. Please confirminput$Labelis factor-ordered as intended so the x-axis order matches expectations.R/groupComparisonQCPlots.R (2)
124-124: LGTM: QQ plot uses .data pronoun correctly
aes(sample = .data$residual)is the right tidy-eval form. Ensure rlang’s.datais imported (see repo-wide check script).
175-176: LGTM: residual plot aesthetics migrated
aes(x = .data$fitted, y = .data$residual)bound tosub.residuals.tableis correct.
R/utils_dataprocess_plots.R
Outdated
| profile_plot = ggplot(input, aes(x = .data$RUN, y = .data$newABUNDANCE, | ||
| color = .data[[type_color]], linetype = .data$FEATURE)) + |
There was a problem hiding this comment.
🧩 Analysis chain
Import rlang::.data and minor consistency nit
- You switched to tidy-eval correctly. Ensure rlang’s .data pronoun is imported, otherwise R CMD check will warn/fail.
- Optional: use explicit
data = inputfor consistency with other calls.
Apply this minimal tweak for consistency:
-profile_plot = ggplot(input, aes(x = .data$RUN, y = .data$newABUNDANCE,
+profile_plot = ggplot(data = input, aes(x = .data$RUN, y = .data$newABUNDANCE,
color = .data[[type_color]], linetype = .data$FEATURE)) +To wire imports via roxygen (outside these lines), add once in a package R file:
#' @importFrom rlang .data
NULLRun the script below to verify:
Also applies to: 66-66
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check roxygen import tag:"
rg -n '@importFrom\s+rlang\s+\.data' -S R || echo "MISSING: add @importFrom rlang .data"
echo "Check NAMESPACE for import:"
rg -n 'importFrom\(rlang,\s*\.data\)' NAMESPACE || echo "MISSING: NAMESPACE importFrom(rlang,.data)"
echo "Check DESCRIPTION for rlang in Imports/Depends:"
rg -nP '^(Imports|Depends):' -nC2 DESCRIPTION | rg -n 'rlang' || echo "MISSING: add rlang to Imports"
echo "Any remaining aes_string?:"
rg -n 'aes_string\(' -S R || trueLength of output: 661
Import rlang::.data and add ggplot data argument
- Add explicit data = input to ggplot calls in R/utils_dataprocess_plots.R (lines 59–60 and 66):
-profile_plot = ggplot(input, aes(x = .data$RUN, y = .data$newABUNDANCE,
+profile_plot = ggplot(data = input, aes(x = .data$RUN, y = .data$newABUNDANCE,
color = .data[[type_color]], linetype = .data$FEATURE)) +- Add a single roxygen import (e.g., in R/zzz.R or any package R file):
#' @importFrom rlang .data
NULL- Regenerate NAMESPACE and ensure DESCRIPTION lists rlang in Imports (run devtools::document() or roxygen2::roxygenize()), then re-run R CMD check.
Verification: script output → @importFrom rlang .data missing; NAMESPACE importFrom missing; DESCRIPTION missing rlang; no aes_string found.
🤖 Prompt for AI Agents
In R/utils_dataprocess_plots.R around lines 59–60 (and also update the ggplot
call at line 66), the ggplot call is missing an explicit data = input and uses
.data without ensuring rlang is imported; update both ggplot calls to include
data = input, add a roxygen import line for rlang::.data (for example in
R/zzz.R: a roxygen @importFrom rlang .data directive followed by NULL), add
rlang to DESCRIPTION Imports, then regenerate NAMESPACE (run
devtools::document() or roxygen2::roxygenize()) and re-run R CMD check to ensure
@importFrom rlang .data, NAMESPACE importFrom, and DESCRIPTION import are
present and no aes_string remains.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
R/utils_dataprocess_plots.R (3)
59-63: Tidy-eval migration looks good; consider explicit data arg and verify linewidth support.
- Optional: prefer
data = inputfor consistency with other calls.- Verify
ggplot2 (>= 3.4.0)since you’re usinglinewidth; otherwise switch tosize.Apply minimal tweaks if you don’t bump ggplot2:
-profile_plot = ggplot(input, aes(x = .data$RUN, y = .data$newABUNDANCE, +profile_plot = ggplot(data = input, aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], linetype = .data$FEATURE)) + - geom_line(linewidth = 0.5) + geom_line(size = 0.5)Run to verify imports/versions:
#!/bin/bash set -euo pipefail echo "Check rlang::.data roxygen import:" rg -n '@importFrom\s+rlang\s+\.data' -S R || echo "MISSING: add @importFrom rlang .data" echo "Check NAMESPACE for import:" rg -n 'importFrom\(rlang,\s*\.data\)' NAMESPACE || echo "MISSING: NAMESPACE importFrom(rlang,.data)" echo "Check DESCRIPTION imports:" rg -nP '^(Imports|Depends):' -nC2 DESCRIPTION | rg -n 'rlang' || echo "MISSING: add rlang to Imports" echo "Check ggplot2 minimal version:" rg -n '^Imports:' -nC3 DESCRIPTION | rg -n 'ggplot2' && rg -n 'ggplot2\s*\(>=\s*3\.4\.0\)' DESCRIPTION || echo "Consider: require ggplot2 (>= 3.4.0) or replace linewidth with size" echo "Any remaining aes_string?:" rg -n 'aes_string\(' -S R || true
159-164: LGTM; confirm linewidth compatibility.
286-286: Verify linewidth support or use size.R/utils_groupcomparison_plots.R (3)
192-196: LGTM on aes() + .data migration.Ensure
rlang::.datais imported via roxygen/NAMESPACE (see script in prior comment).
229-237: LGTM; tidy-eval for dynamic y works. Validate linewidth support.
302-302: Verify linewidth or swap to size for broader compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/utils_dataprocess_plots.R(8 hunks)R/utils_groupcomparison_plots.R(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
R/utils_dataprocess_plots.R (7)
100-100: LGTM.
194-194: LGTM.
212-214: LGTM.
237-239: LGTM.
247-247: LGTM.
271-273: LGTM; good tidy-eval conversion.
291-291: LGTM.R/utils_groupcomparison_plots.R (5)
213-213: LGTM.
251-254: LGTM.
256-259: LGTM.
261-264: LGTM.
291-293: LGTM.
| if (is_censored) { | ||
| profile_plot = profile_plot + | ||
| geom_point(aes_string(x = "RUN", y = "newABUNDANCE", color = type_color, shape = "censored"), | ||
| geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$censored), |
There was a problem hiding this comment.
Bug: shape mapped to nonexistent column (censored) — use is_censored to match data prep.
You factorize input$is_censored above but map shape = .data$censored. Align the aesthetic to avoid empty shapes.
Apply:
- geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$censored),
+ geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$is_censored),
data = input,
size = dot.size.profile) +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$censored), | |
| geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$is_censored), | |
| data = input, | |
| size = dot.size.profile) + |
🤖 Prompt for AI Agents
In R/utils_dataprocess_plots.R around line 66, the geom_point maps shape =
.data$censored which doesn't exist (the data uses is_censored); change the
aesthetic to shape = .data$is_censored so it references the factorized variable
from data prep (and update any corresponding scale_shape_* calls or legend
titles to use is_censored if present).
| aes(x = .data$RUN, y = .data$newABUNDANCE, | ||
| color = .data$analysis, size = .data$analysis, | ||
| shape = .data$censored)) + | ||
| geom_point(data = input[input$PEPTIDE == "Run summary"], | ||
| aes_string(x = "RUN", y = "newABUNDANCE", | ||
| color = "analysis", size = "analysis", | ||
| shape = "censored")) + | ||
| aes(x = .data$RUN, y = .data$newABUNDANCE, | ||
| color = .data$analysis, size = .data$analysis, | ||
| shape = .data$censored)) + | ||
| scale_shape_manual(values = c(16, 1), |
There was a problem hiding this comment.
Same shape/aesthetic mismatch here.
Map to is_censored in both point layers to match the available column.
Apply:
- geom_point(data = input[input$PEPTIDE != "Run summary"],
- aes(x = .data$RUN, y = .data$newABUNDANCE,
- color = .data$analysis, size = .data$analysis,
- shape = .data$censored)) +
+ geom_point(data = input[input$PEPTIDE != "Run summary"],
+ aes(x = .data$RUN, y = .data$newABUNDANCE,
+ color = .data$analysis, size = .data$analysis,
+ shape = .data$is_censored)) +
geom_point(data = input[input$PEPTIDE == "Run summary"],
- aes(x = .data$RUN, y = .data$newABUNDANCE,
- color = .data$analysis, size = .data$analysis,
- shape = .data$censored)) +
+ aes(x = .data$RUN, y = .data$newABUNDANCE,
+ color = .data$analysis, size = .data$analysis,
+ shape = .data$is_censored)) +🤖 Prompt for AI Agents
In R/utils_dataprocess_plots.R around lines 168 to 175, both geom_point layers
are mapping shape to .data$censored which doesn't exist; change the mapping to
.data$is_censored in both point layers (i.e., replace shape = .data$censored
with shape = .data$is_censored) and keep the existing scale_shape_manual so the
shape values align with the is_censored factor levels.
tonywu1999
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
R/utils_dataprocess_plots.R (2)
66-66: Critical: Shape aesthetic references nonexistent column.The
geom_point()mapsshape = .data$censored, but line 49 shows the column is namedis_censored. This mismatch will cause plotting errors or missing shapes.Apply this fix:
- geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$censored), + geom_point(aes(x = .data$RUN, y = .data$newABUNDANCE, color = .data[[type_color]], shape = .data$is_censored), data = input, size = dot.size.profile) +
168-174: Critical: Both geom_point layers reference nonexistent column.Both
geom_point()calls mapshape = .data$censored, but the data column isis_censored(see line 49 in.makeProfilePlot). This will cause plotting errors.Apply this fix:
geom_point(data = input[input$PEPTIDE != "Run summary"], - aes(x = .data$RUN, y = .data$newABUNDANCE, - color = .data$analysis, size = .data$analysis, - shape = .data$censored)) + + aes(x = .data$RUN, y = .data$newABUNDANCE, + color = .data$analysis, size = .data$analysis, + shape = .data$is_censored)) + geom_point(data = input[input$PEPTIDE == "Run summary"], - aes(x = .data$RUN, y = .data$newABUNDANCE, - color = .data$analysis, size = .data$analysis, - shape = .data$censored)) + + aes(x = .data$RUN, y = .data$newABUNDANCE, + color = .data$analysis, size = .data$analysis, + shape = .data$is_censored)) +
🧹 Nitpick comments (1)
R/utils_dataprocess_plots.R (1)
237-237: Optional: Add explicit data argument for consistency.While
ggplot(input, aes(...))is valid, other updated calls in this file use the explicitdata = inputform. Consider applying for consistency:- ggplot(input, aes(x = .data$RUN, y = .data$ABUNDANCE)) + + ggplot(data = input, aes(x = .data$RUN, y = .data$ABUNDANCE)) +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DESCRIPTION(2 hunks)NAMESPACE(1 hunks)R/msstats-package.R(1 hunks)R/utils_dataprocess_plots.R(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
R/msstats-package.R (1)
4-4: LGTM: rlang import enables tidy evaluation.The
@importFrom rlang .datadirective correctly imports the.datapronoun, which is essential for the tidy evaluation pattern used throughout the updated plotting code.NAMESPACE (1)
109-109: LGTM: Generated import for .data pronoun.This NAMESPACE entry is correctly generated from the
@importFrom rlang .dataroxygen directive and enables tidy evaluation across the package.DESCRIPTION (2)
40-41: LGTM: rlang dependency added appropriately.Adding rlang to Imports is necessary to support the
.datapronoun used in the modernized ggplot2 code throughout the package.
57-57: LGTM: Standard RoxygenNote version bump.The RoxygenNote version update reflects the regeneration of package documentation and NAMESPACE.
R/utils_dataprocess_plots.R (11)
59-60: LGTM: Clean migration to tidy evaluation.The ggplot call correctly uses
data = inputwith.datapronouns and.data[[type_color]]for the dynamic column reference. This eliminates the deprecatedaes_string().
62-62: LGTM: Correct migration from size to linewidth.Switching to
linewidth = 0.5forgeom_line()resolves the ggplot2 deprecation warning about usingsizefor line width.
100-100: LGTM: geom_text correctly uses .data pronouns.The text layer aesthetics properly reference columns via
.data$RUN,.data$ABUNDANCE, and.data$Name.
158-163: LGTM: Summary plot correctly migrated to tidy evaluation.The ggplot call uses explicit
data = inputwith.datapronouns for all aesthetics, andgeom_line()correctly useslinewidthinstead of the deprecatedsize.
194-194: LGTM: geom_text correctly uses .data pronouns.The text layer aesthetics properly reference columns via
.datapronouns.
212-213: LGTM: geom_point correctly uses .data pronouns.The point layer aesthetics properly reference columns via
.data$RUN,.data$newABUNDANCE, and.data$analysis.
239-239: LGTM: geom_boxplot correctly uses .data pronoun.The fill aesthetic properly references the column via
.data$LABEL.
247-247: LGTM: geom_text correctly uses .data pronouns.The text layer aesthetics properly reference columns via
.datapronouns.
271-271: LGTM: ggplot correctly uses .data pronouns with explicit data.The aesthetics properly use
.data$Labeland.data$Mean. Thedata = inputargument is present (though placed afteraes, which is valid R syntax).
272-272: LGTM: geom_errorbar correctly uses .data pronouns.The error bar aesthetics properly reference columns via
.data$Meanand.data$ciw.
286-286: LGTM: geom_hline correctly uses linewidth.Switching to
linewidth = 0.6resolves the ggplot2 deprecation warning about usingsizefor line width.
Motivation and Context
DataProcessPlots in MSstats is generating warnings of deprecated functions in ggplot.
Changes
aes_stringtoaessizetolinewidthfor geom_line, geom_hline functionsTesting
Deprecation messages disappear.
Checklist Before Requesting a Review
PR Type
Enhancement
Description
Replace aes_string with aes across plots
Use .data pronoun for tidy evaluation
Update ggplot mappings and labels
Minor formatting and EOF newline adjustments
Diagram Walkthrough
File Walkthrough
groupComparisonQCPlots.R
Refactor QC plots to aes with .dataR/groupComparisonQCPlots.R
.data$residual)
utils_dataprocess_plots.R
Modernize data process plots to tidy-eval aesR/utils_dataprocess_plots.R
utils_groupcomparison_plots.R
Update group comparison plots to aes/.dataR/utils_groupcomparison_plots.R
Summary by CodeRabbit
Bug Fixes
Refactor
Chores