Skip to content

Merge the Sample size calculation feature for dose response curve into Development#181

Open
swaraj-neu wants to merge 10 commits intodevelfrom
feat/dose-response-curve-full
Open

Merge the Sample size calculation feature for dose response curve into Development#181
swaraj-neu wants to merge 10 commits intodevelfrom
feat/dose-response-curve-full

Conversation

@swaraj-neu
Copy link
Contributor

@swaraj-neu swaraj-neu commented Mar 19, 2026

  • Integrate the sample size calculation into MSstatsShiny
  • Adjust the max replicates per dose to 5 in future experiments
  • Update the TPR simulation docs
  • Add try-catch block in TRP simulation function for better error handling

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

  • Added import binding for futureExperimentSimulation, run_tpr_simulation, and plot_tpr_power_curve from MSstatsResponse package
  • Created new NAMESPACE_EXPDES constant list containing six UI element namespace keys: sidebar_controls, protein_select, rep_range, run_simulation, result_plot, and download_future

Module Server Logic

  • expdesServer function signature extended with two optional parameters: preprocess_data = NULL and statmodel_contrast = NULL
  • Introduced dual-mode conditional rendering based on analysis type (dose-response vs. standard sample size):
    • Dose-response mode: Renders protein selector, replicate range slider (1-5 replicates), and "Run simulation" button
    • Standard mode: Retains legacy sample size/power estimation controls
  • Added prepared_response_data() reactive that merges preprocessed protein-level data with contrast matrix by "GROUP" and prepares it for dose-response fitting
  • TPR simulation triggered via observeEvent with modal spinner feedback, executing run_tpr_simulation(rep_range=..., n_proteins=1000)
  • PDF download handler for dose-response results generates two panels ("Strong" and "Weak" interaction detection power) with line plots showing TPR vs. number of concentrations across replicate levels

Error Handling

  • Try-catch block added around protein list loading with user-facing warning notification on failure
  • Try-catch block added around TPR simulation execution with error message displayed via notification (8-second duration)
  • PDF download handler includes fallback content ("No simulation results. Please run the simulation first.") when results are null, preventing empty PDF generation

UI Changes

  • Removed hardcoded parameter controls from sidebar; replaced with single uiOutput placeholder for dynamic rendering
  • Simplified informational text by removing redundant TMT incompatibility paragraph
  • Changed output element references from module-local identifiers to NAMESPACE_EXPDES constants for consistency

Cross-Module Integration

  • statmodelServer updated to export contrast reactive via returned list, making contrast matrix accessible to downstream consumers
  • main server.R modified to extract statmodel_contrast from statmodelServer and forward it to expdesServer via callModule

Documentation

  • Updated roxygen2 documentation in expdesServer.Rd to reflect new optional parameters and their descriptions

Unit 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

  • Hardcoded magic values: Maximum replicates set to 5, PDF dimensions (16×6 inches), slider value ranges, and color codes ("#1b9e77" for strong interaction, "#d95f02" for weak interaction) are hardcoded rather than defined as constants
  • Inline UI strings: UI labels and titles ("Dose response power analysis", "Select protein (strong interaction)", "Replicates per dose") are embedded in code rather than externalized
  • Missing input validation: No explicit validation of rep_range values or protein selection before passing to run_tpr_simulation
  • Missing null/NA checks: prepared_response_data() assumes successful merge operation without explicit validation of merge results
  • Inconsistent comment formatting: Mix of # ---- section markers and standard # comments

@swaraj-neu swaraj-neu requested a review from tonywu1999 March 19, 2026 21:06
@swaraj-neu swaraj-neu self-assigned this Mar 19, 2026
@swaraj-neu swaraj-neu added the enhancement New feature or request label Mar 19, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026

📝 Walkthrough

Walkthrough

This 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

