Skip to content

fix(compare): exclude BAM bin field from core field comparison - #208

Merged
nh13 merged 1 commit into
mainfrom
nh/fix-compare-bams-tag-order
Mar 31, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/fix-compare-bams-tag-order

Conversation

@nh13

@nh13 nh13 commented Mar 31, 2026

Copy link
Copy Markdown
Member

Summary

  • Exclude the BAM bin field (bytes 10-11) from raw_core_fields_equal so that bin differences between tools no longer cause false core_diff reports
  • Add unit tests for core comparison with differing bin values and for the structured comparison with differing bin + tag order

Context

PR #197 replaced RecordBuf-based field-by-field comparison with raw byte comparison. The raw comparison inadvertently included the BAM bin field, which is a BAM-specific index optimization not part of the SAM data model. Different tools (e.g. fgumi vs fgbio) may compute different bin values for the same alignment, causing all records to be flagged as core_diff even when core fields are identical.

Test plan

  • New test test_core_equal_different_bin_values — records with identical core fields but different bin values are reported as equal
  • New test test_structured_different_bin_different_tag_order — records with different bin and different tag order report core_match=true, tags_match=false, tag_order_match=true
  • All existing tests pass (cargo ci-test)
  • Formatting and linting pass (cargo ci-fmt && cargo ci-lint)

Fixes #207

The raw byte comparison introduced in #197 included the BAM `bin` field
(bytes 10-11) when comparing core fields.  The bin field is a BAM-specific
index optimization not part of the SAM data model, and different tools may
compute different values for the same alignment.  This caused false
core_diff reports when comparing outputs from different tools (e.g. fgumi
vs fgbio) that only differed in tag order and bin values.

Skip bytes 10-11 in `raw_core_fields_equal` so that bin differences no
longer trigger false mismatches.

Fixes #207
@nh13
nh13 temporarily deployed to github-actions March 31, 2026 04:56 — with GitHub Actions Inactive
@codecov

codecov Bot commented Mar 31, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.06%. Comparing base (91e5af6) to head (6e25f13).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/commands/compare/raw_compare.rs 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
- Coverage   88.06%   88.06%   -0.01%     
==========================================
  Files         113      113              
  Lines       52795    52820      +25     
==========================================
+ Hits        46495    46516      +21     
- Misses       6300     6304       +4     

☔ View full report in Codecov by Sentry.
📢 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 marked this pull request as ready for review March 31, 2026 05:20
@nh13

nh13 commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Mar 31, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

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.

@coderabbitai

coderabbitai Bot commented Mar 31, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d9aab363-d28b-4d75-ab63-12ad49f10d2e

📥 Commits

Reviewing files that changed from the base of the PR and between 91e5af6 and 6e25f13.

📒 Files selected for processing (1)
  • src/commands/compare/raw_compare.rs

📝 Walkthrough

Walkthrough

The change modifies raw byte comparison logic for BAM core fields by excluding the bin field from equality checks. Two constants define the bin field's offset and length, allowing comparison to concatenate bytes before and after it. The function now validates that aux-data end offsets match before declaring equivalence. Test coverage is expanded to verify records with identical core fields and tag values (but differing bin values or tag ordering) are correctly identified as equivalent.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: excluding the BAM bin field from core field comparison.
Description check ✅ Passed The description clearly relates to the changeset, explaining the bin field exclusion, test additions, and context from PR #197.
Linked Issues check ✅ Passed The PR directly addresses issue #207 by excluding the bin field from core comparison, fixing false core_diff reports when only tag order differs.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the bin field comparison issue and adding relevant test cases; no unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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-compare-bams-tag-order

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 and usage tips.

@nh13
nh13 merged commit 90ec6fe into main Mar 31, 2026
7 checks passed
@nh13
nh13 deleted the nh/fix-compare-bams-tag-order branch March 31, 2026 05:38

This branch was previously deployed

1 inactive deployment
github-actions — 6e25f13b Deployed Mar 31, 2026 by nh13 via coverage #781
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.

compare bams reports false core_diff when only tag order differs

1 participant