Merge the Sample size calculation feature for dose response curve into Development#181
Merge the Sample size calculation feature for dose response curve into Development#181swaraj-neu wants to merge 10 commits intodevelfrom
Conversation
…ay crash or disconnect the Shiny session
📝 WalkthroughWalkthroughThis PR adds dose-response simulation support to the experiment design module by integrating MSstatsResponse functions for future experiment simulation, propagating the contrast matrix through the module hierarchy, and introducing conditional UI/server logic to handle response curve mode alongside the existing standard mode. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Sidebar UI
participant Server as expdesServer
participant DataPrep as Data Preparation
participant Sim as MSstatsResponse Functions
participant Plot as Plot Generation
User->>UI: Select protein from list
User->>UI: Adjust replicate range
User->>UI: Click "Run simulation"
UI->>Server: Trigger reactive observer
Server->>DataPrep: Merge protein-level data + contrast matrix
DataPrep->>DataPrep: Prepare dose-response fit data
Server->>Sim: run_tpr_simulation(rep_range, n_proteins=1000)
Sim-->>Server: Return simulation results
Server->>Plot: plot_tpr_power_curve(simulation_results)
Plot-->>Server: Generate power curve plot
Server->>UI: Display plot & enable PDF download
User->>Server: Click download PDF
Server->>Sim: Extract simulation data for export
Sim-->>Server: Return formatted results
Server-->>User: Download future_exp.pdf with panels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
R/module-statmodel-server.R (1)
279-283: Prefer returning a read-only contrast accessor.
contrastis a mutablereactiveValues, so exposing it here leaks internal state (matrixandrow) across the module boundary. From the wiring in this PR, the downstream consumer only needs the matrix value, so returningreactive(contrast$matrix)would keep the contract smaller and avoid accidental cross-module writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-statmodel-server.R` around lines 279 - 283, Return a read-only accessor instead of the mutable reactiveValues `contrast`: replace the exported `contrast` from the module return with a reactive that reads `contrast$matrix` (e.g., use `reactive({ contrast$matrix })`) so callers get only the matrix value and cannot modify internal `contrast` reactiveValues; update any caller references expecting `contrast` to use the new reactive accessor name if you rename it.R/module-expdes-server.R (1)
291-317: Extract the panel builder once for plot and download.The ggplot construction here duplicates the
make_panel()logic inplot_tpr_power_curve(). Keeping both copies in sync will be fragile; a shared helper would prevent label/theme drift between the interactive and PDF outputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 291 - 317, Extract the ggplot construction into a single reusable helper (e.g., make_panel) and reuse it from both plot_tpr_power_curve() and the download handler currently assigned to output[[NAMESPACE_EXPDES$download_future]]: move the make_panel definition out of the downloadHandler block to module scope (or into a shared helper file) so both plot_tpr_power_curve and the downloadHandler call the same function with the same arguments (data, title, color); ensure the helper accepts NumConcs/TPR/N_rep inputs and returns a ggplot object, then replace the duplicated ggplot code inside plot_tpr_power_curve and the downloadHandler with calls to make_panel(simulation_results_subset, title, color).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@man/expdesServer.Rd`:
- Around line 15-17: The Rd shows statmodel_contrast in \usage{} but it is
missing from the arguments docs—open R/module-expdes-server.R and add an Roxygen
`@param` statmodel_contrast entry for the statmodel_contrast argument (matching
its name and describing expected type, purpose and default behavior), then
re-run document generation (e.g., devtools::document() or
roxygen2::roxygenise()) so the updated `@param` is propagated into
man/expdesServer.Rd; ensure the description aligns with how statmodel_contrast
is used in the function that declares it.
In `@R/module-expdes-server.R`:
- Around line 146-156: The roxygen docs for expdesServer are missing the new
parameter documentation for statmodel_contrast; add a `@param` entry for
statmodel_contrast in the roxygen block above the expdesServer function
describing its purpose, expected type (e.g., function or NULL), return/behavior
impact, and default (NULL) so the generated help correctly documents the
parameter and matches the function signature.
- Around line 181-183: The current tryCatch around the protein_choices
assignment swallows errors from
prepared_response_data()/prepare_dose_response_fit(), hiding validation failures
and leaving the UI stuck; replace the empty error handler so the validation
error is surfaced: either remove the tryCatch so the error propagates, or in the
error function call shiny::showNotification or shiny::validate(shiny::need(...))
with e$message (or rethrow using stop(e)) so users see the
prepare_dose_response_fit() validation message; locate the code that assigns
protein_choices <- unique(prepared_response_data()$protein) and update the error
handling accordingly.
- Around line 218-228: The handler currently only checks
input[[NAMESPACE_EXPDES$protein_select]] but always calls
run_tpr_simulation(rep_range = ..., n_proteins = 1000), so the selection has no
effect; update the observer to read the selected value
(input[[NAMESPACE_EXPDES$protein_select]]) and translate it into the appropriate
argument(s) for run_tpr_simulation (e.g., pass a selected_protein id, adjust
n_proteins, or supply an interaction_strength parameter) and call
run_tpr_simulation with that value instead of the hard-coded n_proteins; if
run_tpr_simulation lacks the needed parameter, extend its signature to accept
and use the protein-specific input and update any downstream expectations
accordingly.
- Around line 53-65: The loop that builds results with run_one over grid_df can
return NULL when every run fails (so results becomes NULL) and the outer
tryCatch then incorrectly treats the whole job as successful; after assembling
results from the do.call(rbind, lapply(...)) call check for the all-failed case
and throw an error (e.g. stop("All simulations failed for grid_df; see
individual errors from run_one/futureExperimentSimulation")) so the outer
tryCatch surfaces the failure; specifically modify the code after results is
assigned to detect is.null(results) or (is.data.frame(results) && nrow(results)
== 0) and call stop with a clear message referencing
run_one/futureExperimentSimulation/grid_df.
---
Nitpick comments:
In `@R/module-expdes-server.R`:
- Around line 291-317: Extract the ggplot construction into a single reusable
helper (e.g., make_panel) and reuse it from both plot_tpr_power_curve() and the
download handler currently assigned to
output[[NAMESPACE_EXPDES$download_future]]: move the make_panel definition out
of the downloadHandler block to module scope (or into a shared helper file) so
both plot_tpr_power_curve and the downloadHandler call the same function with
the same arguments (data, title, color); ensure the helper accepts
NumConcs/TPR/N_rep inputs and returns a ggplot object, then replace the
duplicated ggplot code inside plot_tpr_power_curve and the downloadHandler with
calls to make_panel(simulation_results_subset, title, color).
In `@R/module-statmodel-server.R`:
- Around line 279-283: Return a read-only accessor instead of the mutable
reactiveValues `contrast`: replace the exported `contrast` from the module
return with a reactive that reads `contrast$matrix` (e.g., use `reactive({
contrast$matrix })`) so callers get only the matrix value and cannot modify
internal `contrast` reactiveValues; update any caller references expecting
`contrast` to use the new reactive accessor name if you rename it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6b7cfbc-9254-4fa8-9a19-e17327596728
📒 Files selected for processing (10)
NAMESPACER/MSstatsShiny.RR/constants.RR/module-expdes-server.RR/module-expdes-ui.RR/module-statmodel-server.RR/server.Rman/expdesServer.Rdman/plot_tpr_power_curve.Rdman/run_tpr_simulation.Rd
| observeEvent(input[[NAMESPACE_EXPDES$run_simulation]], { | ||
| req(input[[NAMESPACE_EXPDES$protein_select]]) | ||
| req(input[[NAMESPACE_EXPDES$rep_range]]) | ||
|
|
||
| show_modal_spinner(text = "Running simulations... This may take a minute.") | ||
|
|
||
| output$result_plot = renderPlotly({ | ||
| designSampleSizePlots(future_exp(), isPlotly = TRUE) | ||
| tryCatch({ | ||
| results <- run_tpr_simulation( | ||
| rep_range = input[[NAMESPACE_EXPDES$rep_range]], | ||
| n_proteins = 1000 | ||
| ) |
There was a problem hiding this comment.
protein_select currently has no effect on the simulation.
This handler only gates on input[[NAMESPACE_EXPDES$protein_select]]; the call still runs run_tpr_simulation() with a fixed n_proteins = 1000 and no protein-specific input. Every selection therefore produces the same curve, which is misleading for a control labeled “Select protein (strong interaction)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/module-expdes-server.R` around lines 218 - 228, The handler currently only
checks input[[NAMESPACE_EXPDES$protein_select]] but always calls
run_tpr_simulation(rep_range = ..., n_proteins = 1000), so the selection has no
effect; update the observer to read the selected value
(input[[NAMESPACE_EXPDES$protein_select]]) and translate it into the appropriate
argument(s) for run_tpr_simulation (e.g., pass a selected_protein id, adjust
n_proteins, or supply an interaction_strength parameter) and call
run_tpr_simulation with that value instead of the hard-coded n_proteins; if
run_tpr_simulation lacks the needed parameter, extend its signature to accept
and use the protein-specific input and update any downstream expectations
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
R/module-expdes-server.R (3)
312-324: Consider extracting sharedmake_panelhelper.This
make_panelfunction (lines 312-324) duplicates logic from the one inplot_tpr_power_curve(lines 88-110). A shared helper accepting bothtitleandshow_legendparameters would reduce duplication and ensure consistent styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 312 - 324, The two identical plotting helpers should be consolidated: extract a shared make_panel helper (used by plot_tpr_power_curve and the current function) that accepts parameters (data, title, color, show_legend) and reuses existing symbols like k_grid and ltype_values; update both call sites to call the new shared make_panel and toggle the legend via show_legend instead of duplicating ggplot construction so styling and behavior remain consistent across plots.
265-287: Consider refactoringfuture_expto a reactive.Defining
future_expas a local function insideobserve()causes both output handlers to be reassigned on every reactive dependency change. While functional, this is not idiomatic Shiny and could be inefficient with complex UIs.♻️ Idiomatic alternative using reactive()
Define
future_expas a reactive outside the observe block:future_exp <- reactive({ req(!is_response_curve(), input$param) sample_x <- if (input$param == "sample") TRUE else input$nsample power_x <- if (input$param == "npower") TRUE else input$power designSampleSize( data = data_comparison()$FittedModel, desiredFC = input$desirFC, FDR = input$FDR, numSample = sample_x, power = power_x ) })Then define outputs at the top level of the server function, not inside observe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 265 - 287, The local function future_exp should be converted to a reactive so outputs aren't re-bound on every dependency change: create future_exp <- reactive({ ... }) (moving it outside any observe) that uses req(!is_response_curve(), input$param) and computes sample_x and power_x the same way, then calls designSampleSize(data = data_comparison()$FittedModel, desiredFC = input$desirFC, FDR = input$FDR, numSample = sample_x, power = power_x); after that, move the output[[NAMESPACE_EXPDES$result_plot]] <- renderPlotly({ designSampleSizePlots(future_exp(), isPlotly = TRUE) }) and output[[NAMESPACE_EXPDES$download_future]] <- downloadHandler(...) to the top-level server scope so they reference future_exp() reactively instead of recreating handlers inside an observe.
85-86: Consider adding a defensive check for linetype count.The
ltypesvector has exactly 5 elements, which matches the current slider maximum (line 201). If the slider range is later extended,ltypes[seq_along(rep_levels)]will silently includeNAvalues, causing plot rendering issues.🛡️ Defensive approach
ltypes <- c("dotted", "dotdash", "dashed", "longdash", "solid") + if (length(rep_levels) > length(ltypes)) { + warning("More replicate levels than available linetypes; recycling linetypes.") + ltypes <- rep_len(ltypes, length(rep_levels)) + } ltype_values <- setNames(ltypes[seq_along(rep_levels)], as.character(rep_levels))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 85 - 86, The code constructs ltype_values from ltypes and rep_levels but doesn't guard against rep_levels being longer than ltypes, which would produce NAs; in the ltypes / ltype_values logic add a defensive check in the server (check length(rep_levels) against length(ltypes) inside the reactive/function that builds ltype_values) and handle it by either (a) throwing a clear error or warning when length(rep_levels) > length(ltypes), or (b) extending ltypes safely (e.g. recycling or repeating the last element) before calling setNames; reference the ltypes vector and ltype_values assignment and rep_levels when adding the guard so the code fails predictably instead of creating NA linetypes.
🤖 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-expdes-server.R`:
- Around line 300-305: The content function inside the downloadHandler currently
returns NULL when simulation_results() is NULL; instead, write a clear error
message into the provided file path so the downloaded file contains the error
text (do not call stop()); locate the downloadHandler's content = function(file)
{ ... } block and where it checks simulation_results(), and replace the early
return with code that writes a descriptive message (e.g., "Please run the
simulation first.") to the file (using writeLines or similar) and then exit the
function normally so the download contains the error text rather than an
empty/invalid file.
---
Nitpick comments:
In `@R/module-expdes-server.R`:
- Around line 312-324: The two identical plotting helpers should be
consolidated: extract a shared make_panel helper (used by plot_tpr_power_curve
and the current function) that accepts parameters (data, title, color,
show_legend) and reuses existing symbols like k_grid and ltype_values; update
both call sites to call the new shared make_panel and toggle the legend via
show_legend instead of duplicating ggplot construction so styling and behavior
remain consistent across plots.
- Around line 265-287: The local function future_exp should be converted to a
reactive so outputs aren't re-bound on every dependency change: create
future_exp <- reactive({ ... }) (moving it outside any observe) that uses
req(!is_response_curve(), input$param) and computes sample_x and power_x the
same way, then calls designSampleSize(data = data_comparison()$FittedModel,
desiredFC = input$desirFC, FDR = input$FDR, numSample = sample_x, power =
power_x); after that, move the output[[NAMESPACE_EXPDES$result_plot]] <-
renderPlotly({ designSampleSizePlots(future_exp(), isPlotly = TRUE) }) and
output[[NAMESPACE_EXPDES$download_future]] <- downloadHandler(...) to the
top-level server scope so they reference future_exp() reactively instead of
recreating handlers inside an observe.
- Around line 85-86: The code constructs ltype_values from ltypes and rep_levels
but doesn't guard against rep_levels being longer than ltypes, which would
produce NAs; in the ltypes / ltype_values logic add a defensive check in the
server (check length(rep_levels) against length(ltypes) inside the
reactive/function that builds ltype_values) and handle it by either (a) throwing
a clear error or warning when length(rep_levels) > length(ltypes), or (b)
extending ltypes safely (e.g. recycling or repeating the last element) before
calling setNames; reference the ltypes vector and ltype_values assignment and
rep_levels when adding the guard so the code fails predictably instead of
creating NA linetypes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b44211c1-c3f5-4780-98a6-4d8d0d48acd4
📒 Files selected for processing (2)
R/module-expdes-server.Rman/expdesServer.Rd
🚧 Files skipped from review as they are similar to previous changes (1)
- man/expdesServer.Rd
…ose response plot
man/run_tpr_simulation.Rd
Outdated
| run_tpr_simulation(rep_range, n_proteins = 1000) | ||
| } | ||
| \arguments{ | ||
| \item{rep_range}{integer vector of length 2, c(min, max) for replicate sweep} |
There was a problem hiding this comment.
Users should also pass in the actual dataset and the Protein ID that is considered a strong interaction.
R/module-expdes-server.R
Outdated
| CONC_MAP <- list( | ||
| "2" = c(0, 3000), | ||
| "3" = c(0, 1000, 3000), | ||
| "4" = c(0, 1, 1000, 3000), | ||
| "5" = c(0, 1, 100, 1000, 3000), | ||
| "6" = c(0, 1, 100, 300, 1000, 3000), | ||
| "7" = c(0, 1, 10, 100, 300, 1000, 3000), | ||
| "8" = c(0, 1, 10, 30, 100, 300, 1000, 3000), | ||
| "9" = c(0, 1, 3, 10, 30, 100, 300, 1000, 3000) | ||
| ) | ||
|
|
There was a problem hiding this comment.
As discussed with Sarah, this should not be hard coded but rather each subsequent value should be picked based on farthest distance from the log(median).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
R/module-expdes-server.R (2)
198-203: Consider handling empty interaction subsets gracefully.If
resultscontains no rows whereInteraction == "Strong"or"Weak", the resultingggplotwill render an empty chart. This won't crash but may confuse users.♻️ Optional: Add a check for empty data
+ strong_data <- results[results$Interaction == "Strong", ] + weak_data <- results[results$Interaction == "Weak", ] + + if (nrow(strong_data) == 0 || nrow(weak_data) == 0) { + plot.new() + text(0.5, 0.5, "Incomplete results: missing interaction categories.", + cex = 1.2) + dev.off() + return() + } + p_strong <- make_panel( - results[results$Interaction == "Strong", ], + strong_data, "Strong interaction detection power", "#1b9e77") p_weak <- make_panel( - results[results$Interaction == "Weak", ], + weak_data, "Weak interaction detection power", "#d95f02")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 198 - 203, When creating p_strong and p_weak from results, guard against empty subsets by checking results[results$Interaction == "Strong", ] and results[results$Interaction == "Weak", ] before passing to make_panel; if a subset is empty, call make_panel with a small placeholder data frame or set a flag to create a clear "no data" ggplot (or skip rendering) so the UI shows a meaningful message instead of an empty chart; update references where p_strong and p_weak are used to handle the placeholder/skip case.
112-156: Consider moving output definitions outside theobserve()block.Reassigning
output[[...]]insideobserve()on every reactive invalidation can cause flickering and unnecessary re-renders. A cleaner pattern is to define outputs once at the top level and put the conditional logic inside each render function:output[[NAMESPACE_EXPDES$result_plot]] <- renderPlotly({ req(!is_response_curve()) req(input$param) # ... existing logic })This is a minor structural improvement; the current implementation is functional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/module-expdes-server.R` around lines 112 - 156, The observe() currently reassigns output[[NAMESPACE_EXPDES$result_plot]] and output[[NAMESPACE_EXPDES$download_future]] on every invalidation; move those output definitions out of observe() and into top-level render functions (renderPlotly and downloadHandler) so they are defined once, and keep only enable/disable input logic inside the observe(). Inside the renderPlotly and downloadHandler use req(!is_response_curve()) and req(input$param) and replicate the sample_x/power_x selection logic (or better, expose it as a small reactive like future_exp() or compute it inside the render) before calling designSampleSize/designSampleSizePlots; ensure future_exp() is a reactive or local function used from the render functions so outputs no longer get reassigned inside observe().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@R/module-expdes-server.R`:
- Around line 198-203: When creating p_strong and p_weak from results, guard
against empty subsets by checking results[results$Interaction == "Strong", ] and
results[results$Interaction == "Weak", ] before passing to make_panel; if a
subset is empty, call make_panel with a small placeholder data frame or set a
flag to create a clear "no data" ggplot (or skip rendering) so the UI shows a
meaningful message instead of an empty chart; update references where p_strong
and p_weak are used to handle the placeholder/skip case.
- Around line 112-156: The observe() currently reassigns
output[[NAMESPACE_EXPDES$result_plot]] and
output[[NAMESPACE_EXPDES$download_future]] on every invalidation; move those
output definitions out of observe() and into top-level render functions
(renderPlotly and downloadHandler) so they are defined once, and keep only
enable/disable input logic inside the observe(). Inside the renderPlotly and
downloadHandler use req(!is_response_curve()) and req(input$param) and replicate
the sample_x/power_x selection logic (or better, expose it as a small reactive
like future_exp() or compute it inside the render) before calling
designSampleSize/designSampleSizePlots; ensure future_exp() is a reactive or
local function used from the render functions so outputs no longer get
reassigned inside observe().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e925a4a-9027-44c9-9c63-a6785c1e5511
📒 Files selected for processing (3)
NAMESPACER/MSstatsShiny.RR/module-expdes-server.R
✅ Files skipped from review due to trivial changes (1)
- R/MSstatsShiny.R
🚧 Files skipped from review as they are similar to previous changes (1)
- NAMESPACE
Motivation & Context
This PR integrates the sample size calculation feature for dose-response curves into MSstatsShiny, enabling power analysis for dose-response experiments through TPR (True Positive Rate) simulations. The feature allows users to simulate and visualize detection power across varying numbers of concentrations and replicates, with a maximum replicate limit of 5 per dose. The implementation removes the local TPR simulation function and imports it from the centralized MSstatsResponse package, and includes improved error handling for simulation failures and PDF generation.
Detailed Changes
Namespace & Dependencies
futureExperimentSimulation,run_tpr_simulation, andplot_tpr_power_curvefromMSstatsResponsepackageNAMESPACE_EXPDESconstant list containing six UI element namespace keys:sidebar_controls,protein_select,rep_range,run_simulation,result_plot, anddownload_futureModule Server Logic
preprocess_data = NULLandstatmodel_contrast = NULLprepared_response_data()reactive that merges preprocessed protein-level data with contrast matrix by "GROUP" and prepares it for dose-response fittingobserveEventwith modal spinner feedback, executingrun_tpr_simulation(rep_range=..., n_proteins=1000)Error Handling
UI Changes
uiOutputplaceholder for dynamic renderingNAMESPACE_EXPDESconstants for consistencyCross-Module Integration
contrastreactive via returned list, making contrast matrix accessible to downstream consumersstatmodel_contrastfromstatmodelServerand forward it toexpdesServerviacallModuleDocumentation
expdesServer.Rdto reflect new optional parameters and their descriptionsUnit Tests
No unit tests were added or modified to verify these changes. The existing test files remain unchanged, and no new test coverage was implemented for dose-response simulation, TPR functionality, or the updated module signatures.
Coding Guidelines Violations
rep_rangevalues or protein selection before passing torun_tpr_simulationprepared_response_data()assumes successful merge operation without explicit validation of merge results# ----section markers and standard#comments