-
Notifications
You must be signed in to change notification settings - Fork 2
docs(fragpipe): Modify vignettes to include global profiling run example #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRoxygenNote in DESCRIPTION bumped. Examples, documentation, and vignette updated to show passing an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FragPipeConverter as FragPipetoMSstatsPTMFormat
participant OptionalProtein as input_protein (optional)
participant Output as msstats_data
note over User,FragPipeConverter #e6f7ff: Example-level interaction (API call)
User->>FragPipeConverter: call(input, annotation=NULL?, input_protein=?)
alt input_protein provided
FragPipeConverter->>OptionalProtein: read/use protein data
FragPipeConverter->>FragPipeConverter: merge PROTEIN into output
else no input_protein
FragPipeConverter->>FragPipeConverter: leave PROTEIN NULL or absent
end
FragPipeConverter->>Output: return msstats_data (PTM, PROTEIN?)
Output-->>User: msstats_data (PTM [, PROTEIN])
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vignettes/MSstatsPTM_LabelFree_Workflow.Rmd (1)
143-154: Add a file-existence guard for msstats_proteome_lf.csv to prevent vignette build failures.system.file returns "" if the resource is missing; fread("") will error during R CMD check. Guard it.
Apply this diff:
-annot = data.table::fread(annot) -input_protein = system.file("tinytest/raw_data/Fragpipe/msstats_proteome_lf.csv", - package = "MSstatsPTM") - -input_protein = data.table::fread(input_protein) # Global profiling run +annot = data.table::fread(annot) +input_protein_path = system.file("tinytest/raw_data/Fragpipe/msstats_proteome_lf.csv", + package = "MSstatsPTM") +if (!nzchar(input_protein_path) || !file.exists(input_protein_path)) { + stop("msstats_proteome_lf.csv not found in MSstatsPTM tinytest data") +} +input_protein = data.table::fread(input_protein_path) # Global profiling runR/converters.R (1)
244-265: Nice: LFQ example now shows input_protein; add a comment about the no-proteome fallback.Readers may not realize use_unmod_peptides is the fallback when no global run is available. Add a one-liner to the examples for discoverability.
Apply this diff:
#' input_protein = data.table::fread(input_protein) #' #' msstats_data = FragPipetoMSstatsPTMFormat(input, #' annot, #' input_protein = input_protein, #' label_type="LF", #' mod_id_col = "STY", #' localization_cutoff=.75, #' protein_id_col = "ProteinName", #' peptide_id_col = "PeptideSequence") +#' # If no global profiling run is available, omit input_protein and set: +#' # msstats_data = FragPipetoMSstatsPTMFormat(input, annot, +#' # label_type = "LF", mod_id_col = "STY", +#' # localization_cutoff = .75, protein_id_col = "ProteinName", +#' # peptide_id_col = "PeptideSequence", use_unmod_peptides = TRUE) #' head(msstats_data$PTM) #' head(msstats_data$PROTEIN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
inst/tinytest/raw_data/Fragpipe/msstats_proteome_lf.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
DESCRIPTION(1 hunks)R/converters.R(1 hunks)man/FragPipetoMSstatsPTMFormat.Rd(1 hunks)vignettes/MSstatsPTM_LabelFree_Workflow.Rmd(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 (3)
man/FragPipetoMSstatsPTMFormat.Rd (1)
124-146: CSV packaged correctly — examples should find it.
msstats_proteome_lf.csv is present at inst/tinytest/raw_data/Fragpipe/msstats_proteome_lf.csv, so system.file(...) will locate it.vignettes/MSstatsPTM_LabelFree_Workflow.Rmd (1)
155-165: LFQ example CSV verified — headers present.msstats_proteome_lf.csv exists and contains the expected columns: ProteinName, PeptideSequence, PrecursorCharge, FragmentIon, ProductCharge, IsotopeLabelType, Condition, BioReplicate, Run, Intensity.
DESCRIPTION (1)
43-43: Approve — RoxygenNote bumped to 7.3.3; align CI/dev roxygen2DESCRIPTION (line 43) contains "RoxygenNote: 7.3.3". No functional change; ensure CI and developer environments use roxygen2 7.3.3 to avoid doc churn.
Motivation and Context
A user from this thread was not taking advantage of the converter enabling conversion of both the PTM and global profiling experiments together.
Changes
Testing
Vignettes and docs run fine
Checklist Before Requesting a Review
Summary by CodeRabbit
New Features
Documentation
Chores