Skip to content

Add robust parsing for temperature, time, and dosage for response curves#152

Merged
tonywu1999 merged 9 commits intodevelfrom
fix-response-bugs
Feb 10, 2026
Merged

Add robust parsing for temperature, time, and dosage for response curves#152
tonywu1999 merged 9 commits intodevelfrom
fix-response-bugs

Conversation

@tonywu1999
Copy link
Copy Markdown
Contributor

@tonywu1999 tonywu1999 commented Feb 9, 2026

Summary by CodeRabbit

  • Chores

    • Updated package dependencies to include additional data manipulation libraries.
  • Refactor

    • Streamlined internal dose-response fitting workflow for improved data handling and processing.
    • Updated UI label for dose-response selection from "Drug" to "Treatment" for clarity.
  • Tests

    • Expanded test coverage for dose-response matrix construction and data editing functionality.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This PR refactors the dose-response workflow by replacing the convertGroupToNumericDose-dependent build_response_curve_matrix with a data-driven implementation. It introduces prepare_dose_response_fit helper function, adds utility for applying matrix edits, extends package imports (stringr, dplyr, tidyr), and updates corresponding tests and server logic.

Changes

Cohort / File(s) Summary
Package Dependencies
DESCRIPTION, NAMESPACE, R/MSstatsShiny.R
Added stringr to DESCRIPTION imports. Extended importFrom statements in NAMESPACE and MSstatsShiny.R to include dplyr functions (bind_rows, case_when, n, n_distinct, select, summarise, ungroup), stringr functions (str_detect, str_extract, str_extract_all, str_trim), and tidyr (pivot_wider).
Dose-Response Refactoring
R/module-statmodel-server.R
Replaced build_response_curve_matrix implementation to derive GROUP column and is_control flag from group names instead of convertGroupToNumericDose; added prepare_dose_response_fit() function to standardize dose-response matrix schema; added update_matrix_from_edit() utility; updated render_group_comparison_plot_inputs and dose-response plot paths to use new helpers; changed UI label from "Select Drug" to "Select Treatment".
Utility Updates
R/utils.R
Replaced MSstatsPrepareDoseResponseFit call in fitResponseCurves with prepare_dose_response_fit; updated importFrom directive to remove MSstatsPrepareDoseResponseFit dependency.
Tests
tests/testthat/test-utils-statmodel-server.R
Added tests for build_response_curve_matrix with time-based and temperature-based scenarios; extended existing tests with stubbed showNotification; added tests for update_matrix_from_edit covering numeric and mixed-type data conversions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

Review effort 3/5

Suggested reviewers

  • sszvetecz

Poem

🐰 A dose of refactoring hops into view,
Data-driven parsing, now fresh and new!
GroupNames decoded, matrices reshaped with care,
prepare_dose_response_fit—elegance in the air! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately summarizes the main changes: adding robust parsing for temperature, time, and dosage parameters in response curve functionality, which is reflected in the build_response_curve_matrix refactoring and related updates throughout the codebase.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-response-bugs

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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 9, 2026

Failed to generate code suggestions for PR

Copy link
Copy Markdown

@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: 2

🤖 Fix all issues with AI agents
In `@R/module-statmodel-server.R`:
- Around line 261-287: The function prepare_dose_response_fit should validate
required columns before building cols_to_use: check that the expected response
column ("LogIntensities") and the expected protein column ("Protein") exist (or
explicitly allow a documented alternative), and if any are missing, stop() with
a clear error listing the missing columns rather than inserting NA and silently
dropping them; perform this validation after computing intervention_value but
before constructing cols_to_use/subset_df so downstream doseResponseFit gets a
predictable schema (refer to prepare_dose_response_fit, cols_to_use, subset_df,
and doseResponseFit).
- Around line 205-257: The build_response_curve_matrix function creates a
list-column measurements via str_extract_all which breaks downstream when groups
have multiple tokens; to fix, unnest measurements (use tidyr::unnest on the
measurements column) right after creating measurements so each row has a single
token, then extract numeric value and unit from that character column (use
str_extract on the unnested measurements) before pivot_wider (measurement_type,
value, unit); also remove or replace the direct UI call showNotification with a
returned warning value or a callback parameter so the data-processing function
remains UI-free and testable.
🧹 Nitpick comments (1)
tests/testthat/test-utils-statmodel-server.R (1)

380-420: Tests cover single-measurement groups well but lack multi-measurement coverage.

The tests validate the core scenarios (dose, time, temperature) but only for groups containing a single numeric+unit token. Consider adding a test for groups with multiple measurements (e.g., "Drug_10nM_24h") to verify the pivot_wider path and catch the list-column issue flagged in build_response_curve_matrix.

Comment thread R/module-statmodel-server.R
Comment thread R/module-statmodel-server.R
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant