feature(response-curve): Enable setting up metadata for response curves#133
feature(response-curve): Enable setting up metadata for response curves#133tonywu1999 merged 6 commits intodevelfrom
Conversation
WalkthroughAdds a new "response curve" comparison mode with constant, DESCRIPTION/NAMESPACE import of MSstatsResponse, UI radio option and a new response-curve panel, server matrix builder and routing, and tests for the panel and matrix function. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Shiny UI
participant Server as Shiny Server
participant MSR as MSstatsResponse
User->>UI: Select "Create response curves"
UI->>Server: request get_contrast_panel_ui
Server->>UI: build_response_curve_panel()
User->>UI: submit response-curve inputs
UI->>Server: submit inputs
Server->>Server: validate_contrast_inputs(mode=response_curve)
alt valid
Server->>Server: matrix_build() → build_response_curve_matrix
Server->>MSR: convertGroupToNumericDose(condition_list)
MSR-->>Server: numeric dose mapping
Server->>Server: assemble/append contrast$matrix (GROUP, drug, dose_nM)
Server-->>UI: return contrast matrix / success
else invalid
Server-->>UI: return validation errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
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: 1
🧹 Nitpick comments (2)
tests/testthat/test-utils-statmodel-server.R (1)
419-440: Consider notifying users when duplicates are silently dropped.The test correctly validates that duplicate Conditions are deduplicated by keeping the first occurrence. However, this silent behavior could confuse users who try to correct mistakes by re-entering the same Condition with updated X_axis or Amount values—their updates would be ignored without notification.
Consider adding a warning message when duplicates are detected to inform users that subsequent entries for the same Condition are being ignored.
R/module-statmodel-server.R (1)
233-233: Simplify deduplication syntax.The outer
rbind()call is unnecessary when filtering a data frame. The expressioncontrast$matrix[!duplicated(contrast$matrix$Condition),]already returns a data frame.Apply this diff to simplify:
- contrast$matrix = rbind(contrast$matrix[!duplicated(contrast$matrix$Condition),]) + contrast$matrix = contrast$matrix[!duplicated(contrast$matrix$Condition),]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
R/constants.R(1 hunks)R/module-statmodel-server.R(5 hunks)R/statmodel-ui-comparisons.R(2 hunks)R/statmodel-ui-headers.R(1 hunks)tests/testthat/test-statmodel-ui-options-contrasts.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 (13)
R/constants.R (1)
11-14: LGTM!The new constants follow the established naming conventions and integrate cleanly with the existing structure. The additions support the new response-curve comparison mode appropriately.
Also applies to: 21-22
R/statmodel-ui-headers.R (1)
31-32: LGTM!The updated header text clearly communicates the expanded functionality and accurately reflects the addition of the response curve analysis option.
tests/testthat/test-statmodel-ui-options-contrasts.R (1)
57-71: LGTM!The test thoroughly validates the response curve panel UI structure and follows the established testing patterns from the other panel tests in this file.
tests/testthat/test-utils-statmodel-server.R (3)
380-394: LGTM!The test correctly validates the initialization case when the contrast matrix is NULL, ensuring the function creates a properly structured data frame with the expected columns and values.
396-417: LGTM!The test properly verifies that new entries are appended to the existing response matrix with correct values.
442-461: LGTM!The test properly validates that multiple unique conditions with different X_axis values are all retained in the response matrix.
R/statmodel-ui-comparisons.R (3)
26-26: LGTM!Minor punctuation cleanup improves consistency in the tooltip text.
32-33: LGTM!The new "Create response curves" option is properly integrated into the radio button choices and correctly mapped to the new constant.
84-95: LGTM!The new
build_response_curve_panelfunction creates a clear and well-structured UI for configuring response curves. The placeholder text provides helpful guidance, and the button labels are descriptive.R/module-statmodel-server.R (4)
35-36: LGTM!The routing for the response curve panel is properly integrated into the
get_contrast_panel_uifunction and follows the established pattern for other comparison modes.
74-83: LGTM!The
render_response_curve_inputsfunction correctly renders the dynamic condition selector. The X_axis and Amount inputs are appropriately kept as static inputs in the UI panel since they don't depend on the condition list.
504-504: LGTM!The call to
render_response_curve_inputsis correctly placed during server initialization alongside the other input rendering functions.
547-549: LGTM!The response curve matrix building is properly integrated into the
matrix_buildevent handler and follows the established pattern for other comparison modes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/testthat/test-utils-statmodel-server.R (3)
396-417: LGTM: Append functionality test is correct.The test properly verifies that a new row is appended to an existing matrix. Consider optionally verifying that the first row remains unchanged to ensure append doesn't mutate existing data.
Optional enhancement:
expect_equal(nrow(result), 2) + expect_equal(result$GROUP[1], "Control") + expect_equal(result$Amount[1], 0) expect_equal(result$GROUP[2], "T5") expect_equal(result$X_axis[2], "Time") expect_equal(result$Amount[2], 5)
419-440: LGTM: Deduplication logic test is sound.The test correctly verifies that duplicate GROUP entries are removed while preserving the first occurrence. Consider optionally verifying that non-duplicate rows (T15) remain present to ensure deduplication doesn't affect other entries.
Optional enhancement:
expect_equal(nrow(result), 2) control_row <- result[result$GROUP == "Control", ] expect_equal(control_row$X_axis, "Time") expect_equal(control_row$Amount, 0) + expect_true("T15" %in% result$GROUP) + expect_equal(result$Amount[result$GROUP == "T15"], 15)
442-461: LGTM: Multiple x-axes test covers the heterogeneous case.The test verifies that different x-axis values can coexist across groups. Consider optionally verifying that each group retains its correct X_axis and Amount values to ensure the function doesn't inadvertently overwrite or mix data.
Optional enhancement:
expect_equal(nrow(result), 3) expect_true(all(c("Group_A", "Group_B", "Group_C") %in% result$GROUP)) + expect_equal(result$X_axis[result$GROUP == "Group_A"], "Temperature") + expect_equal(result$Amount[result$GROUP == "Group_A"], 25) + expect_equal(result$X_axis[result$GROUP == "Group_B"], "Pressure") + expect_equal(result$Amount[result$GROUP == "Group_B"], 100) + expect_equal(result$X_axis[result$GROUP == "Group_C"], "pH") + expect_equal(result$Amount[result$GROUP == "Group_C"], 7.5)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
R/module-statmodel-server.R(6 hunks)tests/testthat/test-utils-statmodel-server.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- R/module-statmodel-server.R
⏰ 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 (2)
tests/testthat/test-utils-statmodel-server.R (2)
380-394: LGTM: Initial matrix creation test is well-structured.The test correctly verifies that
build_response_curve_matrixcreates a single-row data frame with the expected columns (GROUP, X_axis, Amount) when starting from NULL.
380-461: Implementation and test structure verified; manual test execution required.The
build_response_curve_matrixfunction implementation exists at R/module-statmodel-server.R:228 and all three required constants (comparisons_response_curve_choice,comparisons_response_curve_xaxis,comparisons_response_curve_amount) are properly defined in R/constants.R. The function logic correctly handles the test scenarios: creating new matrices when null, appending rows to existing matrices, removing duplicates while preserving the first occurrence, and managing multiple unique x-axes.The test cases are well-structured and properly validate expected behavior. However, the test suite could not be executed in the verification environment due to the absence of R runtime. Please manually run the test suite to confirm all assertions pass:
library(testthat) test_file('tests/testthat/test-utils-statmodel-server.R', filter = 'build_response_curve_matrix')
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
R/module-statmodel-server.R (1)
228-233:build_response_curve_matrix()is simple and correct, but confirm GROUP label format & user messaging.The implementation is straightforward:
- Calls
convertGroupToNumericDose(condition_list), which (per MSstatsResponse docs) expects MSstats-style GROUP labels like"DMSO"or"DrugName_003uM"and returns a data frame with columnsdose_nM(numeric) anddrug(character).- Combines this with
GROUP = condition_listinto a single metadata table.Two follow-ups to consider:
Example dataset with conditions 1–6:
If the bundled example dataset encodes conditions simply as"1","2", … for timepoints, these labels may not parse into meaningfuldose_nM/drugvalues inconvertGroupToNumericDose, which can make the response-curve configuration feel opaque. It would be good to:
- Either ensure the example dataset’s GROUP/Condition levels follow the MSstatsResponse naming convention, or
- Add explicit help text/tooltips in the response-curve panel explaining how such simple labels are interpreted and how they map onto the X-axis of the curve.
Optional robustness check:
You might want to guard against unparseable GROUP levels (e.g., all-NAdose_nM) and surface a clearer message instead of silently returning a table withNAdoses, which could confuse users.For example:
build_response_curve_matrix = function(contrast, condition_list) { - condition_to_metadata_table = convertGroupToNumericDose(condition_list) - contrast$matrix = data.frame(GROUP = condition_list, condition_to_metadata_table) + condition_to_metadata_table = convertGroupToNumericDose(condition_list) + if (any(is.na(condition_to_metadata_table$dose_nM))) { + warning("Some GROUP levels could not be parsed into numeric doses; please check group naming or see the response-curve help text.") + } + contrast$matrix = data.frame(GROUP = condition_list, + condition_to_metadata_table) return(contrast$matrix) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
NAMESPACE(1 hunks)R/module-statmodel-server.R(5 hunks)R/statmodel-ui-comparisons.R(2 hunks)tests/testthat/test-statmodel-ui-options-contrasts.R(1 hunks)tests/testthat/test-utils-statmodel-server.R(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/testthat/test-utils-statmodel-server.R
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/testthat/test-statmodel-ui-options-contrasts.R
- R/statmodel-ui-comparisons.R
⏰ 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 (2)
R/module-statmodel-server.R (2)
35-36: New response-curve panel routing looks consistent.Wiring
comparison_mode_response_curvethroughget_contrast_panel_ui()tobuild_response_curve_panel(ns)matches the existing pattern for the other modes; no issues from a server-routing perspective.
100-109: Response-curve validation branch is aligned with existing patterns.The new
comparison_mode_response_curvebranch correctly centralizes basic checks:
X_axismust be non-NULL and non-empty (aftertrimws).Amountmust be non-NULL and non-NA.This mirrors the structure of the other contrast modes and addresses the earlier feedback about validating response-curve parameters before building the matrix.
| } else if (input[[NAMESPACE_STATMODEL$comparison_mode]] == CONSTANTS_STATMODEL$comparison_mode_response_curve) { | ||
| contrast$matrix = build_response_curve_matrix( | ||
| contrast, condition_list()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all usages of comparison_mode_response_curve
rg -n "comparison_mode_response_curve" RRepository: Vitek-Lab/MSstatsShiny
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Get context around line 35
echo "=== Context at line 35 ==="
sed -n '25,50p' R/module-statmodel-server.R
echo ""
echo "=== Context at line 100 ==="
sed -n '90,115p' R/module-statmodel-server.R
echo ""
echo "=== Context at line 542 (reviewed code) ==="
sed -n '535,555p' R/module-statmodel-server.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 3519
🏁 Script executed:
#!/bin/bash
# Find dataComparison function
echo "=== Searching for dataComparison function ==="
rg -n "dataComparison\s*=" R --type r
echo ""
echo "=== Searching for generate_analysis_code function ==="
rg -n "generate_analysis_code\s*=" R --type r
echo ""
echo "=== Searching for build_response_curve_matrix function ==="
rg -n "build_response_curve_matrix\s*=" R --type r
echo ""
echo "=== Searching for uses of contrast\$matrix ==="
rg -n "contrast\\\$matrix" R --type r | head -30Repository: Vitek-Lab/MSstatsShiny
Length of output: 2847
🏁 Script executed:
#!/bin/bash
# View build_response_curve_matrix function
echo "=== build_response_curve_matrix function ==="
sed -n '229,280p' R/module-statmodel-server.R
echo ""
echo "=== generate_analysis_code function ==="
sed -n '335,380p' R/module-statmodel-server.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 4438
🏁 Script executed:
#!/bin/bash
# Find where generate_analysis_code is called
echo "=== Calls to generate_analysis_code ==="
rg -n "generate_analysis_code" R --type r -B 2 -A 2
echo ""
echo "=== data_comparison variable usage ==="
rg -n "data_comparison" R --type r | head -20
echo ""
echo "=== MSstatsResponse usage ==="
rg -n "MSstatsResponse" R --type rRepository: Vitek-Lab/MSstatsShiny
Length of output: 2792
🏁 Script executed:
#!/bin/bash
# View the data_comparison and related code context
sed -n '550,610p' R/module-statmodel-server.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 2147
🏁 Script executed:
#!/bin/bash
# Search for dataComparison function definition
echo "=== Search for dataComparison function definition ==="
rg -n "^dataComparison\s*=" R --type r
echo ""
echo "=== Alternative search pattern ==="
rg -n "dataComparison.*function" R --type r
echo ""
echo "=== Look in separate files ==="
find R -name "*.R" -type f | xargs grep -l "dataComparison"Repository: Vitek-Lab/MSstatsShiny
Length of output: 375
🏁 Script executed:
#!/bin/bash
# View the dataComparison function
echo "=== dataComparison function ==="
sed -n '1212,1300p' R/utils.R
# Get file length to see how much more there is
echo ""
echo "=== File stats ==="
wc -l R/utils.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Find tmt_model and lf_model functions
echo "=== Search for tmt_model and lf_model ==="
rg -n "tmt_model\s*=" R --type r
echo ""
rg -n "lf_model\s*=" R --type r
echo ""
echo "=== Check if they're in utils.R ==="
grep -n "tmt_model\|lf_model" R/utils.R | head -20Repository: Vitek-Lab/MSstatsShiny
Length of output: 855
🏁 Script executed:
#!/bin/bash
# View lf_model function
echo "=== lf_model function (starting at line 304) ==="
sed -n '304,350p' R/main_calculations.R
echo ""
echo "=== tmt_model function (starting at line 411) ==="
sed -n '411,460p' R/main_calculations.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 3559
🏁 Script executed:
#!/bin/bash
# Search for TODO comments related to response curves
echo "=== TODO comments in module-statmodel-server.R ==="
grep -n "Todo\|TODO" R/module-statmodel-server.R
echo ""
echo "=== Response curve related comments ==="
grep -n "response\|dose" R/module-statmodel-server.R | grep -i "todo\|fixme\|hack"
echo ""
echo "=== Check create_group_comparison_plot for response curve handling ==="
sed -n '258,310p' R/module-statmodel-server.RRepository: Vitek-Lab/MSstatsShiny
Length of output: 3054
🏁 Script executed:
#!/bin/bash
# Check how the matrix is displayed in GUI
echo "=== Matrix display code (around line 578) ==="
sed -n '575,585p' R/module-statmodel-server.R
echo ""
echo "=== Check for any mode checks before calling analysis functions ==="
sed -n '559,572p' R/module-statmodel-server.R
echo ""
echo "=== Check build_response_curve_panel to understand UI flow ==="
rg -n "build_response_curve_panel" R --type r -A 10 | head -30Repository: Vitek-Lab/MSstatsShiny
Length of output: 2339
Response curve matrix incompatibility requires conditional handling in downstream analysis pipeline.
The response-curve matrix building at lines 542-544 produces a data.frame with columns GROUP, dose_nM, and other metadata (from convertGroupToNumericDose), not the numeric contrast matrix expected by MSstats/MSstatsTMT.
Currently, this data.frame is passed directly to dataComparison() (line 559), which calls lf_model() or tmt_model() without mode detection. Both functions pass the matrix to MSstats::MSstatsContrastMatrix() (lf_model line 321, tmt_model line 424), which expects a numeric matrix. This will fail or produce incorrect results.
Additionally:
generate_analysis_code()has a TODO comment (line 336) indicating response curves are not yet handled; the current iteration logic (lines 348-356) assumes a numeric matrix.create_group_comparison_plot()has a TODO (line 261) for dose response curve plotting and will not properly render results.- Six explicit TODOs throughout the module confirm the response curve feature is incomplete.
Add conditional mode checking before passing contrast$matrix to analysis and plotting functions, or implement the complete response curve pipeline. For the GUI, update the "Comparison matrix" panel text/tooltip to clarify that "Create response curves" displays dose/metadata mapping rather than classical contrast coefficients.
🤖 Prompt for AI Agents
In R/module-statmodel-server.R around lines 542-544, the code builds a
response-curve data.frame (GROUP, dose_nM, metadata) and assigns it to
contrast$matrix then passes it unchanged into the downstream analysis pipeline;
MSstats::MSstatsContrastMatrix expects a numeric contrast matrix so this will
break. Fix by adding a conditional branch: when
input[[NAMESPACE_STATMODEL$comparison_mode]] ==
CONSTANTS_STATMODEL$comparison_mode_response_curve, do not pass the
response-curve data.frame into dataComparison()/lf_model()/tmt_model(); instead
either (A) convert the response-curve mapping into the numeric contrast matrix
format required by MSstats::MSstatsContrastMatrix before assignment to
contrast$matrix, or (B) mark contrast$mode = "response_curve" and update
dataComparison(), lf_model(), tmt_model(), generate_analysis_code(), and
create_group_comparison_plot() to detect that mode and follow an alternate
response-curve analysis/rendering path (implementing the TODOs), and also update
the GUI "Comparison matrix" panel text/tooltip to state that "Create response
curves" yields a dose/metadata mapping rather than classical contrast
coefficients. Ensure all callers either receive a numeric matrix or explicitly
handle the response-curve mode to avoid passing a data.frame to
MSstatsContrastMatrix.
tonywu1999
force-pushed
the
feature-dose-response-comparisons
branch
from
7f1ba1c to
4ce844f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
R/module-statmodel-server.R (1)
521-523: Downstream pipeline incompatibility remains unaddressed.The wiring is correct, but as flagged in a previous review, the data.frame produced by
build_response_curve_matrix(containingGROUP,dose_nM, etc.) is passed todataComparison()at line 540, which ultimately callsMSstats::groupComparison()expecting a numeric contrast matrix. This will cause runtime failures when users select "Create response curves" and click calculate.The TODO comments throughout the file (lines 74, 214, 234, 240, 304, 315) confirm this feature path is incomplete.
Ensure the response curve analysis path is either:
- Disabled in the UI until the full pipeline is implemented, or
- Routed to a separate analysis function (e.g., from MSstatsResponse) that handles dose-response data appropriately
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
DESCRIPTION(1 hunks)NAMESPACE(1 hunks)R/constants.R(1 hunks)R/module-statmodel-server.R(3 hunks)R/statmodel-ui-comparisons.R(2 hunks)R/statmodel-ui-headers.R(1 hunks)tests/testthat/test-statmodel-ui-options-contrasts.R(1 hunks)tests/testthat/test-utils-statmodel-server.R(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- NAMESPACE
- tests/testthat/test-utils-statmodel-server.R
- tests/testthat/test-statmodel-ui-options-contrasts.R
- R/statmodel-ui-headers.R
- R/statmodel-ui-comparisons.R
🔇 Additional comments (3)
R/constants.R (1)
18-19: LGTM!The new constant follows the existing naming convention and is properly integrated.
R/module-statmodel-server.R (1)
35-36: LGTM!The routing logic correctly follows the existing pattern for other comparison modes.
DESCRIPTION (1)
30-30: MSstatsResponse is a valid Bioconductor dependency.MSstatsResponse is an active Bioconductor package (BioC 3.22) available from Bioconductor and r-universe. It is compatible with R >= 4.2 and the dependency addition is valid.
| #' @importFrom MSstatsResponse convertGroupToNumericDose | ||
| build_response_curve_matrix = function(contrast, condition_list) { | ||
| condition_to_metadata_table = convertGroupToNumericDose(condition_list) | ||
| contrast$matrix = data.frame(GROUP = condition_list, condition_to_metadata_table) | ||
| return(contrast$matrix) | ||
| } |
There was a problem hiding this comment.
Add error handling for convertGroupToNumericDose.
The call to convertGroupToNumericDose can fail if condition names don't follow expected patterns (e.g., no numeric dose extractable). Other builder functions validate inputs or handle edge cases gracefully.
Consider wrapping in a tryCatch to provide a user-friendly error message:
#' @importFrom MSstatsResponse convertGroupToNumericDose
build_response_curve_matrix = function(contrast, condition_list) {
- condition_to_metadata_table = convertGroupToNumericDose(condition_list)
- contrast$matrix = data.frame(GROUP = condition_list, condition_to_metadata_table)
+ tryCatch({
+ condition_to_metadata_table = convertGroupToNumericDose(condition_list)
+ contrast$matrix = data.frame(GROUP = condition_list, condition_to_metadata_table)
+ }, error = function(e) {
+ stop("Failed to extract dose information from condition names. ",
+ "Ensure conditions follow a pattern like 'Drug_100nM'. Error: ",
+ conditionMessage(e))
+ })
return(contrast$matrix)
}🤖 Prompt for AI Agents
In R/module-statmodel-server.R around lines 207 to 212, the call to
convertGroupToNumericDose can throw an error for unexpected condition names;
wrap the call in a tryCatch that captures errors and stops with a clear,
user-friendly message (e.g., mentioning which input condition_list failed and
suggesting valid formats), and ensure that on error you do not assign an invalid
value to contrast$matrix; on success proceed to set contrast$matrix as before
and return it.
1 participant
Summary by CodeRabbit
New Features
UI Updates
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.