Skip to content

fix(duplex-metrics): pair duplex UMI halves by /A,/B strand family - #1031

Merged
nh13 merged 1 commit into
mainfrom
nh/duplex-umi-strand-orientation
Oct 8, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/duplex-umi-strand-orientation

Conversation

@nh13

@nh13 nh13 commented Oct 7, 2026

Copy link
Copy Markdown
Member

Summary of changes

duplex-metrics oriented each read's RX halves by R1 strand before calling the per-position UMI consensus. For a duplex whose mates map in the same orientation (FF/RR: inversions and most inter-chromosomal chimeras), R1 is on the same strand in both the /A and /B families, so the /B reads were left unswapped and the two halves were mixed into one consensus.

Before: a single FF duplex family with AAA-CAG/A + CAG-AAA/B produced umi_counts.txt = NAN 4 raw / 4 with errors / 2 unique and duplex_umi_counts.txt = NAN-NAN. This inflates raw_observations_with_errors and invents N-containing UMIs (most visible with short UMIs).
After: AAA and CAG, each 2 raw / 0 with errors / 1 unique, and a single duplex UMI AAA-CAG.

The fix pairs the halves by the MI /A,/B strand family (an unsuffixed MI is treated as /A, matching the existing AB-strand counting), as fgbio does per single-strand family (CollectDuplexSeqMetrics.scala:407-408). The reported duplex UMI in duplex_umi_counts.txt is still oriented by R1 strand (DXM-02), leading with the half a positive-strand R1 reads first.

Family-size, yield, grouping and consensus outputs are unaffected. FR/RF pairs produce identical output to before.

Issue(s) addressed by this PR

No separate issue; the bug is described above.

Reviewer guidance

  • The production change is confined to record_duplex_umi_metrics / record_duplex_coordinate_group in src/lib/commands/shared_metrics.rs. That function is shared by the standalone duplex-metrics command and the inline consensus-metrics path, so both get the fix. It now receives &TemplateMetadata (which already carries is_b_strand) instead of an (mi, rx, r1_positive) tuple, so the strand classification is not re-derived.
  • Known divergence from fgbio (duplex UMI orientation only): for FF/RR duplexes, neither strand family is F1R2-only, so fgbio's orientation pick in duplex_umi_counts.txt depends on which single-strand family groupBy(MI) iterates first (JVM hash order of the MI strings). fgumi deterministically leads with the /A leading half for FF and the /A trailing half for RR. A probe of 300 synthetic molecules per orientation against fgbio 4.1.0 showed umi_counts.txt and duplex_family_sizes.txt identical for FR/RF/FF/RR, and duplex_umi_counts.txt identical for FR/RF; for FF/RR about half the duplex UMI keys are reported in the opposite orientation, as expected from the above.
  • Tests: test_duplex_umi_halves_paired_by_strand_family (FR, RF, FF, RR, and inter-chromosomal FF/RR; the FF/RR cases fail on main) and test_duplex_umi_orientation_of_b_only_family (pins the /B-only orientation branches). The test helper build_test_pair now delegates to build_test_pair_on_refs so mates can sit on different references.

Does this introduce a breaking change?

  • Yes
  • No

✔️ Checklist

  • I have checked there are not other open Pull Requests for the same update/change
  • I added test case(s) whose failure behavior is different before vs after the change
  • All non-obvious additions to the code are thoroughly commented
  • All CI checks pass (compile, test, format, lints)
  • I added/updated all relevant documentation (e.g., README.md and docs/*)
  • All relevant logs, outputs, screenshots, etc. are attached
  • I have followed all the other contributing guidelines not covered by the above items (e.g., git etiquette, branch naming, commit messages)

duplex-metrics oriented each read's RX halves by R1 strand before calling
the per-position UMI consensus. For a duplex whose mates map in the same
orientation (FF/RR: inversions and most inter-chromosomal chimeras), R1 is
on the same strand in both the /A and /B families, so the /B reads were
left unswapped and the two halves were mixed into one consensus (e.g. AAA
with CAG -> NAN). This inflated raw_observations_with_errors and invented
N-containing UMIs in umi_counts.txt, and corrupted the duplex UMI key in
duplex_umi_counts.txt.

Pair the halves by the MI /A,/B suffix, as fgbio does per single-strand
family (CollectDuplexSeqMetrics.scala:407-408), and keep orienting the
reported duplex UMI by R1 strand (DXM-02).
@nh13
nh13 deployed to github-actions October 7, 2026 01:56 — with GitHub Actions Active
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: 376f8992-2a34-46ba-8fc2-9bb048eecc6e
📥 Commits

Reviewing files that changed from the base of the PR and between 62b98c0 and d6a1456.

📒 Files selected for processing (2)
  • src/lib/commands/duplex_metrics.rs
  • src/lib/commands/shared_metrics.rs

Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.


⚠️ A high-level summary could not be generated for this review. CodeRabbit will regenerate it on the next update, or you can request a refresh with @coderabbitai summary.

Walkthrough

Duplex metrics now pair UMI halves by MI suffix and select reported orientation using R1 strand information. Test fixtures support mates on different references, and new tests cover duplex UMI accounting across read orientations and inter-chromosomal pairs.

Changes

Duplex UMI metrics

Layer / File(s) Summary
Pairing, orientation, and validation
src/lib/commands/shared_metrics.rs, src/lib/commands/duplex_metrics.rs
Metrics processing uses template metadata to pair RX halves by MI suffix, selects reported orientation using positive-strand R1 presence, and compares family RX tags against both orientations. Test helpers support separate mate references, and tests cover suffix pairing, orientation, and UMI accounting.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to d6a14

The duplex UMI pairing change has no identified issue requiring a fix before merge.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required conventional-commit format. Its scope identifies duplex-metrics, and its lowercase imperative description summarizes the change.
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.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@nh13

nh13 commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Oct 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.45%. Comparing base (62b98c0) to head (d6a1456).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1031      +/-   ##
==========================================
- Coverage   96.47%   96.45%   -0.03%     
==========================================
  Files         299      299              
  Lines      152214   152273      +59     
==========================================
+ Hits       146854   146880      +26     
- Misses       5360     5393      +33     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13

nh13 commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nh13
nh13 added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit 0e53bae Oct 8, 2026
21 checks passed
@nh13
nh13 deleted the nh/duplex-umi-strand-orientation branch October 8, 2026 00:56
@nh13 nh13 mentioned this pull request Oct 8, 2026

This branch was successfully deployed

1 active deployment
github-actions — d6a1456c Deployed Oct 7, 2026 by nh13 via coverage #4848
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