fix(vignette): Turn off nM to M conversion in vignette#8
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 (8)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new exported function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
🧹 Nitpick comments (1)
vignettes/MSstatsResponse.Rmd (1)
157-166: Document thetransform_nM_to_Mparameter choice and its implications.The vignette changes
transform_nM_to_MfromTRUEtoFALSE, but provides no explanation of what this parameter does or whyFALSEis chosen. Since doses are in nanomolar (column nameddose_nM, line 161) and remain in nM throughout the analysis, downstream IC50 predictions will also be reported in nM rather than molar units. This choice is appropriate given thattransform_dose = TRUE(line 202) applieslog10(dose + 1)transformation, which behaves more sensibly with nM-scale values (e.g., log10(1001) ≈ 3) than with molar-scale values (e.g., log10(10^-6 + 1) ≈ 0).Consider adding explanatory text in the prose (around lines 151-156) or an inline comment to clarify:
- What
transform_nM_to_Mcontrols (unit conversion from nM to M)- Why
FALSEis appropriate for this dataset (doses already in nM, more intuitive IC50 values)- That IC50 estimates (section starting line 245) will be in nM units
📝 Suggested documentation addition
Add explanatory text before the code block:
This function standardizes the data structure for downstream analysis. Users can either use pre-processed data as shown above (e.g., dia_normalized$ProteinLevelData) or begin at this step with their own datasets. It is important to ensure that column names are correctly matched to the expected ones when using MSstatsPrepareDoseResponseFit(). The dose_column corresponds to the values on the x-axis (e.g., dose, concentration, time, or temperature). The protein_column should contain unique identifiers (e.g., protein, gene, or peptide sequence), and the log_abundance_column represents the response values on the y-axis (e.g., log intensity, expression, or growth). + +**Unit handling:** Since our doses are already in nanomolar units (`dose_nM`), we set `transform_nM_to_M = FALSE` to keep doses on the nanomolar scale throughout the analysis. This ensures that downstream IC50 estimates are reported in nanomolar units, which are more intuitive for typical drug concentrations. If your doses were in molar units or you prefer molar-scale outputs, set this parameter to `TRUE`.Or add an inline comment:
transform_nM_to_M = FALSE + # Keep doses in nM; IC50 estimates will be in nM (not converted to M) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vignettes/MSstatsResponse.Rmd` around lines 157 - 166, Document that the parameter transform_nM_to_M in MSstatsPrepareDoseResponseFit controls conversion of dose units from nanomolar to molar, and add a brief explanation near the MSstatsPrepareDoseResponseFit call (or as an inline comment) stating that transform_nM_to_M = FALSE is chosen because the input dose column "dose_nM" is already in nM so downstream results and IC50 estimates (later IC50 section) will be reported in nM; also mention that transform_dose = TRUE applies log10(dose + 1) and thus behaves more sensibly on nM-scale values, so leaving transform_nM_to_M = FALSE preserves intuitive nM-scale IC50s.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vignettes/MSstatsResponse.Rmd`:
- Around line 157-166: Document that the parameter transform_nM_to_M in
MSstatsPrepareDoseResponseFit controls conversion of dose units from nanomolar
to molar, and add a brief explanation near the MSstatsPrepareDoseResponseFit
call (or as an inline comment) stating that transform_nM_to_M = FALSE is chosen
because the input dose column "dose_nM" is already in nM so downstream results
and IC50 estimates (later IC50 section) will be reported in nM; also mention
that transform_dose = TRUE applies log10(dose + 1) and thus behaves more
sensibly on nM-scale values, so leaving transform_nM_to_M = FALSE preserves
intuitive nM-scale IC50s.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a29b6b95-90f0-410b-b205-bfb444830509
📒 Files selected for processing (1)
vignettes/MSstatsResponse.Rmd
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/calculatePeptideWeights.Rd`:
- Line 40: The documentation for light_intensity_score is incorrect: update the
`@return` section in the R source (where calculatePeptideWeights / related roxygen
block is defined) to describe that light_intensity_score is a binary flag (1/0)
determined by the peptide's median-light rank within its protein and the
top_n_peptides filter (1 when top_n_peptides is NULL or when rank <=
top_n_peptides, else 0), not a normalized median intensity; then run
devtools::document() to regenerate man/calculatePeptideWeights.Rd so the help
text matches the implementation in R/protein_turnover_ratio_helper.R (reference
symbols: light_intensity_score, top_n_peptides, calculatePeptideWeights).
- Around line 50-73: The examples in the man page use an undeclared dataset
feature_data; update the examples to either use a documented dataset (e.g.,
replace feature_data with DIA_MSstats_Normalized) or wrap the entire example
block in \dontrun{} so it won't be executed during R CMD check; update the
corresponding `@examples` in R/protein_turnover_ratio_helper.R to reflect this
change and re-run roxygenize to regenerate documentation for
calculatePeptideWeights, calculateTurnoverRatios, and doseResponseFit examples.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44362aed-f1b9-4467-b161-ea26fe7a4d5f
📒 Files selected for processing (4)
NAMESPACEman/calculatePeptideWeights.Rdman/doseResponseFit.Rdman/selectTopNPeptides.Rd
💤 Files with no reviewable changes (2)
- man/doseResponseFit.Rd
- man/selectTopNPeptides.Rd
| \itemize{ | ||
| \item n_obs: Number of observations for this peptide | ||
| \item coverage_score: Proportion of timepoints observed | ||
| \item light_intensity_score: Normalized median light intensity (per protein) |
There was a problem hiding this comment.
light_intensity_score description doesn't match the implementation.
The doc states light_intensity_score: Normalized median light intensity (per protein), but per the implementation in R/protein_turnover_ratio_helper.R, light_intensity_score is a binary 0/1 flag: 1 when top_n_peptides is NULL (no filtering), otherwise 1 if the peptide's median-light rank within its protein is <= top_n_peptides, else 0. It is not a normalized intensity value.
Since this is a roxygen2-generated file, please update the @return item in the R source and re-run devtools::document().
📝 Proposed wording
-\item light_intensity_score: Normalized median light intensity (per protein)
+\item light_intensity_score: Binary top-N flag per protein (1 if peptide's median light intensity rank <= top_n_peptides, else 0; always 1 when top_n_peptides is NULL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@man/calculatePeptideWeights.Rd` at line 40, The documentation for
light_intensity_score is incorrect: update the `@return` section in the R source
(where calculatePeptideWeights / related roxygen block is defined) to describe
that light_intensity_score is a binary flag (1/0) determined by the peptide's
median-light rank within its protein and the top_n_peptides filter (1 when
top_n_peptides is NULL or when rank <= top_n_peptides, else 0), not a normalized
median intensity; then run devtools::document() to regenerate
man/calculatePeptideWeights.Rd so the help text matches the implementation in
R/protein_turnover_ratio_helper.R (reference symbols: light_intensity_score,
top_n_peptides, calculatePeptideWeights).
1 participant
Motivation and Context
The vignette example for MSstatsPrepareDoseResponseFit was converting nanomolar dose values to molar units by default. This PR updates the vignette to stop performing that conversion (transform_nM_to_M = FALSE) when dose_nM is already present. The PR also introduces a new public utility calculatePeptideWeights, removes selectTopNPeptides from the public API/docs, and updates several documentation examples and IC50-related tests to reflect API/behavior changes.
Summary of the solution
Detailed changes
vignettes/MSstatsResponse.Rmd
NAMESPACE
New/updated documentation (man/*.Rd and R roxygen changes)
R/source changes (documentation/example guards)
Tests
Unit tests added or modified
Modified:
No new tests were added beyond the adjusted expectations and call-site changes described.
Coding guidelines