Merge the Sample size calculation feature for dose response curve into Development#181
Open
swaraj-neu wants to merge 9 commits intodevelfrom
Open
Merge the Sample size calculation feature for dose response curve into Development#181swaraj-neu wants to merge 9 commits intodevelfrom
swaraj-neu wants to merge 9 commits intodevelfrom
Conversation
…ay crash or disconnect the Shiny session
|
Warning Rate limit exceeded
⌛ 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 (1)
📝 WalkthroughWalkthroughAdds dose-response simulation and plotting: new simulation and plotting functions, expdes module refactored for dual-mode operation, statmodel module now exposes contrast reactives, and namespace/constants updated for new UI IDs and imports. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Expdes UI
participant Server as Expdes Server
participant StatModel as StatModel Module
participant Sim as TPR Simulation
participant Plot as TPR Plotter
User->>UI: Select dose-response mode
UI->>Server: Render sidebar (dynamic uiOutput)
User->>UI: Select protein, set rep_range, click Run
UI->>Server: input$run_simulation event
Server->>StatModel: Request contrast (statmodel_contrast$matrix)
StatModel-->>Server: Return contrast reactive
Server->>Server: prepare_dose_response_fit(preprocess_data, contrast)
Server->>Sim: run_tpr_simulation(rep_range, n_proteins, prepared data)
Sim-->>Server: Return simulation_results (Interaction,TPR,N_rep,NumConcs)
Server->>Plot: plot_tpr_power_curve(simulation_results)
Plot-->>Server: Return Plotly subplot (Strong & Weak)
Server->>UI: Render result_plot (plotlyOutput)
UI-->>User: Display TPR power curves
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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: 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
Comment on lines
+218
to
+228
| 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This PR integrates sample size / power calculation for dose‑response curve experiments into MSstatsShiny by adding TPR simulation utilities and a dose‑response mode to the experimental design module. It enables users to simulate true positive rate (TPR) across grids of replicate counts and numbers of concentrations (dose levels) using MSstatsResponse::futureExperimentSimulation, and to visualize results via Plotly—helping researchers plan replicate and concentration schemes before running experiments.
Solution summary: add a hardcoded concentration ladder (CONC_MAP), expose run_tpr_simulation() and plot_tpr_power_curve(), extend the expdesServer/UI to support a dose‑response mode with dynamic sidebar controls and a “Run simulation” action, forward statmodel contrast data downstream, and improve error handling and user feedback during simulations.
Detailed changes
Namespace and imports
New constants
New exported functions and docs
expdesServer and UI changes
Module integration
Error handling and UX
Styling and visuals
Unit tests added or modified
Coding guidelines / potential issues violated or to review