Repository navigation
fix(clip): clip chimeric templates, soft-only mate window, drop --sort-order (R2-CLIP-02/03/04) - #502
Conversation
WalkthroughClip now uses soft-clip-only mate boundaries and flag-based primary selection. It preserves input order, repairs primary and supplementary mate fields, and adds threaded integration coverage for paired, fragment, secondary-only, and supplementary records. ChangesClip boundary and primary-template handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClipExecute
participant process_fn
participant find_primary_pair_indices
participant RawRecordClipper
participant fix_supplemental_mate_info
ClipExecute->>process_fn: process template
process_fn->>find_primary_pair_indices: locate primary R1 and R2
find_primary_pair_indices-->>process_fn: return primary indices
process_fn->>RawRecordClipper: clip primary records
RawRecordClipper-->>process_fn: return clipped records
process_fn->>fix_supplemental_mate_info: repair supplementary mate fields
Possibly related PRs
Suggested labels: 🚥 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 #502 +/- ##
==========================================
+ Coverage 91.17% 92.55% +1.38%
==========================================
Files 78 165 +87
Lines 51916 99363 +47447
==========================================
+ Hits 47333 91967 +44634
- Misses 4583 7396 +2813 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
831d9c3 to
1a641f2
Compare
c7535af to
389e375
Compare
1a641f2 to
fea962a
Compare
389e375 to
fb85484
Compare
fea962a to
541f93f
Compare
fb85484 to
6794f10
Compare
541f93f to
06e0eee
Compare
06e0eee to
4faa90e
Compare
|
@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: 2
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/clip.rs (1)
630-710: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThreaded fixed-position clipping duplicates
clip_pair's branching logic inline.The R1/R2 fixed-position clip selection here re-implements
Clip::clip_pair'sis_r1_first/is_r2_lastbranching (minus metrics) rather than delegating to it. Currently consistent (verified against 463-565), but any future tweak toclip_pairrisks silently diverging from the threaded path since there's no shared code path enforcing parity beyond tests. Consider extracting the shared fixed-position-clip decision into a small&self-free helper both paths can call.🤖 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/clip.rs` around lines 630 - 710, Extract the fixed-position clipping decision logic currently duplicated in the threaded loop and Clip::clip_pair into a shared free helper, using the existing read-orientation checks and R1/R2 threshold parameters. Update both the threaded pair branch and clip_pair to call this helper, while preserving their separate metrics and other clipping behavior.
🤖 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 `@crates/fgumi-sam/src/record_utils.rs`:
- Around line 516-533: The typed unsoftclipped_end function lacks the
empty-CIGAR handling present in unsoftclipped_end_raw. Add a guard after
converting the record CIGAR operations that returns None when the operations are
empty, and update the function documentation to state this behavior.
In `@tests/integration/test_clip_command.rs`:
- Around line 269-286: The combined fixed, overlap, and mate-extension clipping
test uses weak range-based assertions instead of validating the expected output.
In the loop over records in this test, assert each read’s identity and exact
expected CIGAR, following the independent exact-CIGAR oracle used by
test_clip_command_threads_mode_fragment, while retaining the check that both
expected reads are present.
---
Outside diff comments:
In `@src/lib/commands/clip.rs`:
- Around line 630-710: Extract the fixed-position clipping decision logic
currently duplicated in the threaded loop and Clip::clip_pair into a shared free
helper, using the existing read-orientation checks and R1/R2 threshold
parameters. Update both the threaded pair branch and clip_pair to call this
helper, while preserving their separate metrics and other clipping behavior.
🪄 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: aa4f6e9b-20a5-4f01-b48b-81b89d714821
📒 Files selected for processing (4)
crates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/record_utils.rssrc/lib/commands/clip.rstests/integration/test_clip_command.rs
|
Addressed the two inline findings:
On the outside-diff-range note about the threaded fixed-position clipping duplicating |
4faa90e to
f210aa2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
f210aa2 to
8dd3177
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_clip_command.rs`:
- Around line 166-388: Add an end-to-end --threads integration test covering a
template with primary R1, primary R2, and supplementary R1 records, verifying
Clip::execute repairs the supplementary mate metadata after clipping. Extend
OutputRecord/read_output_records or add a helper to independently decode and
assert supplementary mate reference/position/strand, MC, MQ, and template length
against expected primary-pair values; ensure the test exercises
execute_threads_mode wiring and record ordering.
🪄 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: d8e7bde3-0945-4f1f-9186-720548f5d30e
📒 Files selected for processing (4)
crates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/record_utils.rssrc/lib/commands/clip.rstests/integration/test_clip_command.rs
…t-order (R2-CLIP-02/03/04) Three round-2 clip parity fixes, stacked on #492 (CLIP-01 / setMateInfo): - R2-CLIP-02: the clip loop was `if len==1 {frag} else if len==2 {pair}` with no else, so any template with a secondary/supplementary read (len > 2) was left COMPLETELY unclipped with no mate fixing. Now locate the primary R1/R2 by SAM flags (fgbio `Template.r1`/`r2`), clip them, and repair mate info on every supplementary alignment via a port of htsjdk 5.0.0 `SamPairUtil.setMateInformationOnSupplementalAlignment` (mate ref/pos/strand, mate-unmapped flag, TLEN = -matePrimary.TLEN, MC when mate mapped, and MQ unconditionally). Both the single-threaded and threaded paths are refactored. - R2-CLIP-03: `--clip-bases-past-mate` bounded the mate window with unclipped start/end (soft + HARD clips); fgbio uses `mate.unSoftClipped{Start,End}` (soft only, since hard-clipped bases are physically absent). Added soft-only `unsoftclipped_{start,end}[_raw]` helpers and used them in both clippers, so a hard-clipped mate no longer widens the window and under-clips. - R2-CLIP-04: `--sort-order` on clip only relabeled the SO header field (a no-op); removed the flag entirely (rely on a downstream `fgumi sort`). fgbio-oracle parity (fgbio 4.1.0 ClipBam, default Hard): - CHIM (len-3 chimeric template, --clip-overlapping-reads): before DIFFER (fgumi left 100M/100M unclipped, supp mate-info stale) -> after MATCH (75M25H/25H75M, supp gets MC/MQ/mate-pos/TLEN like fgbio). - HARDMATE (FR dovetail, reverse mate 50M50H, --clip-bases-past-mate): before DIFFER (fgumi 110M10H) -> after MATCH (60M60H, past mate's soft end). - SORTFLAG: --sort-order now rejected ("unexpected argument"). Fixture: reports/fgbio-parity-fixtures/r2-clip-02-03-04.sh (+ before/after evidence). Tests: soft-only helper unit tests (typed + raw), find_primary_pair_indices partition test; full suite 2213 passed; ci-fmt/ci-lint clean.
8dd3177 to
6783d10
Compare
|
Re: the outside-diff suggestion to extract the duplicated fixed-position-clip decision from the threaded loop ( The finding itself is tagged 🔵 Trivial / ⚖️ Poor tradeoff, and I agree with that self-assessment: the threaded path and |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/commands/clip.rs`:
- Around line 305-332: The match handling primary read indices omits templates
containing only a primary R2, so they bypass clipping and mate-info repair. In
both match sites using find_primary_pair_indices, add a (None, Some(i2)) arm
mirroring the existing (Some(i1), None) handling: clip records[i2], and perform
any corresponding lone-read mate-info repair required by that path.
🪄 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: c6292d24-1aa6-4484-be58-b9daf5d49bf3
📒 Files selected for processing (4)
crates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/record_utils.rssrc/lib/commands/clip.rstests/integration/test_clip_command.rs
This branch was previously deployed
1 participant
Round-2 fgbio-parity fixes for
clip, stacked on #492 (CLIP-01 /setMateInfo). Auto-retargets tomainwhen #492 merges. Tracker:reports/2026-07-08-fgbio-behavioral-parity-tracker-round2.md§R2.4 (PR 4).Findings
R2-CLIP-02 (S1) — chimeric/split templates left completely unclipped. The clip loop was
if len==1 {frag} else if len==2 {pair}with no else, so any template with a secondary/supplementary read (len > 2, i.e. any chimeric/split-read pair) was passed through unclipped with no mate fixing. Now the primary R1/R2 are located by SAM flags (fgbioClipBam'sTemplate.r1/r2), clipped, and each supplementary alignment has its mate info repaired via a port of htsjdk 5.0.0SamPairUtil.setMateInformationOnSupplementalAlignment— mate ref/pos/strand, mate-unmapped flag,TLEN = -matePrimary.TLEN,MC(when the mate is mapped), andMQ(unconditionally, as of htsjdk 5.0.0). Both the single-threaded and threaded paths are refactored identically.R2-CLIP-03 (S2) —
--clip-bases-past-mateused a soft+hard mate window. The mate window was bounded withunclipped_{start,end}(soft and hard clips); fgbio usesmate.unSoftClipped{Start,End}(soft only — hard-clipped bases are physically absent). When the mate carries hard clips the window was too wide and the read was under-clipped. Added soft-onlyunsoftclipped_{start,end}[_raw]helpers and used them in both the typed and raw clippers.R2-CLIP-04 (S1) —
--sort-orderwas a no-op. It only relabeled theSOheader field; neither path re-sorted, so--sort-order coordinateproduced a BAM labeled coordinate but physically query-grouped. Per the tracker decision the flag is removed (consistent with round-1 FILT-03); rely on a downstreamfgumi sort.R2-CLIP-01 (mate-info recompute) was already resolved by #492's
setMateInfoport — verified, no further change.fgbio-oracle parity (§0 step 2/4)
Oracle = fgbio 4.1.0
ClipBam(default clipping mode Hard).fgumi compare bams --mode content:--clip-overlapping-reads100M/100Munclipped; supp mate-info stale, noMC/MQ)75M25H/25H75M; supp getsMC:Z:25H75M MQ:i:60, mate-pos 175,TLEN 150)50M50H,--clip-bases-past-mate110M10H— past the soft+hard end 209)60M60H— past the mate's soft end 159)clip … --sort-order coordinateunexpected argument '--sort-order')Fixture (added to the PR-12 corpus):
reports/fgbio-parity-fixtures/r2-clip-02-03-04.shwith before/after evidence.Tests / CI
unsoftclipped_*helpers (typed + raw, hard-clip-ignoring);find_primary_pair_indices(secondary/supplementary skipped, order-independent, lone fragment).--sort-orderflag.cargo ci-fmt/cargo ci-lintclean; full suite 2213 passed.Merge order
Merge #492 → this. It auto-retargets to
mainonce #492 lands.Summary by CodeRabbit
--sort-orderoption; clip output preserves input order (coordinate-sorted output requiresfgumi sort).clip --threadsintegration coverage with exact CIGAR/flag assertions, including secondary-only and supplementary mate repair scenarios.