fix(simulate): template-coordinate order via canonical fgumi-sort + hermetic tests - #576
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
WalkthroughRisk is concentrated in output-order identity and temporary-BAM finalization. Simulation now delegates template-coordinate ordering to ChangesSimulation sorting pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant fgumi_simulate
participant TemporaryBAM
participant RawExternalSorter
participant FinalBAM
participant IntegrationTest
fgumi_simulate->>TemporaryBAM: write simulated records in molecule order
TemporaryBAM->>RawExternalSorter: provide unsorted BAM
RawExternalSorter->>FinalBAM: write template-coordinate sorted BAM
IntegrationTest->>FinalBAM: verify ordering with fgumi sort
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #576 +/- ##
==========================================
+ Coverage 92.99% 93.06% +0.07%
==========================================
Files 167 166 -1
Lines 103266 102849 -417
==========================================
- Hits 96030 95717 -313
+ Misses 7236 7132 -104 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ebed334 to
2360ed8
Compare
2360ed8 to
47aa253
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/simulate/mapped_reads.rs (1)
232-243: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRemove the obsolete O(N) molecule metadata buffers.
External sorting means generation can stream directly in
mol_idorder.
src/lib/commands/simulate/mapped_reads.rs#L232-L243: draw the seed/unmapped decision inside the write loop.src/lib/commands/simulate/grouped_reads.rs#L225-L230: draw the seed inside the write loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/commands/simulate/mapped_reads.rs` around lines 232 - 243, Remove the eager O(N) molecule metadata collection around the mapped-read generation and draw each molecule’s seed and unmapped decision directly inside the write loop, preserving deterministic RNG advancement for every molecule. Apply the corresponding change in src/lib/commands/simulate/mapped_reads.rs lines 232-243 and draw each seed inside the write loop in src/lib/commands/simulate/grouped_reads.rs lines 225-230; update the surrounding generation logic to use these per-iteration values without the metadata buffers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_simulate_sort.rs`:
- Around line 59-106: Replace the self-consistency-only `fgumi sort --verify`
assertion in `simulate_output_is_template_coordinate_sorted` with an independent
expected-result oracle. Generate or trace the expected ordered record names and
flags for mapped, simplex, and duplex cases, explicitly covering F2R1, then read
the simulated BAM and assert every record’s name and flag matches the expected
sequence without omissions or extras. Retain coverage for all existing
subcommands and duplex variants.
---
Outside diff comments:
In `@src/lib/commands/simulate/mapped_reads.rs`:
- Around line 232-243: Remove the eager O(N) molecule metadata collection around
the mapped-read generation and draw each molecule’s seed and unmapped decision
directly inside the write loop, preserving deterministic RNG advancement for
every molecule. Apply the corresponding change in
src/lib/commands/simulate/mapped_reads.rs lines 232-243 and draw each seed
inside the write loop in src/lib/commands/simulate/grouped_reads.rs lines
225-230; update the surrounding generation logic to use these per-iteration
values without the metadata buffers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5dbc371a-457f-4f93-9c22-8533fcbfdc68
📒 Files selected for processing (6)
src/lib/commands/simulate/common.rssrc/lib/commands/simulate/grouped_reads.rssrc/lib/commands/simulate/mapped_reads.rssrc/lib/commands/simulate/mod.rssrc/lib/commands/simulate/sort.rstests/integration/test_simulate_sort.rs
💤 Files with no reviewable changes (2)
- src/lib/commands/simulate/mod.rs
- src/lib/commands/simulate/sort.rs
…umi-sort engine `simulate mapped-reads`/`grouped-reads` computed their own template-coordinate sort key (`simulate/sort.rs`, `TemplateCoordKey::for_f1r2_pair`) from intended parameters, hard-coding an "R1 is forward/left" assumption. But `simulate` emits both read-1 orientations, so for reverse-R1 (F2R1) pairs the key's `pos1`/strand disagreed with the record's actual leftmost 5' coordinate — the exact fields `samtools sort --template-coordinate` keys on. Result: ~0.7% of records (1000 molecules, seed 42: 44 of 5994) were emitted out of template-coordinate order, so the "template-coordinate sorted" output wasn't. Root cause is duplication: a second, hand-rolled ordering that drifted from the one fgumi already has. Fix by deleting `simulate/sort.rs` and generating records in molecule order to an unsorted temp BAM, then sorting into the final output with the canonical `fgumi-sort` `RawExternalSorter` (`SortOrder::TemplateCoordinate`) — the same engine `fgumi sort`/`fgumi group` use and that matches `samtools sort --template-coordinate` exactly at the coordinate-key level. `grouped-reads` forces the tertiary (library|mi) key lane since it always writes MI tags. Correct for every orientation by construction; output is now byte-identical to `fgumi sort --order template-coordinate` of the same records. `MoleculeInfo` no longer carries a sort key (molecules are emitted in id order).
…rt --verify` The previous `test_simulate_sort` tests were `#[ignore]`d, stale, and slow: they shelled out to `cargo run --release` (recursively compiling fgumi inside the test, blowing nextest's timeout), omitted the now-required `--reference`, and byte-compared against an external `samtools`. Rewrite them to: - run the prebuilt binary via `CARGO_BIN_EXE_fgumi` (no recursive compile), - generate a small deterministic reference FASTA fixture in a tempdir, - gate on `#[cfg(feature = "simulate")]` so the binary has the subcommand, - verify the output with `fgumi sort --verify --order template-coordinate` — fgumi's own canonical sort-order checker — instead of a brittle byte-exact samtools diff. An `#[rstest]` table covers mapped-reads, grouped-reads simplex/duplex, and a larger duplex dataset. These now run in the normal suite (no `#[ignore]`, no samtools) and are the regression gate for the sort fix in the preceding commit.
47aa253 to
829cebd
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
The bug
fgumi simulateclaims its output is template-coordinate sorted, but for ~0.7% of records it wasn't. Reproduced (1000 molecules, seed 42): 44 of 5994 records are out of order versussamtools sort --template-coordinate.Root cause:
simulatecarried its own template-coordinate key (simulate/sort.rs,TemplateCoordKey::for_f1r2_pair), computed from intended parameters under a hard-coded "R1 is forward/left" assumption. Butsimulateemits both read-1 orientations, so for reverse-R1 (F2R1) pairs the key'spos1/strand disagreed with the record's actual leftmost-5′ coordinate — exactly what samtools keys on. A second, hand-rolled ordering that drifted from the one fgumi already has.(Discovered while auditing the
#[ignore]dtest_simulate_sorttests — they were also stale, missing the now-required--reference, which is why the failure looked like a crash rather than a mis-sort.)The fix (principled: one source of truth)
Delete
simulate/sort.rs. Generate records in molecule order to an unsorted temp BAM, then sort into the final output with the canonicalfgumi-sortengine (RawExternalSorter,SortOrder::TemplateCoordinate) — the same onefgumi sort/fgumi groupuse, which matchessamtools sort --template-coordinateexactly at the coordinate-key level. Correct for every orientation by construction.grouped-readsforces the tertiary (library|mi) key lane since it always writes MI tags.Verified: simulate-vs-samtools position diffs 44 → 0; output is now byte-identical to
fgumi sort --order template-coordinateof the same records.Hermetic tests (the regression gate)
Rewrote
test_simulate_sort: run the prebuilt binary viaCARGO_BIN_EXE_fgumi(no recursivecargo run→ no timeout), generate a deterministic reference FASTA fixture, gate on#[cfg(feature="simulate")], and verify withfgumi sort --verify --order template-coordinate(fgumi's own checker) instead of a brittle byte-exact samtools diff. An#[rstest]table covers mapped-reads and grouped-reads simplex/duplex/large. No longer#[ignore]d — they run in the normal suite.Verification
cargo ci-test→ 4726 passed, 0 failed (incl. the 4 un-ignored sort tests; the deletedsimulate/sort.rsunit tests are covered by the canonicalfgumi-sorttests).cargo ci-fmt,cargo ci-lint(-D warnings -W clippy::pedantic) → clean.Independent of #572/#573/#574/#575.
Summary by CodeRabbit
Bug Fixes
Tests
fgumibinary (no reliance on external tools).fgumi sort --verify --order template-coordinate.