Skip to content

fix(zipper)!: rename sort-key tag from pa to tc to avoid bwa-mem pa:f collision - #270

Merged
nh13 merged 1 commit into
mainfrom
nh/fix-zipper-pa-tag-collision
Apr 15, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/fix-zipper-pa-tag-collision

Conversation

@nh13

@nh13 nh13 commented Apr 14, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #268.

fgumi zipper wrote a pa:B:i,<6 int32s> tag on secondary/supplementary records to carry the primary alignments' template-coordinate sort key. bwa-mem already uses pa:f:<float> (primary-alignment score fraction), so fgumi-zippered BAMs carried the same tag name with two different types — readers that pick the wrong interpretation silently returned wrong values, and strict validators rejected the file.

This PR renames the tag from pa to tc (template-coordinate) everywhere it appears: the zipper writer (both RecordBuf and raw-byte paths) and the dedup existence check. tc names what the tag actually stores and is lowercase per the SAM-spec convention for non-standard program tags.

While in the area, TC_TAG and TemplateCoordinateInfo (formerly PA_TAG and PrimaryAlignmentInfo) also moved out of src/lib/sort/ and into crates/fgumi-sam/ alongside SamTag, since they encode a SAM tag and its on-disk layout, not sort-pipeline internals. No behavior change in the sort pipeline itself.

Breaking changes

  • Tag bytes: pa:B:i,... → tc:B:i,... on secondary/supplementary records emitted by fgumi zipper.
  • CLI flag: fgumi zipper --skip-pa-tags → fgumi zipper --skip-tc-tags.
  • Metric field: dedup's missing_pa_tag → missing_tc_tag in JSON/TSV metrics output.
  • Public Rust API: PA_TAG / PrimaryAlignmentInfo → TC_TAG / TemplateCoordinateInfo, moved from fgumi_lib::sort to fgumi_sam (re-exported via fgumi_lib::sam).

Migration

  • Existing fgumi-zippered BAMs must be re-zippered — or the pa tag renamed to tc with a one-off pass — before fgumi dedup will accept them.
  • External tools that parsed pa:B:i from fgumi output must switch to tc:B:i.
  • bwa-mem's own pa:f tag is no longer touched and passes through unchanged.

Test plan

  • cargo ci-test (2479 passed, 19 skipped)
  • cargo ci-fmt
  • cargo ci-lint

… collision

`fgumi zipper` wrote a `pa:B:i,<6 int32s>` tag on secondary/supplementary
records to carry the primary alignments' template-coordinate sort key.
bwa-mem already uses `pa:f:<float>` (primary-alignment score fraction),
so fgumi-zippered BAMs carried the same tag name with two different
types — readers that pick the wrong interpretation silently returned
wrong values.

Rename the tag from `pa` to `tc` (template-coordinate) everywhere it
appears: in the zipper writer (both RecordBuf and raw-byte paths) and
the dedup existence check. `tc` names what the tag actually stores and
is lowercase per the SAM-spec convention for non-standard program tags.

Additionally, move TC_TAG and TemplateCoordinateInfo (formerly PA_TAG
and PrimaryAlignmentInfo) out of src/lib/sort/ and into crates/fgumi-sam/
alongside SamTag, since they encode a SAM tag and its on-disk layout,
not sort-pipeline internals.

Breaking changes:
- Zipper CLI flag --skip-pa-tags renamed to --skip-tc-tags.
- Dedup metric field missing_pa_tag renamed to missing_tc_tag.
- Public Rust items PA_TAG and PrimaryAlignmentInfo renamed to TC_TAG
  and TemplateCoordinateInfo, and moved from the sort module to
  fgumi_sam (re-exported through crate::sam).
- Existing fgumi-zippered BAMs must be re-zippered (or the tag
  manually renamed) before fgumi dedup will accept them.

Fixes #268.
@nh13
nh13 temporarily deployed to github-actions April 14, 2026 23:13 — with GitHub Actions Inactive
@nh13
nh13 marked this pull request as ready for review April 14, 2026 23:14
@coderabbitai

coderabbitai Bot commented Apr 14, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This pull request systematically renames the SAM auxiliary tag emitted by fgumi zipper from pa to tc (template-coordinate), resolving a collision with bwa-mem's pa:f tag. Changes include: introducing a new template_coordinate module in fgumi-sam with TC_TAG constant and TemplateCoordinateInfo struct; replacing the SamTag::PA constant with SamTag::TC; updating dedup command to use TC_TAG and renaming the missing_pa_tag metric to missing_tc_tag; renaming the zipper command's --skip-pa-tags flag to --skip-tc-tags and its internal helper functions; removing the PrimaryAlignmentInfo struct from sort/keys.rs; and updating documentation and tests accordingly.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and specifically describes the main breaking change: renaming the sort-key tag from pa to tc to fix the bwa-mem collision.
Description check ✅ Passed Description is comprehensive and directly related to the changeset, explaining the fix, breaking changes, migration path, and test results.
Linked Issues check ✅ Passed PR fully addresses issue #268: renames pa:B:i tag to tc:B:i across zipper writer and dedup check, passes bwa's pa:f through unchanged, and explains migration for existing zippered BAMs.
Out of Scope Changes check ✅ Passed All changes are scoped to the tag rename objective. The refactoring of PrimaryAlignmentInfo→TemplateCoordinateInfo from sort/ to fgumi_sam/ is a logical reorganization directly supporting the tag rename, not an out-of-scope change.
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-zipper-pa-tag-collision

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.