Cohort / File(s) Summary
Namespace & Imports
NAMESPACE, R/MSstatsShiny.R
Added imports for dose-response simulation functions (futureExperimentSimulation, run_tpr_simulation, plot_tpr_power_curve) from MSstatsResponse package.
Constants Definition
R/constants.R
Introduced new NAMESPACE_EXPDES constant list with six string mappings for sidebar and output control identifiers (sidebar_controls, protein_select, rep_range, run_simulation, result_plot, download_future).
Experiment Design Module
R/module-expdes-server.R, R/module-expdes-ui.R
Refactored expdesServer to accept preprocess_data and statmodel_contrast parameters; added conditional response-curve mode that renders protein selector, replicate-range slider, and runs dose-response simulations; retained standard mode for non-response-curve analyses. UI simplified to use dynamic uiOutput placeholders instead of hard-coded controls.
Contrast Propagation
R/module-statmodel-server.R, R/server.R
Updated statmodelServer to expose the reactive contrast object in its return value; modified server to extract and forward statmodel_contrast to expdesServer via callModule.
Documentation
man/expdesServer.Rd
Updated function signature and arguments documentation to reflect new optional parameters and clarified module description.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • PR #155: Introduces helper functions (prepare_dose_response_fit, build_response_curve_matrix) and statmodel-server refactors that provide foundational support for the dose-response integration added in this PR.
  • PR #152: Implements prepare_dose_response_fit function that is directly invoked in the new response-curve mode logic within expdesServer.
  • PR #180: Modifies the same code paths for dose-response support, including NAMESPACE imports, NAMESPACE_EXPDES constants, and expdesServer signature expansion.

Suggested labels

Review effort 3/5

Suggested reviewers

  • sszvetecz

Poem

🐰 A hop through the data streams, so grand,
Response curves plotted across the land,
With doses and powers in simulation's flow,
New futures experiment, watch them grow!
Contrasts and proteins dance side by side,

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main objective of integrating sample size calculation for dose-response curves into the development branch, which aligns with the primary changes across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dose-response-curve-full

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
R/module-statmodel-server.R (1)

279-283: Prefer returning a read-only contrast accessor.

contrast is a mutable reactiveValues, so exposing it here leaks internal state (matrix and row) across the module boundary. From the wiring in this PR, the downstream consumer only needs the matrix value, so returning reactive(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 in plot_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

📥 Commits

Reviewing files that changed from the base of the PR and between c2a30aa and eb85531.

📒 Files selected for processing (10)
  • NAMESPACE
  • R/MSstatsShiny.R
  • R/constants.R
  • R/module-expdes-server.R
  • R/module-expdes-ui.R
  • R/module-statmodel-server.R
  • R/server.R
  • man/expdesServer.Rd
  • man/plot_tpr_power_curve.Rd
  • man/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
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
R/module-expdes-server.R (3)

312-324: Consider extracting shared make_panel helper.

This make_panel function (lines 312-324) duplicates logic from the one in plot_tpr_power_curve (lines 88-110). A shared helper accepting both title and show_legend parameters 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 refactoring future_exp to a reactive.

Defining future_exp as a local function inside observe() 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_exp as 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 ltypes vector 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 include NA values, 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

📥 Commits

Reviewing files that changed from the base of the PR and between eb85531 and e190633.

📒 Files selected for processing (2)
  • R/module-expdes-server.R
  • man/expdesServer.Rd
🚧 Files skipped from review as they are similar to previous changes (1)
  • man/expdesServer.Rd

@swaraj-neu swaraj-neu linked an issue Mar 19, 2026 that may be closed by this pull request
run_tpr_simulation(rep_range, n_proteins = 1000)
}
\arguments{
\item{rep_range}{integer vector of length 2, c(min, max) for replicate sweep}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users should also pass in the actual dataset and the Protein ID that is considered a strong interaction.

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)
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
R/module-expdes-server.R (2)

198-203: Consider handling empty interaction subsets gracefully.

If results contains no rows where Interaction == "Strong" or "Weak", the resulting ggplot will 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 the observe() block.

Reassigning output[[...]] inside observe() 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

📥 Commits

Reviewing files that changed from the base of the PR and between e190633 and 90561c3.

📒 Files selected for processing (3)
  • NAMESPACE
  • R/MSstatsShiny.R
  • R/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Sample-Size] Integrate sample size calculation into MSstatsShiny

2 participants