Merge the Sample size calculation feature for dose response curve into Development#181
Open
swaraj-neu wants to merge 12 commits intodevelfrom
Open
Merge the Sample size calculation feature for dose response curve into Development#181swaraj-neu wants to merge 12 commits intodevelfrom
swaraj-neu wants to merge 12 commits intodevelfrom
Conversation
…ay crash or disconnect the Shiny session
|
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 13 minutes and 21 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 (1)
📝 WalkthroughWalkthroughThis PR adds response-curve (dose-response) simulation support to the experiment-design module: imports MSstatsResponse simulation/plot functions, propagates the statmodel contrast to expdesServer, introduces conditional UI/server logic for response-curve mode, and adds constants for module IDs. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Sidebar UI (NAMESPACE_EXPDES)
participant Expdes as expdesServer
participant Prep as Data Preparation / preprocess_data
participant Stat as statmodelServer (contrast)
participant MSR as MSstatsResponse (run_tpr_simulation / plot_tpr_power_curve)
User->>UI: select protein, set rep range, click "Run simulation"
UI->>Expdes: trigger run (input)
Expdes->>Prep: request ProteinLevelData
Expdes->>Stat: read contrast matrix
Expdes->>Prep: merge ProteinLevelData + contrast
Expdes->>MSR: run_tpr_simulation(prepared_data, rep_range)
MSR-->>Expdes: simulation_results
Expdes->>MSR: plot_tpr_power_curve(simulation_results)
MSR-->>Expdes: power curve plot
Expdes-->>UI: display plot, enable download
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)
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
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
tonywu1999
reviewed
Mar 20, 2026
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} |
Contributor
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
Comment on lines
+7
to
+17
| 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) | ||
| ) | ||
|
|
Contributor
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
…ratilze the strong interaction protein dropdown label
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 65-67: Update the sliderInput for replicates in
R/module-expdes-server.R: change the max from 10 to 5 on the sliderInput call
using ns(NAMESPACE_EXPDES$rep_range) so the UI is capped at 5 replicates per
dose, and verify the default value vector (value = c(1, 5)) remains within the
new bounds; adjust the value if necessary.
- Around line 112-130: The early-return branch and the error handler currently
leave previous results in simulation_results(), causing stale plots/downloads;
update both places (the if block checking user_concs and the error = function(e)
handler) to explicitly clear the reactive by calling simulation_results(NULL)
before returning/after remove_modal_spinner(), so that any previous simulation
output is removed when a run fails or inputs are invalid; ensure you reference
the existing simulation_results() reactive and keep the remove_modal_spinner()
and showNotification(...) calls intact.
🪄 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: ea4219ea-1552-4951-b487-0a2ee663a04b
📒 Files selected for processing (2)
NAMESPACER/module-expdes-server.R
✅ Files skipped from review due to trivial changes (1)
- NAMESPACE
Comment on lines
+65
to
+67
| sliderInput(ns(NAMESPACE_EXPDES$rep_range), | ||
| "Replicates per dose", | ||
| min = 1, max = 10, value = c(1, 5), step = 1), |
There was a problem hiding this comment.
Cap the replicate range at 5 here.
Lines 65-67 still expose max = 10, so the UI can request runs that this PR says should be disallowed for future experiments.
🔧 Minimal fix
sliderInput(ns(NAMESPACE_EXPDES$rep_range),
"Replicates per dose",
- min = 1, max = 10, value = c(1, 5), step = 1),
+ min = 1, max = 5, value = c(1, 5), step = 1),📝 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.
Suggested change
| sliderInput(ns(NAMESPACE_EXPDES$rep_range), | |
| "Replicates per dose", | |
| min = 1, max = 10, value = c(1, 5), step = 1), | |
| sliderInput(ns(NAMESPACE_EXPDES$rep_range), | |
| "Replicates per dose", | |
| min = 1, max = 5, value = c(1, 5), step = 1), |
🤖 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 65 - 67, Update the sliderInput for
replicates in R/module-expdes-server.R: change the max from 10 to 5 on the
sliderInput call using ns(NAMESPACE_EXPDES$rep_range) so the UI is capped at 5
replicates per dose, and verify the default value vector (value = c(1, 5))
remains within the new bounds; adjust the value if necessary.
Comment on lines
+112
to
+130
| if (is.null(user_concs) || length(user_concs) < 2) { | ||
| showNotification("Could not extract concentrations from contrast matrix.", type = "error") | ||
| remove_modal_spinner() | ||
| return() | ||
| } | ||
|
|
||
| output$info = renderText({ | ||
| paste0( | ||
| "hover: ", MSstatsShiny::xy_str(input$plot_hover) | ||
| results <- run_tpr_simulation( | ||
| rep_range = input[[NAMESPACE_EXPDES$rep_range]], | ||
| concentrations = user_concs, | ||
| dose_range = c(2, length(user_concs)), | ||
| data = prepared_response_data(), | ||
| protein = input[[NAMESPACE_EXPDES$protein_select]], | ||
| n_proteins = 1000 | ||
| ) | ||
| simulation_results(results) | ||
| remove_modal_spinner() | ||
| }, error = function(e) { | ||
| remove_modal_spinner() | ||
| showNotification(conditionMessage(e), type = "error", duration = 8) |
There was a problem hiding this comment.
Clear stale simulation output on failed reruns.
If a previous run succeeded, the early-return path at Lines 112-116 and the error handler at Lines 128-130 leave simulation_results() unchanged. The plot and PDF download can then keep serving the old curve even though the latest run failed.
🧹 Minimal fix
if (is.null(user_concs) || length(user_concs) < 2) {
+ simulation_results(NULL)
showNotification("Could not extract concentrations from contrast matrix.", type = "error")
remove_modal_spinner()
return()
}
@@
}, error = function(e) {
+ simulation_results(NULL)
remove_modal_spinner()
showNotification(conditionMessage(e), type = "error", duration = 8)
})🤖 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 - 130, The early-return branch and
the error handler currently leave previous results in simulation_results(),
causing stale plots/downloads; update both places (the if block checking
user_concs and the error = function(e) handler) to explicitly clear the reactive
by calling simulation_results(NULL) before returning/after
remove_modal_spinner(), so that any previous simulation output is removed when a
run fails or inputs are invalid; ensure you reference the existing
simulation_results() reactive and keep the remove_modal_spinner() and
showNotification(...) calls intact.
…esults at the start of every run
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
2 participants
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 & Context
This change integrates a dose-response sample size / power calculation workflow into MSstatsShiny by reusing simulation utilities from the MSstatsResponse package. It adds a dose-response ("response curve") mode that lets users select a protein, choose replicates per dose (now capped at 5), run TPR simulations, and view/download power curves. The implementation centralizes the TPR simulation logic (imported from MSstatsResponse), improves error handling around simulations, and adapts the UI/server wiring to support both standard sample-size and dose-response workflows.
Detailed Changes
Namespace & imports
New constants
expdes module (server & UI)
statmodel module & server integration
Documentation
Minor UI text cleanup
Unit Tests
Coding Guidelines Violations