Repository navigation
feat(group,dedup): add opt-in --verify for strict template-coordinate sort-order - #909
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe PR adds opt-in template-coordinate order verification to ChangesTemplate-coordinate verification
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The opt-in verification is comprehensively covered, with no identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant CLI
participant PipelineBuilder
participant GroupByPosition
participant RecordPositionGrouper
participant OrderVerifier
CLI->>PipelineBuilder: pass verify option
PipelineBuilder->>GroupByPosition: configure verifying(header)
GroupByPosition->>RecordPositionGrouper: enable_verify(header)
RecordPositionGrouper->>OrderVerifier: validate each record
OrderVerifier-->>RecordPositionGrouper: accept or InvalidData
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #909 +/- ##
==========================================
- Coverage 93.55% 93.53% -0.03%
==========================================
Files 301 301
Lines 150626 150745 +119
==========================================
+ Hits 140923 141001 +78
- Misses 9703 9744 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
… order Add a `--verify` flag to `fgumi group` and `fgumi dedup` that checks the input is in strict `fgumi sort --order template-coordinate` order and fails fast (non-zero exit) on the first out-of-order record, before emitting a wrong result. Off by default. Today group/dedup only validate the input header's sort tags (SO/GO/SS), not the actual record order, so a mislabeled or mangled file is silently mis-grouped. `--verify` closes that gap for callers who want a hard guarantee. The check runs inline in a single pass at the top of `RecordPositionGrouper::process_record` -- the serial chokepoint shared by all four paths (group/dedup x non-chain/chain) -- before the secondary/supplementary skip, so the whole file's canonical order is checked. It reuses fgumi-sort's own `extract_template_key_inline` and the tolerant `TemplateKey::core_cmp` (ignoring the name_hash tie-break so both fgumi- and samtools-sorted inputs pass), keying on the CB cell tag to match what `fgumi sort --order template-coordinate` produces by default. The check is intentionally stricter than grouping/dedup require, which is why it is opt-in and documented as strict; it composes with --output as a precondition gate rather than a check-only mode (`fgumi sort --verify` already provides pure check-only).
f2123ef to
6f80b42
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Adds an opt-in
--verifyflag tofgumi groupandfgumi dedupthat checks the input is in strictfgumi sort --order template-coordinateorder and fails fast (non-zero exit) on the first out-of-order record, before writing a wrong result. Off by default.fgumi group/dedupalready assume template-coordinate input but only validate the header's sort tags (SO/GO/SS), not the actual record order — so a mislabeled or mangled file is silently mis-grouped.--verifycloses that gap for callers who want a hard guarantee.Design
RecordPositionGrouper::process_record— the serial chokepoint shared by all four paths (group/dedup× non-chain /--threads Nchain). It runs before the secondary/supplementary skip, so the whole file's canonical order is checked, including sec/supp reads that grouping later drops.fgumi_sort::extract_template_key_inlineand compares withTemplateKey::core_cmp(the same tolerant comparisonfgumi sort --verifyuses, which ignores thename_hashtie-break so both fgumi- and samtools-sorted inputs pass). It keys on theCBcell tag — matching whatfgumi sort --order template-coordinateemits by default (parse_cell_tag), using the same fixed-seed hasher — so a correctly-sorted single-cell/multi-library input is not spuriously rejected.fgumi sort --verifyalready exists; here--verifyis a precondition gate that composes with--output.Testing
proptest, 256 cases each):--verifyaccepts a random(tid,pos)stream iff it is non-decreasing under the template key, and any key-sorted stream is always accepted — fuzzing the fold over arbitrary orders, shuffles, and ties.--threadschain):--verifyaccepts a correctly-sorted input and leaves output identical to the non-verify run; rejects an out-of-order input with a clear error while the same input is accepted without--verify.fgumi sortorders by cell-barcode hash (overriding library order) is accepted by--verify; a CB-blind check would spuriously reject it.Notes
main.Risk: output changes only when
--verifyrejects out-of-order input;unsafechanges are none, and noCLAUDE.mdallowlist update applies; memory bounds, queue capacity, and thread/backpressure policy changes are none.Fix: Use
--verifywithfgumi grouporfgumi dedupto fail fast on input that is not in strict template-coordinate order. Verification is disabled by default.fgumi sort --order template-coordinatekey and comparison logic.CBtag and fixed-seed hash for ordering.