Skip to content

fix(index): position-bin placed-but-unmapped reads in the BAI - #651

Merged
nh13 merged 1 commit into
mainfrom
tf_bai_placed_unmapped
Jul 24, 2026
Merged

nh13 merged 1 commit into
mainfrom
tf_bai_placed_unmapped

Conversation

@tfenne

@tfenne tfenne commented Jul 23, 2026 •

Copy link
Copy Markdown
Member

What & why

A read with the UNMAPPED flag but a valid reference and position — the common "mate mapped, self unmapped" record, which carries its mate's tid/pos so it coordinate-sorts alongside its mate — was dropped from the BAI entirely: no bin, no chunk, no linear-index entry. htslib/samtools position-bin such reads, so region queries and idxstats over a fgumi index silently missed them (a "fetch everything in this window" would lose the unmapped mates, and the idxstats unmapped column disagreed with samtools).

What it does

extract_alignment_context now returns a binning context for any read that has a reference and position, binning it over a 1-base [pos, pos+1) span when it has no CIGAR (matching htslib's bam_endpos = pos + max(rlen, 1)); only a read with no reference/position is treated as unplaced. The is_mapped flag still records the read as unmapped, so it isn't miscounted as mapped — it just becomes queryable, matching htslib. Normal mapped reads (CIGAR span ≥ 1) bin identically to before, so the blast radius is limited to placed-unmapped reads.

This changes .bai content for both index paths (the pooled sort writer and IndexingBamWriter), which is the intent: both now match samtools on content.

Validation

  • New unit test: a placed-but-unmapped record → binned over [pos, pos+1) with is_mapped=false; a mapped read → its CIGAR span; a truly-unplaced read → unplaced.
  • The samtools cross-check (test_sort_write_index_matches_samtools_index) now compares ALL reads (dropping the previous -F 4 mapped-only workaround) and full idxstats — and matches.
  • Full suite green: cargo ci-fmt, cargo ci-lint (clippy pedantic), and all tests including the samtools-gated ones.

Stacked on #650 (BAI compaction); merge that first.

Summary by CodeRabbit

  • Bug Fixes
    • Improved BAM indexing for reads marked as unmapped but containing valid reference positions.
    • These reads are now included in the appropriate genomic regions and represented with their correct mapping status.
    • Corrected indexing for records without CIGAR-derived reference spans by treating them as covering one base.
    • Region queries and index statistics now more closely match samtools results, including placed-but-unmapped reads.

@tfenne
tfenne requested a review from nh13 as a code owner July 23, 2026 18:50
@tfenne
tfenne temporarily deployed to github-actions July 23, 2026 18:50 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 23, 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 Plus

Run ID: e7768404-0efc-4f2f-a034-f2edc1e52a9c

📥 Commits

Reviewing files that changed from the base of the PR and between 50372a6 and e35d4ca.

📒 Files selected for processing (2)
  • crates/fgumi-bam-io/src/writer.rs
  • tests/integration/test_sort_write_index.rs

Walkthrough

Placed-but-unmapped reads with valid coordinates are now indexed over a one-base span, while truly unplaced reads remain excluded. Unit and integration tests verify exact region records and full idxstats parity with samtools.

Changes

BAM indexing behavior

Layer / File(s) Summary
Alignment context and unit coverage
crates/fgumi-bam-io/src/writer.rs
Positioned reads are binned regardless of the UNMAPPED flag; mapping status is preserved, zero-length spans become one base, and unit tests cover mapped, placed-unmapped, and unplaced records.
Index query parity
tests/integration/test_sort_write_index.rs
Integration tests compare exact region-query records and complete idxstats output between fgumi and samtools.
Estimated code review effort: 3 (Moderate) ~20 minutes

Possibly related PRs

Suggested reviewers: nh13

🚥 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 accurately summarizes the main change: BAI indexing now position-bins placed-but-unmapped reads.
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 tf_bai_placed_unmapped

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.

❤️ Share

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

@codecov

codecov Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.57%. Comparing base (50372a6) to head (e35d4ca).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #651   +/-   ##
=======================================
  Coverage   93.57%   93.57%           
=======================================
  Files         175      175           
  Lines      106764   106803   +39     
=======================================
+ Hits        99904    99944   +40     
+ Misses       6860     6859    -1     

☔ 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.

@tfenne
tfenne force-pushed the tf_bai_placed_unmapped branch from 5cdcbe4 to 392c868 Compare July 23, 2026 19:09
@tfenne
tfenne temporarily deployed to github-actions July 23, 2026 19:09 — with GitHub Actions Inactive
Base automatically changed from tf_compact_bai to main July 23, 2026 22:44
A read with the UNMAPPED flag but a valid reference and position — the common "mate mapped, self unmapped" record, which carries its mate's tid/pos so it coordinate-sorts alongside it — was dropped from the index entirely: no bin, no chunk, no linear-index entry. htslib/samtools position-bin such reads, so region queries and idxstats over a fgumi index silently missed them (a fetch of "all reads in this window" would lose the unmapped mates, and idxstats' unmapped column disagreed with samtools).

extract_alignment_context now returns an alignment context for any read that has a reference and position, binning it over a 1-base [pos, pos+1) span when it has no CIGAR (matching htslib's bam_endpos = pos + max(rlen, 1)); only a read with no reference/position is treated as unplaced. The is_mapped flag still records the read as unmapped, so it is not miscounted as mapped — it simply becomes queryable, matching htslib.

The samtools cross-check now asserts equality over ALL reads (dropping the earlier -F 4 mapped-only workaround) and over full idxstats, and passes. This also changes the .bai content for the existing IndexingBamWriter path, which is the intent: both index paths now match samtools.
@nh13
nh13 force-pushed the tf_bai_placed_unmapped branch from 392c868 to e35d4ca Compare July 23, 2026 22:48
@nh13
nh13 temporarily deployed to github-actions July 23, 2026 22:48 — with GitHub Actions Inactive
@nh13

nh13 commented Jul 24, 2026

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 24, 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 43f3734 into main Jul 24, 2026
14 checks passed
@nh13
nh13 deleted the tf_bai_placed_unmapped branch July 24, 2026 15:41
@nh13

nh13 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Thank-you @tfenne!!!

This branch was previously deployed

1 inactive deployment
github-actions — e35d4ca2 Deployed Jul 23, 2026 by nh13 via coverage #2994
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.

2 participants