feat(response-curve): Enable modeling and plotting of dose response curves#139
feat(response-curve): Enable modeling and plotting of dose response curves#139tonywu1999 merged 6 commits intodevelfrom
Conversation
WalkthroughAdds dose‑response curve support: new MSstatsResponse imports, constants and UI for response-curve selection, updated matrix builder signatures, a new fitResponseCurves utility that calls MSstatsResponse, and server plumbing to prepare data and render response‑curve plots. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Shiny UI
participant Server as Server Logic
participant Utils as fitResponseCurves
participant MSR as MSstatsResponse
User->>UI: choose "Dose Response Curve" and select protein/drug
UI->>Server: inputs (plot_type, protein, drug, condition_list)
Server->>Server: build_response_curve_matrix(condition_list)
Server->>Utils: fitResponseCurves(statmodel_input, matrix, data)
activate Utils
Utils->>MSR: MSstatsPrepareDoseResponseFit(prepared_data, colmap...)
MSR-->>Utils: prepared dose-response dataset
Utils->>MSR: doseResponseFit(prepared_data, params)
MSR-->>Utils: fitted response results
Utils-->>Server: list(ComparisonResult = results)
deactivate Utils
Server->>MSR: visualizeResponseProtein(results, protein, drug)
MSR-->>Server: plot object / rendering output
Server->>UI: render dose-response plot
UI->>User: display plot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
Failed to generate code suggestions for PR |
![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 (2)
R/utils.R (1)
1244-1244: Unused parameterstatmodel_input.The
statmodel_inputparameter is passed to the function but never used. Either remove it or document why it's needed for future use.-fitResponseCurves <- function(statmodel_input, matrix, input_data) { +fitResponseCurves <- function(matrix, input_data) {R/module-statmodel-server.R (1)
266-282: TODO comment should be addressed or tracked.Line 268-269 contains a TODO comment indicating that
build_response_curve_matrix(condition_list())$drugshould be changed to use the drug column from a user-defined matrix. This suggests the current implementation may not handle custom matrix configurations correctly.Would you like me to open an issue to track implementing user-defined matrix support for the drug column selection?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
NAMESPACE(1 hunks)R/constants.R(2 hunks)R/module-statmodel-server.R(12 hunks)R/statmodel-ui-options-visualization.R(3 hunks)R/utils.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
R/statmodel-ui-options-visualization.R (2)
27-28: LGTM!The addition of "Response Curve" to the plot type selector is properly integrated with the corresponding constant
plot_type_response_curve.
101-109: LGTM!The
create_response_curve_optionsfunction follows the established pattern of other option functions and correctly references the namespace constants for protein and drug selection.R/constants.R (1)
18-18: LGTM!The new constants for response curve visualization are properly added and follow the existing naming conventions. The namespace and constant additions align with their usage in the UI and server modules.
Also applies to: 26-26, 39-40
R/module-statmodel-server.R (2)
208-211: LGTM!The simplified
build_response_curve_matrixfunction correctly returns a data.frame with GROUP and metadata columns derived fromconvertGroupToNumericDose.
595-600: VerifyfitResponseCurvesreceives expected parameters.The call passes
inputasstatmodel_input, but as noted in R/utils.R, this parameter is unused. If the unused parameter is intentional for future use, consider documenting this; otherwise, align the call with the simplified signature.NAMESPACE (1)
51-54: MSstatsResponse is properly declared as a dependency.The package is listed in the
DESCRIPTIONfile (line 30) underImports, so the importFrom directives in the NAMESPACE file are backed by a proper dependency declaration and will not cause installation issues.
| } else if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] == | ||
| CONSTANTS_STATMODEL$plot_type_response_curve) { | ||
| matrix = matrix_build() | ||
| protein_level_data <- merge(preprocess_data()$ProteinLevelData, matrix, by = "GROUP") | ||
| dia_prepared <- MSstatsPrepareDoseResponseFit( | ||
| data = protein_level_data, | ||
| dose_column = "dose_nM", | ||
| drug_column = "drug", | ||
| protein_column = "Protein", | ||
| log_abundance_column = "LogIntensities", | ||
| transform_nM_to_M = TRUE | ||
| ) | ||
| output$comp_plots = renderPlot({ | ||
| visualizeResponseProtein( | ||
| data = dia_prepared, | ||
| protein_name = input[[NAMESPACE_STATMODEL$visualization_which_protein]], | ||
| drug_name = input[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]], | ||
| ratio_response = TRUE, | ||
| show_ic50 = TRUE, | ||
| add_ci = TRUE, | ||
| transform_dose = TRUE, | ||
| n_samples = 1000, | ||
| increasing = FALSE | ||
| ) | ||
| }) | ||
| op = plotOutput(ns("comp_plots")) |
There was a problem hiding this comment.
Code duplication and inconsistent parameter usage between fitting and visualization.
The MSstatsPrepareDoseResponseFit call here (lines 649-656) duplicates the logic in fitResponseCurves (R/utils.R lines 1246-1253). Additionally, ratio_response = TRUE at line 662 differs from ratio_response = FALSE in doseResponseFit (utils.R line 1258), which may cause visualization/fit mismatches.
Consider extracting the prepared data from data_comparison() or creating a shared helper to ensure consistency.
} else if (input[[NAMESPACE_STATMODEL$visualization_plot_type]] ==
CONSTANTS_STATMODEL$plot_type_response_curve) {
- matrix = matrix_build()
- protein_level_data <- merge(preprocess_data()$ProteinLevelData, matrix, by = "GROUP")
- dia_prepared <- MSstatsPrepareDoseResponseFit(
- data = protein_level_data,
- dose_column = "dose_nM",
- drug_column = "drug",
- protein_column = "Protein",
- log_abundance_column = "LogIntensities",
- transform_nM_to_M = TRUE
- )
+ # Reuse the prepared data from data_comparison() to ensure consistency
+ dia_prepared <- data_comparison()$PreparedData # Requires fitResponseCurves to return this
output$comp_plots = renderPlot({
visualizeResponseProtein(
data = dia_prepared,
protein_name = input[[NAMESPACE_STATMODEL$visualization_which_protein]],
drug_name = input[[NAMESPACE_STATMODEL$visualization_response_curve_which_drug]],
- ratio_response = TRUE,
+ ratio_response = FALSE, # Match doseResponseFit parameter
show_ic50 = TRUE,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In R/module-statmodel-server.R around lines 645 to 670, the
MSstatsPrepareDoseResponseFit call duplicates logic from fitResponseCurves
(R/utils.R lines ~1246-1253) and uses ratio_response = TRUE which conflicts with
doseResponseFit's ratio_response = FALSE; replace the inline preparation by
reusing the same prepared object used by fitting (e.g., pull the prepared data
from data_comparison() or call the shared helper used by fitResponseCurves),
remove the duplicated MSstatsPrepareDoseResponseFit block, and ensure the
visualization call uses the same ratio_response value and other parameters as
the fitting pipeline so fit and plot are consistent.
| protein_level_data <- merge(input_data$ProteinLevelData, matrix, by = "GROUP") | ||
| dia_prepared <- MSstatsPrepareDoseResponseFit( | ||
| data = protein_level_data, | ||
| dose_column = "dose_nM", | ||
| drug_column = "drug", | ||
| protein_column = "Protein", | ||
| log_abundance_column = "LogIntensities", | ||
| transform_nM_to_M = TRUE | ||
| ) | ||
| response_results <- doseResponseFit( | ||
| data = dia_prepared, | ||
| increasing = FALSE, | ||
| transform_dose = TRUE, | ||
| ratio_response = FALSE | ||
| ) | ||
| return(list(ComparisonResult = response_results)) |
There was a problem hiding this comment.
Hardcoded column names may cause runtime errors with different data formats.
The column names "dose_nM", "drug", "Protein", and "LogIntensities" are hardcoded. If user data doesn't contain these exact columns after the merge, MSstatsPrepareDoseResponseFit will fail. Consider adding validation or making column names configurable.
Additionally, there's no error handling if the merge produces an empty result or if required columns are missing:
fitResponseCurves <- function(statmodel_input, matrix, input_data) {
protein_level_data <- merge(input_data$ProteinLevelData, matrix, by = "GROUP")
+
+ required_cols <- c("dose_nM", "drug", "Protein", "LogIntensities")
+ missing_cols <- setdiff(required_cols, colnames(protein_level_data))
+ if (length(missing_cols) > 0) {
+ stop("Missing required columns after merge: ", paste(missing_cols, collapse = ", "))
+ }
+
dia_prepared <- MSstatsPrepareDoseResponseFit(
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/testthat/test-utils-statmodel-server.R (1)
380-391: Consider slightly strengthening thebuild_response_curve_matrixtestUsing the new
build_response_curve_matrix(condition_list)signature looks correct, and the current expectations guard basic structure, but they won’t catch regressions in the returned object type or column types.You could add a couple of low‑assumption checks, e.g. that the result is a data.frame and that
dose_nMis numeric:expect_equal(nrow(result), 3) expect_equal(ncol(result), 3) expect_true("GROUP" %in% colnames(result)) expect_true("drug" %in% colnames(result)) expect_true("dose_nM" %in% colnames(result)) + expect_s3_class(result, "data.frame") + expect_true(is.numeric(result$dose_nM))This keeps the test aligned with the new API while giving you earlier signal if the underlying representation changes unexpectedly.
tests/testthat/test-statmodel-ui-options-visualization.R (1)
28-34: Updated namespace expectation is correct; extend tests for the new response‑curve UISwitching the comparison plot test to
NAMESPACE_STATMODEL$visualization_which_proteinmatches the new shared ID in the visualization options module.Given you’ve added a
"Dose Response Curve"plot type andcreate_response_curve_options(), it would be good to expand this test file to cover those as well. For example:
- Assert that the new plot type label is present:
test_that("All possible options in create_plot_type_selector", { ns <- NS("test_module") result <- create_plot_type_selector(ns) ui_html <- htmltools::renderTags(result)$html expect_true(grepl("Volcano Plot", ui_html), info = "Volcano Plot option should be present") expect_true(grepl("Heatmap", ui_html), info = "Heatmap option should be present") expect_true(grepl("Comparison Plot", ui_html), info = "Comparison Plot should be present") + expect_true(grepl("Dose Response Curve", ui_html), + info = "Dose Response Curve option should be present") })
- Add a focused test for the response‑curve options UI:
test_that("Correct elements are present in create_comparison_plot_options", { ns <- NS("test_module") result <- create_comparison_plot_options(ns) ui_html <- htmltools::renderTags(result)$html expect_true(grepl(NAMESPACE_STATMODEL$visualization_which_protein, ui_html), info = "Which protein namespace should be present") }) + +test_that("Correct elements are present in create_response_curve_options", { + ns <- NS("test_module") + result <- create_response_curve_options(ns) + ui_html <- htmltools::renderTags(result)$html + expect_true(grepl(NAMESPACE_STATMODEL$visualization_which_protein, ui_html), + info = "Which protein namespace should be present") + expect_true(grepl(NAMESPACE_STATMODEL$visualization_response_curve_which_drug, ui_html), + info = "Which drug namespace should be present") +})That will lock in the new feature’s UI contract and catch accidental ID/name changes.
Also applies to: 51-62
R/statmodel-ui-options-visualization.R (1)
24-32: New plot type and response‑curve options are wired cleanly; add tests to guard the new IDsThe additions here look consistent and low‑risk:
create_plot_type_selector()now exposes"Comparison Plot"and"Dose Response Curve"via the correspondingCONSTANTS_STATMODEL$plot_type_*values, keeping the logic in terms of constants.create_comparison_plot_options()andcreate_response_curve_options()both use the sharedvisualization_which_proteinID, which matches the updated namespace used in tests and keeps the server side from having to handle multiple protein‑selector IDs.create_response_curve_options()introducesvisualization_response_curve_which_drug, giving a clear separation between protein and drug selection.To make this robust against regressions, I’d rely on tests like the ones suggested in
tests/testthat/test-statmodel-ui-options-visualization.R(checking for the"Dose Response Curve"label and bothvisualization_which_proteinandvisualization_response_curve_which_drugin the rendered HTML). That keeps the UI contract enforced without changing the implementation here.Also applies to: 63-67, 102-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
R/statmodel-ui-comparisons.R(1 hunks)R/statmodel-ui-options-visualization.R(3 hunks)tests/testthat/test-statmodel-ui-options-visualization.R(1 hunks)tests/testthat/test-utils-statmodel-server.R(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
R/statmodel-ui-comparisons.R (1)
28-34: Radio label wording change looks goodSwitching the label to
"Create dose response curves"keeps behavior identical while making the intent clearer and consistent with the “Dose Response Curve” plot type elsewhere in the UI.
1 participant
Summary by CodeRabbit
New Features
UI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.