@codecov

codecov Bot commented Apr 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.39623% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.84%. Comparing base (25ede93) to head (a48009a).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/commands/dedup.rs 71.42% 4 Missing ⚠️
src/lib/commands/zipper.rs 96.73% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
- Coverage   89.84%   89.84%   -0.01%     
==========================================
  Files         120      120              
  Lines       61359    61170     -189     
==========================================
- Hits        55127    54956     -171     
+ Misses       6232     6214      -18     

☔ 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 commented Apr 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 15, 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.

@nh13

nh13 commented Apr 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 15, 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.

@nh13

nh13 commented Apr 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 15, 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.

@nh13

nh13 commented Apr 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 15, 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 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/fgumi-raw-bam/src/tags.rs (1)

2285-2293: Avoid hardcoded source line references in test comments.

“exact check from dedup.rs:932-934” will drift quickly; prefer a symbolic description of the logic.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/fgumi-raw-bam/src/tags.rs` around lines 2285 - 2293, The comment in
test_dedup_tc_tag_check_fails_on_b_array should not reference a hardcoded source
line ("dedup.rs:932-934"); instead update it to describe the logic symbolically
(e.g., "this replicates the same TC tag existence check used by the dedup
logic") so future code moves won't break the comment; locate the test function
test_dedup_tc_tag_check_fails_on_b_array and replace the line mentioning
dedup.rs with a short description referencing the involved symbols
find_string_tag and find_int_tag (or the dedup check function name if one
exists) so the intent is clear without file/line numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 2285-2293: The comment in test_dedup_tc_tag_check_fails_on_b_array
should not reference a hardcoded source line ("dedup.rs:932-934"); instead
update it to describe the logic symbolically (e.g., "this replicates the same TC
tag existence check used by the dedup logic") so future code moves won't break
the comment; locate the test function test_dedup_tc_tag_check_fails_on_b_array
and replace the line mentioning dedup.rs with a short description referencing
the involved symbols find_string_tag and find_int_tag (or the dedup check
function name if one exists) so the intent is clear without file/line numbers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6dfc4170-af5c-4d0a-80a6-6e4ce8f71f73

📥 Commits

Reviewing files that changed from the base of the PR and between 25ede93 and a48009a.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • crates/fgumi-raw-bam/src/tags.rs
  • crates/fgumi-sam/src/lib.rs
  • crates/fgumi-sam/src/tag.rs
  • crates/fgumi-sam/src/template_coordinate.rs
  • docs/src/guide/best-practices.md
  • src/lib/commands/dedup.rs
  • src/lib/commands/zipper.rs
  • src/lib/sam/mod.rs
  • src/lib/sort/keys.rs
  • src/lib/sort/mod.rs
💤 Files with no reviewable changes (1)
  • src/lib/sort/keys.rs

@nh13
nh13 merged commit 40b685c into main Apr 15, 2026
9 checks passed
@nh13
nh13 deleted the nh/fix-zipper-pa-tag-collision branch April 15, 2026 19:25
nh13 added a commit that referenced this pull request Apr 17, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 20, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 20, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 20, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 20, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 20, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 21, 2026
… remaining 48-commit work

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
nh13 added a commit that referenced this pull request Apr 21, 2026
… remaining 48-commit work (#296)

The final PR in the stacked series closes the full delta against the
48-commit backup. In addition to the original scope (vendored bam_codec
deletion + simulate migration), this amends to include:

- Missing new files: sort/keys.rs, 4 raw-bam bench files
- compare/bams.rs RawRecord migration
- codec_caller.rs cascade simplification
- group/filter/correct/extract/clip/bam_io.rs simplifications
- overlapping.rs raw-only API
- Test suite updates for test_compare_bams, test_group_command,
  test_e2e_regression
- Doc touch-ups in best-practices/performance-tuning

Verified: tree matches backup except for 6 main-only files
(prefetch_reader, os_hints, test_async_reader, template_coordinate,
and two benches) that came from main PRs #258/#261/#265/#270/#282
merged after our branch point.

Part 8 of 8 in the series closing #272. Stacks on #295.
@nh13 nh13 mentioned this pull request Apr 22, 2026

This branch was previously deployed

1 inactive deployment
github-actions — a48009a6 Deployed Apr 14, 2026 by nh13 via coverage #1100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zipper emits pa:B:i tag that collides with bwa-mem's pa:f

1 participant