Skip to content

docs(turnover): Update example turnover file#130

Merged
tonywu1999 merged 1 commit intodevelfrom
example-file
Apr 23, 2026
Merged

docs(turnover): Update example turnover file#130
tonywu1999 merged 1 commit intodevelfrom
example-file

Conversation

@tonywu1999
Copy link
Copy Markdown
Contributor

@tonywu1999 tonywu1999 commented Apr 23, 2026

Motivation and Context

This PR updates test assertions in the DIANN to MSstats format converter test suite. The changes reflect corrections or updates to how the PXD055942 dataset (Channel-based SILAC experiment) is processed and validated by the converter, adjusting expected test outputs to match revised converter behavior.

Detailed Changes

  • Updated output row count: The expected total row count for the PXD055942 converter test increased from 20400 to 23568 rows, indicating a change in how the dataset is being processed and output by the converter.

  • Modified IsotopeLabelType distribution: The isotope label type counts were updated:

    • H (Heavy): now expected at 9720 rows
    • L (Light): now expected at 9720 rows
    • NA (Unlabeled): now expected at 4128 rows
  • Replaced PeptideSequence validation set: The set of peptide sequence values used to validate unlabeled rows (those with IsotopeLabelType == NA) was replaced with different peptide identities, indicating a change in which peptides are classified as unlabeled.

  • Updated Intensity validation cases: The test cases for channel-derived intensity validation were modified:

    • Heavy channel expected maximum Intensity changed from 662450.1 to 2453057.5
    • Light channel expected maximum Intensity changed from 116961.9 to 4686039.5
    • Heavy and light peptide sequences/runs under test were swapped between test cases

Unit Tests Modified

  • File: inst/tinytest/test_converters_DIANNtoMSstatsFormat.R
  • Changes: Test assertions for the PXD055942 converter test case were updated to reflect the new expected output characteristics, including row counts, isotope label distributions, peptide sequences, and intensity values.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

📝 Walkthrough

Walkthrough

Test assertions in the PXD055942 Channel-based SILAC converter test case are updated to reflect new output characteristics. Changes include updated row counts, modified IsotopeLabelType distribution, different PeptideSequence values for validation, and adjusted maximum Intensity expectations across labeled and unlabeled categories.

Changes

Cohort / File(s) Summary
DIANN-SILAC Test Assertions
inst/tinytest/test_converters_DIANNtoMSstatsFormat.R
Updated test expectations for PXD055942 converter: row count increased from 20400 to 23568; IsotopeLabelType counts adjusted (H: 9720, L: 9720, NA: 4128); PeptideSequence validation values changed for NA rows; heavy/light intensity validation cases swapped with new expected maximum values (2453057.5 and 4686039.5 respectively).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Diann turnover #128: Introduces upstream DIANN converter logic changes for Channel-based SILAC and isotope label type classification that directly necessitate these test assertion updates.

Poem

🐰 The numbers dance and shift with grace,
New counts replace the old ones' place,
These test assertions, now aligned,
Show converter outputs redesigned! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'docs(turnover): Update example turnover file' is misleading; the actual changes involve updating test assertions for a DIANN to MSstats converter (PXD055942), not documentation or turnover file updates. Update the title to accurately reflect the actual changes, such as 'test(converters): Update DIANN to MSstats converter test assertions for PXD055942' or similar.
Description check ⚠️ Warning The PR description uses only the template placeholder without providing any actual content; all sections are empty or contain only template text and unchecked checkboxes. Fill in the Motivation and Context, Changes, and Testing sections with specific details about why test assertions were updated and what the new expected values represent.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch example-file

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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
Copy Markdown
Contributor

@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.

🧹 Nitpick comments (1)
inst/tinytest/test_converters_DIANNtoMSstatsFormat.R (1)

159-171: Minor: h_pxd_pep and l_pxd_pep are now identical — consider a single variable.

Since both heavy and light channel checks are performed on the same peptide ("TAFDDAIAELDTLNEDSYK(SILAC)(SILAC)(SILAC)"), duplicating the literal in two variables invites future drift (one gets updated, the other doesn't). A single shared variable makes the intent clearer: same peptide, different channel, different run/timepoint.

♻️ Proposed consolidation
-h_pxd_pep = "TAFDDAIAELDTLNEDSYK(SILAC)(SILAC)(SILAC)"
+pxd_pep = "TAFDDAIAELDTLNEDSYK(SILAC)(SILAC)(SILAC)"
 h_pxd_run = "20210805_BoxCarmax2nd_wideMS1_JM_pSIL_pro_heart_32d_1"
-h_pxd_ints = output_pxd_dt[PeptideSequence == h_pxd_pep &
+h_pxd_ints = output_pxd_dt[PeptideSequence == pxd_pep &
                              IsotopeLabelType == "H" &
                              Run == h_pxd_run, Intensity]
 expect_equal(max(h_pxd_ints, na.rm = TRUE), 2453057.5, tolerance = 1)

-l_pxd_pep = "TAFDDAIAELDTLNEDSYK(SILAC)(SILAC)(SILAC)"
 l_pxd_run = "20210805_BoxCarmax2nd_wideMS1_JM_pSIL_pro_heart_0d_1"
-l_pxd_ints = output_pxd_dt[PeptideSequence == l_pxd_pep &
+l_pxd_ints = output_pxd_dt[PeptideSequence == pxd_pep &
                              IsotopeLabelType == "L" &
                              Run == l_pxd_run, Intensity]
 expect_equal(max(l_pxd_ints, na.rm = TRUE), 4686039.5, tolerance = 1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@inst/tinytest/test_converters_DIANNtoMSstatsFormat.R` around lines 159 - 171,
The two test variables h_pxd_pep and l_pxd_pep hold the same peptide literal;
replace them with a single shared variable (e.g., peptide_seq or pxd_pep) and
use that variable in both queries (where h_pxd_pep and l_pxd_pep are referenced)
so the heavy vs light checks only differ by IsotopeLabelType and Run while
avoiding duplicate literals and drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@inst/tinytest/test_converters_DIANNtoMSstatsFormat.R`:
- Around line 159-171: The two test variables h_pxd_pep and l_pxd_pep hold the
same peptide literal; replace them with a single shared variable (e.g.,
peptide_seq or pxd_pep) and use that variable in both queries (where h_pxd_pep
and l_pxd_pep are referenced) so the heavy vs light checks only differ by
IsotopeLabelType and Run while avoiding duplicate literals and drift.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 81b2d09c-50ba-4725-82fa-bfae5dd5c921

📥 Commits

Reviewing files that changed from the base of the PR and between 222317e and be34230.

⛔ Files ignored due to path filters (1)
  • inst/tinytest/raw_data/DIANN/PXD055942.parquet is excluded by !**/*.parquet
📒 Files selected for processing (1)
  • inst/tinytest/test_converters_DIANNtoMSstatsFormat.R

@tonywu1999 tonywu1999 merged commit c0ba69c into devel Apr 23, 2026
2 checks passed
@tonywu1999 tonywu1999 deleted the example-file branch April 23, 2026 16:26
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