Skip to content

fix: recompute the BAM bin field after raw POS/CIGAR mutations (clip, zipper) - #591

Merged
nh13 merged 4 commits into
mainfrom
nh/fix-stale-bam-bin
Jul 19, 2026
Merged

nh13 merged 4 commits into
mainfrom
nh/fix-stale-bam-bin

Conversation

@nh13

@nh13 nh13 commented Jul 18, 2026 •

Copy link
Copy Markdown
Member

Summary

fgumi clip (and, in a narrower case, fgumi zipper) emitted a stale BAM bin field. The raw pipelines mutate an already-encoded BAM record in place and write the bytes verbatim, so — unlike the noodles/htsjdk encoders, which recompute the indexing bin on write — there is no serialization step to refresh bin (bytes 10–11) after POS or the CIGAR changes. Any clip that moved a read into a different bin shipped the pre-clip value, diverging from fgbio.

Reproduction

A read placed at 16300 with 200M straddles the 16 kb bin boundary (level-4 bin 585). A 116-base 5′ soft-clip moves it to pos 16416, 116S84M, whose correct bin is 4682:

Tool resulting alignment bin
fgbio ClipBam pos 16417, 116S84M 4682 ✓
fgumi clip (before) pos 16417, 116S84M 585 ✗ (stale)

The same class of bug affected zipper's mate reconciliation: a read relocated to its mapped mate's coordinate kept the stale unmapped bin (4680) instead of its placed position's bin.

Note: fgumi's own BAI/CSI writer recomputes start/end from POS+CIGAR and ignores the record's bin, so its own index was unaffected. The stale bin hurt external consumers (htsjdk/samtools-based tools reading fgumi output) and broke byte-parity with fgbio.

What changed

  • fgumi-raw-bam::bin (new module): the canonical SAM spec §5.3 reg2bin, the UNMAPPED_BIN constant, and bin_from_raw_bam / set_bin_from_raw_bam, which compute a record's bin from its POS and CIGAR reference span (unmapped POS < 0 → 4680, keying off the alignment start like htsjdk computeIndexingBin, not the unmapped flag). Plus a RawRecord::recompute_bin() wrapper.
  • clip: recompute the bin at the three raw mutation primitives (clip_start_of_alignment, clip_end_of_alignment, make_read_unmapped_raw) so a clipped record is always self-consistent, and in set_mate_info_raw when a clipped-unmapped read is relocated or a pair is cleared to unmapped. upgrade_clipping only converts clip type (no POS/span move), so it correctly needs no recompute.
  • zipper (Template::fix_mate_info): recompute after relocating an unmapped mate to its mate's coordinate and after clearing a pair to unmapped.
  • simulate: region_to_bin now delegates to the shared reg2bin instead of duplicating the spec code; the private UNMAPPED_BIN in builder.rs also shares the constant.

Testing

  • Unit tests at every mutation site (clip start/end/unmap, set_mate_info_raw relocation, fix_mate_info relocation), each asserting the emitted bin equals reg2bin of the post-op coordinates — TDD, each written failing first.
  • reg2bin is covered by the full authoritative vector table ported from htsjdk GenomicIndexUtilTest.testRegionToBinDataProvider, exercising every binning level (16 kb through the whole-window bin 0).
  • End-to-end: rebuilt binary re-run on the reproductions above — every previously-stale case now matches reg2bin of the post-op coordinates, and directly matches fgbio ClipBam / ZipperBams output bins.
  • Full workspace suite (5243 tests), cargo ci-fmt, and cargo ci-lint all pass.

Reading order

  1. crates/fgumi-raw-bam/src/bin.rs — the shared helper and its semantics.
  2. crates/fgumi-sam/src/clipper.rs and src/lib/commands/clip.rs — the clip fix.
  3. src/lib/template.rs — the zipper fix.
  4. src/lib/commands/simulate/mod.rs — the delegation refactor.

Summary by CodeRabbit

  • New Features

    • Added BAM bin calculation and update support based on alignment position and CIGAR span.
    • Added APIs to compute or refresh bins for raw BAM records.
    • Added automatic bin updates when clipping reads or modifying mate information.
  • Bug Fixes

    • Prevented stale bin values after coordinate, CIGAR, or mapping-status changes.
    • Correctly handles unmapped and zero-reference-span reads.
  • Tests

    • Added coverage for bin boundaries, clipping, unmapped reads, and coordinate mutations.

nh13 added 4 commits July 18, 2026 13:27
Add a bin module with the canonical SAM spec §5.3 reg2bin, the
UNMAPPED_BIN constant, and bin_from_raw_bam/set_bin_from_raw_bam that
compute a record's index bin from its POS and CIGAR reference span
(unmapped records get 4680). Expose RawRecord::recompute_bin as an
ergonomic wrapper.

