Merge TPR simulation function into Development#6
Open
swaraj-neu wants to merge 2 commits intodevelfrom
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds TPR power-curve simulation and interactive plotting: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Runner as run_tpr_simulation
participant Simulator as futureExperimentSimulation
participant Plotter as plot_tpr_power_curve
participant GG as ggplot2
participant Plotly as plotly
User->>Runner: rep_range, n_proteins
loop for each (N_rep, NumConcs)
Runner->>Simulator: invoke simulation (params, template)
Simulator-->>Runner: Hit_Rates_Data
Runner->>Runner: filter TPR Strong/Weak, add N_rep & NumConcs
end
Runner-->>User: consolidated data.frame
User->>Plotter: simulation_results
Plotter->>GG: create Strong panel
GG-->>Plotter: ggplot (Strong)
Plotter->>GG: create Weak panel
GG-->>Plotter: ggplot (Weak)
Plotter->>Plotly: ggplotly + subplot(1x2)
Plotly-->>Plotter: interactive subplot
Plotter-->>User: plotly object (1×2)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (1)
R/TPR_Power_Curve.R (1)
34-46: Consider simplifying by removing the intermediatesim_argslist.The
sim_argslist is created and then immediately unpacked in the function call. This adds verbosity without benefit.♻️ Simplified version
run_one <- function(n_rep, k_conc, seed = 123) { set.seed(seed + n_rep * 100 + k_conc) concs_k <- CONC_MAP[[as.character(k_conc)]] - sim_args <- list( - N_proteins = n_proteins, - N_rep = n_rep, - Concentrations = concs_k, - IC50_Prediction = FALSE - ) - - temp_res <- futureExperimentSimulation( - N_proteins = sim_args$N_proteins, - N_rep = sim_args$N_rep, - Concentrations = sim_args$Concentrations, - IC50_Prediction = sim_args$IC50_Prediction - ) + temp_res <- futureExperimentSimulation( + N_proteins = n_proteins, + N_rep = n_rep, + Concentrations = concs_k, + IC50_Prediction = FALSE + ) temp_res$Hit_Rates_Data |>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/TPR_Power_Curve.R` around lines 34 - 46, The sim_args list is unnecessary boilerplate: remove the sim_args variable and call futureExperimentSimulation directly with the original variables (n_proteins, n_rep, concs_k, and FALSE) by passing them to the corresponding parameters N_proteins, N_rep, Concentrations, and IC50_Prediction in the futureExperimentSimulation call so you only use the function name futureExperimentSimulation and the original symbols n_proteins, n_rep, concs_k, and FALSE.
🤖 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/TPR_Power_Curve.R`:
- Around line 91-92: The linetype mapping currently uses a fixed vector ltypes
and assigns ltype_values <- setNames(ltypes[seq_along(rep_levels)],
as.character(rep_levels)), which will produce NAs when length(rep_levels) >
length(ltypes); change the assignment to repeat or recycle ltypes to cover all
replicate levels (e.g., generate a vector of length length(rep_levels) by
repeating ltypes with length.out = length(rep_levels) or using rep(ltypes,
length.out = length(rep_levels))) and then call setNames(...) so ltype_values
maps every element of rep_levels to a valid linetype; refer to the variables
ltypes, ltype_values and rep_levels when making the change.
- Around line 26-28: The function run_tpr_simulation currently assumes rep_range
is a two-element integer vector; add input validation at the top of
run_tpr_simulation to check that rep_range is a length-2 numeric (or integer)
vector with no NA/NaN/Inf values, both elements are whole numbers (or can be
safely coerced to integers), and rep_range[1] <= rep_range[2]; if any check
fails, stop with a clear error message mentioning rep_range and expected form
(e.g., "rep_range must be a length-2 integer vector with min <= max"). After
validation, coerce to integers (if needed) before creating rep_grid to avoid
downstream surprises when using rep_grid.
---
Nitpick comments:
In `@R/TPR_Power_Curve.R`:
- Around line 34-46: The sim_args list is unnecessary boilerplate: remove the
sim_args variable and call futureExperimentSimulation directly with the original
variables (n_proteins, n_rep, concs_k, and FALSE) by passing them to the
corresponding parameters N_proteins, N_rep, Concentrations, and IC50_Prediction
in the futureExperimentSimulation call so you only use the function name
futureExperimentSimulation and the original symbols n_proteins, n_rep, concs_k,
and FALSE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b0cfe25-885d-428f-890c-886741a75f85
📒 Files selected for processing (10)
DESCRIPTIONNAMESPACER/TPR_Power_Curve.Rman/ConvertGroupToNumericDose.Rdman/DoseResponseFit.Rdman/FutureExperimentSimulation.Rdman/PredictIC50Parallel.Rdman/VisualizeResponseProtein.Rdman/plot_tpr_power_curve.Rdman/run_tpr_simulation.Rd
💤 Files with no reviewable changes (5)
- man/ConvertGroupToNumericDose.Rd
- man/VisualizeResponseProtein.Rd
- man/PredictIC50Parallel.Rd
- man/DoseResponseFit.Rd
- man/FutureExperimentSimulation.Rd
Comment on lines
+26
to
+28
| run_tpr_simulation <- function(rep_range, n_proteins = 1000) { | ||
| k_grid <- as.integer(names(CONC_MAP)) | ||
| rep_grid <- seq(rep_range[1], rep_range[2]) |
There was a problem hiding this comment.
Add input validation for rep_range.
The function assumes rep_range is a valid integer vector of length 2 with min <= max, but doesn't validate this. Invalid input could cause confusing errors or unexpected behavior.
🛡️ Proposed fix to add validation
run_tpr_simulation <- function(rep_range, n_proteins = 1000) {
+ if (!is.numeric(rep_range) || length(rep_range) != 2) {
+ stop("rep_range must be an integer vector of length 2, e.g., c(2, 5).")
+ }
+ if (rep_range[1] > rep_range[2]) {
+ stop("rep_range[1] (min) must be <= rep_range[2] (max).")
+ }
k_grid <- as.integer(names(CONC_MAP))
rep_grid <- seq(rep_range[1], rep_range[2])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/TPR_Power_Curve.R` around lines 26 - 28, The function run_tpr_simulation
currently assumes rep_range is a two-element integer vector; add input
validation at the top of run_tpr_simulation to check that rep_range is a
length-2 numeric (or integer) vector with no NA/NaN/Inf values, both elements
are whole numbers (or can be safely coerced to integers), and rep_range[1] <=
rep_range[2]; if any check fails, stop with a clear error message mentioning
rep_range and expected form (e.g., "rep_range must be a length-2 integer vector
with min <= max"). After validation, coerce to integers (if needed) before
creating rep_grid to avoid downstream surprises when using rep_grid.
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.
Transfer the TPR simulation function from MSstatsShiny and export it for external use
Motivation and Context
This PR moves the True Positive Rate (TPR) simulation and interactive plotting functionality from MSstatsShiny into MSstatsResponse and exports it for external use. The goal is to provide standalone functions for sweeping experimental designs (varying concentrations and replicates) to estimate detection power (TPR) and to visualize results interactively with plotly.
Short solution summary: two new exported functions were added — run_tpr_simulation(rep_range, n_proteins = 1000) to run grid simulations across concentration counts (2–9) and replicate counts (bounded to max 5), and plot_tpr_power_curve(simulation_results) to produce a two-panel interactive plotly visualization (Strong / Weak interactions). Package metadata and NAMESPACE were updated and man pages for the new functions were added.
Detailed Changes
New R source
NAMESPACE
DESCRIPTION
Documentation (man/)
Documentation removals
Unit Tests
Recommendation: add unit tests for input validation, a small deterministic simulation mock (or stub futureExperimentSimulation) to validate output structure/contents, and a test for plot_tpr_power_curve requiring plotly (or using requireNamespace mocking).
Coding Guidelines / Policy Violations