fix(annotation): Enable fractions in label free experiments#118
fix(annotation): Enable fractions in label free experiments#118tonywu1999 merged 2 commits intodevelfrom
Conversation
WalkthroughUpdated .checkAnnotation to include "Fraction" for LF annotations and to error on unsupported label_type values. Added a comprehensive tinytest suite covering valid/invalid LF and TMT annotation schemas, missing columns, extra columns, empty data, and unknown label_type handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Check as .checkAnnotation
Caller->>Check: validate(annotation, label_type)
alt label_type == "LF"
Note over Check: Build allowed columns incl. Fraction
Check->>Caller: pass or error (missing/extra cols)
else label_type == "TMT"
Note over Check: Build allowed TMT columns
Check->>Caller: pass or error (missing/extra cols)
else Invalid label_type
Note over Check: Early stop with message<br/>"Labeling type must be either LF or TMT"
Check-->>Caller: error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
R/utils_checks.R (2)
12-21: Optional: normalize label_type to be case-insensitive and reduce duplicationMinor hardening: normalize label_type once (toupper) and branch on the normalized value. Keeps behavior, accepts "lf"/"tmt", and avoids repeated string literals.
Apply this localized refactor:
- if (label_type == "LF"){ + label_type_norm <- toupper(label_type) + if (label_type_norm == "LF"){ max_columns = c("Run", "Raw.file", "Condition", "BioReplicate", "IsotopeLabelType", "Fraction") - } else if (label_type == "TMT"){ + } else if (label_type_norm == "TMT"){ max_columns = c("Run", "Raw.file", "Fraction", "TechRepMixture", "Channel", "Condition", "Mixture", "BioReplicate") } else { - msg = paste("Labeling type must be either LF or TMT") - stop(msg) + stop("Labeling type must be either LF or TMT") }
19-20: Nit: avoid paste() for a static stringSlight cleanup; paste() is unnecessary for a constant.
- msg = paste("Labeling type must be either LF or TMT") + msg = "Labeling type must be either LF or TMT"inst/tinytest/test_utils_checks.R (2)
87-104: Avoid brittle exact-string matching of allowed-column listsAsserting the entire ordered list ties tests to a specific ordering of allowed columns. If order changes (semantically irrelevant), tests will fail.
Consider either:
- Matching key tokens with looser patterns, e.g., separately asserting presence of “Fraction”, “IsotopeLabelType”, etc., or
- Extracting the tail of the message and comparing sets (order-insensitive) by splitting on ", ".
Example approach (order-insensitive):
err <- tryCatch(MSstatsPTM:::.checkAnnotation(annotation_lf_extra, "LF"), error = identity) allowed <- strsplit(sub(".*only include the following columns in the annotation file:\\s*", "", conditionMessage(err)), ",\\s*")[[1]] tinytest::expect_true(all(c("Run","Raw.file","Condition","BioReplicate","IsotopeLabelType","Fraction") %in% allowed))Also applies to: 118-123
154-156: Optional: add case-insensitive label_type tests if adopting normalizationIf you implement the case-insensitive label_type normalization, add complementary tests to lock it in.
Example additions:
expect_silent(MSstatsPTM:::.checkAnnotation(annotation_basic, "lf")) expect_silent(MSstatsPTM:::.checkAnnotation(annotation_basic, "tmt"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
R/utils_checks.R(1 hunks)inst/tinytest/test_utils_checks.R(1 hunks)
🔇 Additional comments (6)
R/utils_checks.R (2)
12-15: LF: Allowing Fraction column is correct and unblocks fractionated LF workflowsIncluding "Fraction" in the LF allowed set fixes the previous omission and aligns LF with common fractionated runs.
18-21: Fail-fast on unsupported label_type is a good guardrailEarly stop with a clear message prevents downstream use of an undefined max_columns.
inst/tinytest/test_utils_checks.R (4)
1-17: Good negative-path coverage for missing Run/Raw.fileThese assert the primary precondition and validate the exact error path.
43-54: Solid: Confirms LF now permits FractionThis directly exercises the LF+Fraction allowance and will catch any regression.
132-147: Nice: Minimal valid schemas covered (only Run or only Raw.file) for both LF and TMTThis ensures subsets are accepted as intended.
157-161: Unknown label_type test validates new guardConfirms the explicit error path and message text.
tonywu1999
changed the title
1 participant
Motivation and Context
we need to enable fractionation experiments with MSstatsPTM with label free https://groups.google.com/g/msstats/c/G9US-6V8uyM
Testing
Checklist Before Requesting a Review
Summary by CodeRabbit