The raw BAM pipelines mutate encoded records in place and emit the bytes
verbatim, so unlike the noodles/htsjdk encoders there is no serialization
step to refresh the bin field after a POS or CIGAR change. These helpers
are the raw-byte equivalent, to be called at each mutation site. Point the
builder's private UNMAPPED_BIN at the shared constant.
…ps a read

The raw clipper mutated POS (start-clip), the CIGAR (start/end-clip), and
unmapped reads whose bases were fully clipped, but never refreshed the
index bin (bytes 10-11) — and clip writes records verbatim, so the stale
pre-clip bin shipped in the output. fgbio recomputes it (htsjdk refreshes
the indexing bin on write), so fgumi diverged for any clip that moved a
read into a different bin.

Recompute the bin at the three raw mutation primitives
(clip_start_of_alignment, clip_end_of_alignment, make_read_unmapped_raw)
so a clipped record is always self-consistent, and in set_mate_info_raw
when a clipped-unmapped read is relocated to its mapped mate's coordinate
or a pair is cleared to unmapped. upgrade_clipping only converts clip
type without moving POS or the aligned span, so it needs no recompute.
…unmaps a read

Template::fix_mate_info (used by zipper) places an unmapped read at its
mapped mate's coordinate and clears both reads to unmapped without
touching the index bin, so a relocated read kept the stale unmapped bin
(4680) instead of its placed position's bin. fgbio emits the placed bin.

Recompute the bin after set_mate_info_one_unmapped relocates the read and
after set_mate_info_both_unmapped clears the pair.
region_to_bin duplicated the SAM spec reg2bin reference code. Delegate to
fgumi_raw_bam::reg2bin so the simulate encoder and the raw clip/zipper
pipelines share one implementation of the binning scheme, keeping the same
1-based-inclusive input contract.
@nh13
nh13 temporarily deployed to github-actions July 18, 2026 17:29 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 18, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a2f8bd2d-da4d-4d41-a05a-8c4ac457fb94

📥 Commits

Reviewing files that changed from the base of the PR and between 602fd60 and c65673e.

📒 Files selected for processing (8)
  • crates/fgumi-raw-bam/src/bin.rs
  • crates/fgumi-raw-bam/src/builder.rs
  • crates/fgumi-raw-bam/src/lib.rs
  • crates/fgumi-raw-bam/src/raw_bam_record.rs
  • crates/fgumi-sam/src/clipper.rs
  • src/lib/commands/clip.rs
  • src/lib/commands/simulate/mod.rs
  • src/lib/template.rs

Walkthrough

Changes

BAM bin calculation is centralized in fgumi-raw-bam, exported publicly, and applied after raw POS/CIGAR or mate-coordinate mutations. Clipping, mate fixing, template updates, and simulated-region binning now maintain consistent bin values, with boundary and unmapped cases covered by tests.

BAM bin recomputation

Layer / File(s) Summary
Shared bin calculation contract
crates/fgumi-raw-bam/src/bin.rs, crates/fgumi-raw-bam/src/builder.rs, crates/fgumi-raw-bam/src/lib.rs
Adds SAM-compatible reg2bin, raw BAM bin extraction and in-place writing, exports the APIs, and centralizes UNMAPPED_BIN.
RawRecord bin refresh
crates/fgumi-raw-bam/src/raw_bam_record.rs
Adds RawRecord::recompute_bin and tests updates after POS, CIGAR, and unmapped-state mutations.
Raw clipping updates
crates/fgumi-sam/src/clipper.rs
Recomputes bins after start clipping, end clipping, and conversion to unmapped records, with boundary tests.
Coordinate update integrations
src/lib/commands/clip.rs, src/lib/commands/simulate/mod.rs, src/lib/template.rs
Refreshes bins after mate relocation or unmapping and delegates simulated region binning to the shared implementation.

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

Sequence Diagram(s)

sequenceDiagram
  participant RawMutation
  participant RawRecord
  participant BinAPI
  participant BAMBytes
  RawMutation->>RawRecord: change POS, CIGAR, or mapping state
  RawRecord->>BinAPI: recompute current raw record bin
  BinAPI->>BAMBytes: write little-endian bin bytes
  BAMBytes-->>RawRecord: updated BAM bin
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: recomputing BAM bin fields after raw POS/CIGAR mutations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/fix-stale-bam-bin

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

@codecov

codecov Bot commented Jul 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.02%. Comparing base (f577f55) to head (c65673e).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
- Coverage   93.03%   93.02%   -0.01%     
==========================================
  Files         167      168       +1     
  Lines      103266   103429     +163     
==========================================
+ Hits        96070    96217     +147     
- Misses       7196     7212      +16     

☔ 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 Jul 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 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 merged commit b9966fc into main Jul 19, 2026
12 checks passed
@nh13
nh13 deleted the nh/fix-stale-bam-bin branch July 19, 2026 20:31
@nh13 nh13 mentioned this pull request Jul 19, 2026

This branch was previously deployed

1 inactive deployment
github-actions — c65673ec Deployed Jul 18, 2026 by nh13 via coverage #2712
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