fix(PTMs): Fix bugs related to PTM E2E processing#199
Conversation
📝 WalkthroughWalkthroughThe module consolidates plot rendering to consistently use Plotly, refactors PTM quantification logic with an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/module-qc-server.R (1)
269-315:⚠️ Potential issue | 🔴 CriticalPTM PDF export is broken:
address = filewithisPlotly = TRUEconflicts with the PDF wrapper.In the PTM branch (lines 271–280),
dataProcessPlotsPTM()is called with bothaddress = fileandisPlotly = TRUE. Per MSstatsPTM documentation,isPlotly = TRUEdirects the function to write Plotly output directly to an HTML file via theaddressparameter.However,
output$saveplot(lines 435–444) wraps the result in apdf()device that opens the same file:content = function(file) { pdf(file) plotresult(TRUE, input$which, FALSE, TRUE, FALSE) dev.off() }This creates a file I/O conflict: the internal
dataProcessPlotsPTMcall attempts to write HTML tofile, while the wrapper simultaneously opensfileas a PDF device. The result is likely file corruption, a locked file, or a silently invalid PDF.Fix: For the save path, either use
isPlotly = FALSE(reverting to PDF-compatible ggplot output) or omit theaddressparameter when the plotting function is wrapped in an external device handler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-server.R` around lines 269 - 315, The PTM branch in plotresult (dataProcessPlotsPTM) is passing address = file together with isPlotly = TRUE which causes the internal function to write HTML while output$saveplot opens a PDF device; change plotresult to detect the saveFile flag (the first parameter) and when saving (saveFile == TRUE) call dataProcessPlotsPTM with isPlotly = FALSE and do not pass address (or set address = NULL), otherwise keep isPlotly = TRUE and pass address = file; update the PTM branch in plotresult (and similarly any other branches that use address/isPlotly) so PDF export uses non-Plotly/ggplot output while interactive uses Plotly.
🧹 Nitpick comments (1)
R/module-qc-server.R (1)
498-512: Optional: collapse the two PTM branches to reduce duplication.The only difference between the PTM+TMT and PTM+non-TMT branches is the
setnamescall; thequantification(temp$PTM, ...)call is identical. Consider merging:♻️ Proposed refactor
- if (loadpage_input()$BIO == "PTM" && loadpage_input()$DDA_DIA == "TMT"){ - temp = copy(preprocess_data()) - setnames(temp$PTM$ProteinLevelData, - c("Abundance", "Condition", "BioReplicate"), - c("LogIntensities", "GROUP", "SUBJECT")) - abundant$results = quantification(temp$PTM, - type = input$typequant, - format = input$format, - use_log_file = FALSE) - } else if (loadpage_input()$BIO == "PTM" && loadpage_input()$DDA_DIA != "TMT"){ - temp = copy(preprocess_data()) - abundant$results =quantification(temp$PTM, - type = input$typequant, - format = input$format, - use_log_file = FALSE) - } else if (loadpage_input()$DDA_DIA == "TMT"){ + if (loadpage_input()$BIO == "PTM") { + temp = copy(preprocess_data()) + if (loadpage_input()$DDA_DIA == "TMT") { + setnames(temp$PTM$ProteinLevelData, + c("Abundance", "Condition", "BioReplicate"), + c("LogIntensities", "GROUP", "SUBJECT")) + } + abundant$results = quantification(temp$PTM, + type = input$typequant, + format = input$format, + use_log_file = FALSE) + } else if (loadpage_input()$DDA_DIA == "TMT"){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-qc-server.R` around lines 498 - 512, Collapse the duplicated PTM branches by calling setnames only when loadpage_input()$DDA_DIA == "TMT" and then performing the single quantification call; specifically, inside the conditional for loadpage_input()$BIO == "PTM" obtain temp <- copy(preprocess_data()), if DDA_DIA == "TMT" call setnames on temp$PTM$ProteinLevelData (referencing the same column names currently used), then assign abundant$results <- quantification(temp$PTM, type = input$typequant, format = input$format, use_log_file = FALSE). This removes the duplicated quantification call and preserves the unique renaming step.
🤖 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/module-qc-server.R`:
- Around line 513-521: preprocess_data() returns a list containing data.tables,
but copy(preprocess_data()) only clones the list structure so the subsequent
setnames(temp$ProteinLevelData, ...) mutates the original; to fix, deep-copy the
nested data.table(s) before modifying them (e.g., after temp =
copy(preprocess_data()) do an explicit copy of temp$ProteinLevelData) so
setnames changes only the local temp used to compute abundant$results via
quantification; apply the same pattern if there are other nested tables in temp.
---
Outside diff comments:
In `@R/module-qc-server.R`:
- Around line 269-315: The PTM branch in plotresult (dataProcessPlotsPTM) is
passing address = file together with isPlotly = TRUE which causes the internal
function to write HTML while output$saveplot opens a PDF device; change
plotresult to detect the saveFile flag (the first parameter) and when saving
(saveFile == TRUE) call dataProcessPlotsPTM with isPlotly = FALSE and do not
pass address (or set address = NULL), otherwise keep isPlotly = TRUE and pass
address = file; update the PTM branch in plotresult (and similarly any other
branches that use address/isPlotly) so PDF export uses non-Plotly/ggplot output
while interactive uses Plotly.
---
Nitpick comments:
In `@R/module-qc-server.R`:
- Around line 498-512: Collapse the duplicated PTM branches by calling setnames
only when loadpage_input()$DDA_DIA == "TMT" and then performing the single
quantification call; specifically, inside the conditional for
loadpage_input()$BIO == "PTM" obtain temp <- copy(preprocess_data()), if DDA_DIA
== "TMT" call setnames on temp$PTM$ProteinLevelData (referencing the same column
names currently used), then assign abundant$results <- quantification(temp$PTM,
type = input$typequant, format = input$format, use_log_file = FALSE). This
removes the duplicated quantification call and preserves the unique renaming
step.
🪄 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: 10d3259b-fd05-4537-a518-82370e7407e1
📒 Files selected for processing (2)
R/module-qc-server.RR/module-qc-ui.R
💤 Files with no reviewable changes (1)
- R/module-qc-ui.R
1 participant
Motivation and Context
This PR fixes PTM (Post-Translational Modification) handling in the QC (Quality Control) module to ensure consistent Plotly-based visualization and simplified quantification logic. Previously, the PTM code path had inconsistent plot rendering approaches and complex nested conditional logic for quantification. The fix consolidates these patterns to improve maintainability and ensure all PTM plots render consistently via Plotly.
Changes
R/module-qc-server.R
output$showplotto consistently userenderPlotly()withplotlyOutput()wrapped in a scroll-enabled div, removing any PTM-specificrenderPlot/plotOutputvariantsplotresult()function to ensure PTM plots always calldataProcessPlotsPTM(..., isPlotly=TRUE)with immediate return, eliminating the previous alternative non-Plotly code path for PTMabundance()function by replacing nested PTM/TMT/filetype conditions with clearer mutually exclusive cases:BIO == "PTM" && DDA_DIA == "TMT"usingtemp$PTMwith column renamingBIO == "PTM" && DDA_DIA != "TMT"usingtemp$PTMDDA_DIA == "TMT"(non-PTM) usingtemp$ProteinLevelDataR/module-qc-ui.R
Testing
No test files were added or modified. The existing test suite includes PTM-related tests in
tests/testthat/test-utils.Randtests/testthat/test-utils-statmodel-server.Rthat cover PTM data handling in other modules, but no dedicated tests for the QC module's PTM plotting and quantification logic were added to verify these changes.Coding Guidelines
No obvious violations of coding guidelines are present. The refactored code improves readability by:
However, the removal of commented code without commit history context may make it difficult to understand why certain approaches were previously attempted.