Skip to content

Conversation

@tonywu1999
Copy link
Contributor

@tonywu1999 tonywu1999 commented Sep 17, 2025

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

  • Update vignette for Fragpipe to have a global profiling dataset with a PTM dataset (created it myself based on the existing PTM dataset)

Testing

Vignettes and docs run fine

Checklist Before Requesting a Review

  • I have read the MSstats contributing guidelines
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • New Features

    • FragPipetoMSstatsPTMFormat now accepts optional annotation and optional protein-level input, enabling creation of a PROTEIN dataset alongside PTM when provided.
  • Documentation

    • Updated LFQ examples, vignette, and man page to show passing input_protein, simplified headings, and to display PROTEIN output in examples.
  • Chores

    • Bumped package metadata (roxygen2) with no logic changes.

@coderabbitai
Copy link

coderabbitai bot commented Sep 17, 2025

Walkthrough

RoxygenNote in DESCRIPTION bumped. Examples, documentation, and vignette updated to show passing an optional input_protein (global profiling) into FragPipetoMSstatsPTMFormat; function signature defaulted annotation to NULL. No runtime logic or control-flow changes beyond the API default adjustments and example updates.

Changes

Cohort / File(s) Summary
Package metadata
DESCRIPTION
Bumped RoxygenNote from 7.3.2 to 7.3.3.
Function signature + examples
R/converters.R
Changed FragPipetoMSstatsPTMFormat signature to make annotation = NULL and keep input_protein = NULL/annotation_protein = NULL; updated examples to load and pass input_protein as a named argument; minor formatting tweaks.
Man page example
man/FragPipetoMSstatsPTMFormat.Rd
Updated LFQ example to read msstats_proteome_lf.csv into input_protein and pass it to FragPipetoMSstatsPTMFormat; removed note about PROTEIN being NULL; heading adjusted.
Vignette (LFQ workflow)
vignettes/MSstatsPTM_LabelFree_Workflow.Rmd
FragPipe LFQ example now reads and passes input_protein, heading changed, and example shows head(msstats_data$PROTEIN) in addition to PTM output.

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])
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A nibble of docs, a hop through notes,
I tucked in protein with LFQ coats.
No codepaths changed, just clearer trails,
A carrot of guidance in sample details.
Thump-thump—examples bloom, concise and bright. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description includes the required template sections (Motivation and Context, Changes, Testing, and Checklist) and explains the vignette update, but the Changes section is minimal and omits a material code-level modification present in the diff: FragPipetoMSstatsPTMFormat's signature was changed to make annotation optional and examples were adjusted. Because the template requires a detailed list of changes and test information and this API change has potential compatibility implications, the description is currently incomplete relative to the repository's template. Please update the PR description to enumerate the modified files and explicitly document the FragPipetoMSstatsPTMFormat signature change (including any backward-compatibility impact), detail the example and vignette edits, and add or describe the tests or verification steps performed (or state why none are needed).
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title is concise and accurately describes the primary user-visible change — updating the FragPipe vignettes to include a global profiling run example — so it reflects the main intent of the changeset. It is relevant to the diff, though the changeset also contains a minor API change (FragPipetoMSstatsPTMFormat signature defaulting annotation to NULL) that the title does not call out.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch docs-fragpipe

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64800fa and f792022.

📒 Files selected for processing (2)
  • R/converters.R (1 hunks)
  • man/FragPipetoMSstatsPTMFormat.Rd (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • man/FragPipetoMSstatsPTMFormat.Rd
  • R/converters.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

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@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: 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 run
R/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

📥 Commits

Reviewing files that changed from the base of the PR and between 43dc051 and 64800fa.

⛔ Files ignored due to path filters (1)
  • inst/tinytest/raw_data/Fragpipe/msstats_proteome_lf.csv is 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 roxygen2

DESCRIPTION (line 43) contains "RoxygenNote: 7.3.3". No functional change; ensure CI and developer environments use roxygen2 7.3.3 to avoid doc churn.

@tonywu1999 tonywu1999 merged commit 16c54eb into devel Sep 17, 2025
2 checks passed
@tonywu1999 tonywu1999 deleted the docs-fragpipe branch September 17, 2025 14:14
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.

2 participants