Repository navigation
fix(compare): preserve /A /B strand suffix in paired-UMI MI keys - #308
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMI value handling in BAM comparison was refactored to support paired-UMI notation. The representation changed from a simple integer ( 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
+ Coverage 90.46% 90.58% +0.12%
==========================================
Files 101 101
Lines 60094 60160 +66
==========================================
+ Hits 54366 54498 +132
+ Misses 5728 5662 -66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/commands/compare/bams.rs (1)
1467-1473:⚠️ Potential issue | 🟡 MinorUse
Displayfor sampledMiKeydiagnostics.These mismatch messages still use
Debug, so paired MI values render as enum internals likeStrand { base: 3, strand: 65 }instead of3/A.Proposed fix
+fn format_mi_key_sample<'a>(values: impl IntoIterator<Item = &'a MiKey>) -> String { + format!("[{}]", values.into_iter().map(ToString::to_string).join(", ")) +} + "MI group '{}' in BAM1 ({} reads) maps to {} different MIs in BAM2: {:?}", mi1, read_hashes.len(), mi2_values.len(), - mi2_values.iter().take(5).collect::<Vec<_>>() + format_mi_key_sample(mi2_values.iter().take(5)) ));Apply the same replacement to the BAM2→BAM1 messages.
Also applies to: 1486-1492, 1778-1784, 1797-1803
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/compare/bams.rs` around lines 1467 - 1473, The diagnostic strings are using Debug for MiKey values so enums print internals; update the sampled-value construction in the grouping_errors.push calls (the one that references mi1, read_hashes, mi2_values) to render MiKey with Display instead of Debug by mapping the sampled iterator to strings (e.g., mi2_values.iter().take(5).map(|m| m.to_string()).collect::<Vec<_>>()) and join or collect those strings into the message; apply the same change to the symmetric BAM2→BAM1 grouping_errors.push sites and the other occurrences that reference mi1/mi2, mi1_values/mi2_values, and MiKey so all sampled MiKey diagnostics use Display.
🧹 Nitpick comments (1)
tests/integration/test_compare_bams.rs (1)
654-657: Assert the mismatch reason, not just failure.This test would also pass if paired MI strings regressed to “missing MI”. Check zero missing MI plus a grouping mismatch so it specifically covers
/Avs/Bpreservation.Proposed test hardening
let (success, stdout) = run_compare(&bam1, &bam2, "grouping", &["--ignore-order"]); assert!(!success, "Expected DIFFER when /A and /B assignments disagree, stdout:\n{stdout}"); assert!(stdout.contains("DIFFER"), "Expected DIFFER, got:\n{stdout}"); + assert!( + stdout.contains("Missing MI in BAM1: 0") && stdout.contains("Missing MI in BAM2: 0"), + "Expected paired-strand MIs to be parsed as present, got:\n{stdout}" + ); + assert!( + stdout.contains("Grouping mismatches: 1"), + "Expected the failure to be a grouping mismatch, got:\n{stdout}" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_compare_bams.rs` around lines 654 - 657, The test currently only checks that run_compare returned a failure and that stdout contains "DIFFER"; strengthen it to assert the specific mismatch reason by checking stdout contains both the zero-missing-MI indicator and the grouping mismatch text so it fails if MI handling regresses. Update the assertions around run_compare(&bam1, &bam2, "grouping", &["--ignore-order"]) to assert stdout contains "0 missing MI" (or the exact zero-missing wording your tool emits) and also contains the grouping/assignment mismatch phrase (e.g. "grouping" or "/A vs /B" as emitted), in addition to keeping the existing DIFFER check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/commands/compare/bams.rs`:
- Around line 1467-1473: The diagnostic strings are using Debug for MiKey values
so enums print internals; update the sampled-value construction in the
grouping_errors.push calls (the one that references mi1, read_hashes,
mi2_values) to render MiKey with Display instead of Debug by mapping the sampled
iterator to strings (e.g., mi2_values.iter().take(5).map(|m|
m.to_string()).collect::<Vec<_>>()) and join or collect those strings into the
message; apply the same change to the symmetric BAM2→BAM1 grouping_errors.push
sites and the other occurrences that reference mi1/mi2, mi1_values/mi2_values,
and MiKey so all sampled MiKey diagnostics use Display.
---
Nitpick comments:
In `@tests/integration/test_compare_bams.rs`:
- Around line 654-657: The test currently only checks that run_compare returned
a failure and that stdout contains "DIFFER"; strengthen it to assert the
specific mismatch reason by checking stdout contains both the zero-missing-MI
indicator and the grouping mismatch text so it fails if MI handling regresses.
Update the assertions around run_compare(&bam1, &bam2, "grouping",
&["--ignore-order"]) to assert stdout contains "0 missing MI" (or the exact
zero-missing wording your tool emits) and also contains the grouping/assignment
mismatch phrase (e.g. "grouping" or "/A vs /B" as emitted), in addition to
keeping the existing DIFFER check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65589270-a768-470c-b5f4-3e1283df53f5
📒 Files selected for processing (2)
src/lib/commands/compare/bams.rstests/integration/test_compare_bams.rs
Paired-UMI grouping (fgumi and fgbio) writes MI as a Z-typed string of
the form "<id>/<A|B>". The grouping-mode comparator parsed MI via
`str::parse::<i64>()`, which rejects the suffixed form, so every record
in a paired-UMI BAM was counted as "missing MI" and the comparator
reported `Records matched: 0 / BAM groupings DIFFER` on files that
agreed exactly.
Replace the `i64`-valued MI map with a `MiKey` enum that preserves the
strand byte (`b'A'` / `b'B'`) alongside the molecule id, so the /A vs /B
distinction is retained for grouping equivalence checks. A bare integer
MI (string or int-typed) round-trips through `MiKey::Int`; a strand
suffix round-trips through `MiKey::Strand { base, strand }`; Display
reproduces the original BAM encoding so the existing error messages
("MI group '...' in BAM1 ...") read unchanged.
Covered by new unit tests for the parser (integer, string-integer,
`/A`, `/B`, invalid-strand, non-numeric) and three integration tests
that build paired-UMI BAMs and exercise both the ordered and
`--ignore-order` grouping paths plus the /A↔/B flip case.
f4ddcbf to
84527b9
Compare
This branch was previously deployed
1 participant
Summary
fgumi compare bams --mode groupingincorrectly reported paired-UMI BAMs as disagreeing (Records matched: 0,BAM groupings DIFFER) even when the two files had identical groupings. The MI reader calledstr::parse::<i64>()on the tag value, which rejects the spec-compliant paired-strand encodingMI:Z:<id>/<A|B>emitted by both fgumi and fgbio, so every record was counted as "missing MI".This PR replaces the
i64-valued MI map with a smallMiKeyenum that parses all three observed MI forms and preserves the/Avs/Bdistinction for grouping-equivalence checks:MI:i:<int>→MiKey::IntMI:Z:<int>→MiKey::IntMI:Z:<int>/A/MI:Z:<int>/B→MiKey::Strand { base, strand }None(treated as missing, as before)Displayround-trips each form back to the original BAM encoding so the existing grouping-mismatch error messages (MI group '…' in BAM1 (N reads) maps to multiple MIs in BAM2) read unchanged.Why not pack into
i64?MoleculeId::compact_index()(base*2,base*2+1) would collide withSingle(n)if a file mixed single and paired MIs, silently aliasing groups and masking real disagreements. The enum is the minimal type that preserves the A/B invariant without that risk.Test plan
New tests (all in
src/lib/commands/compare/bams.rsandtests/integration/test_compare_bams.rs):MI:i:42→Int(42)MI:Z:42→Int(42)MI:Z:0/A→Strand { base: 0, strand: b'A' }MI:Z:7/B→Strand { base: 7, strand: b'B' }13/Aand13/Bproduce different keys (core invariant)NoneDisplayround-trips each formEQUIVALENTwith zero missing MI--ignore-ordergrouping mode on identical paired-strand BAMs reportsEQUIVALENTwith zero missing MI/A↔/Bbetween the two BAMs is detected as a grouping mismatchMissing MI in BAM1: 4,Records matched: 0,DIFFER)cargo ci-fmt,cargo ci-lint,cargo ci-test(2494 passed)