From 86c94f9cce9ab9e0483647a214c9496bb371ecf7 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Sat, 1 Aug 2026 10:03:57 -0700 Subject: [PATCH] docs(zipper): name the `tc` tag in comments that still say `pa` `fgumi zipper` writes the template-coordinate sort key as `tc`; it has been spelled that way since 0.2.0, when the tag was renamed to avoid colliding with bwa-mem's `pa:f` (primary-alignment score fraction). The code follows suit -- `add_template_coordinate_tags_raw` appends `SamTag::TC`, and `tc_info_from_raw` reads it -- but roughly forty doc comments, inline comments, and assertion messages in `zipper.rs` still call it the `pa` tag, including the section banner "Primary Alignment Tag (pa) Tests", which names the very tag the rename exists to avoid. Rename those to `tc`, along with the `has_pa_tag` local. The `--skip-pa-tags` alias is untouched: that flag really is spelled with `pa` and stays accepted. In `fgumi-raw-bam`, `test_dedup_pa_tag_check_fails_on_b_array` keeps its `pa` literal -- it reproduces a historical dedup bug -- but its comment claimed the tag was "produced by fgumi zipper", which stopped being true in 0.2.0. Say what it actually is, and note that the tag name is incidental to the bug: what defeats the dedup check is the `B:i` array type, which `tc` still uses. Comments and assertion messages only; no behavior change. --- crates/fgumi-raw-bam/src/tags.rs | 6 ++- src/lib/commands/zipper.rs | 76 ++++++++++++++++---------------- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/crates/fgumi-raw-bam/src/tags.rs b/crates/fgumi-raw-bam/src/tags.rs index d3c2eb3f0..e68d73955 100644 --- a/crates/fgumi-raw-bam/src/tags.rs +++ b/crates/fgumi-raw-bam/src/tags.rs @@ -2224,7 +2224,7 @@ mod tests { assert_eq!(find_string_tag(&aux, b"XI"), None); } - // --- B:i array (the pa tag type) --- + // --- B:i array (the type the `tc` template-coordinate tag uses) --- #[test] fn test_find_string_tag_cannot_find_b_int_array() { @@ -3535,7 +3535,9 @@ mod tests { #[test] fn test_dedup_pa_tag_check_fails_on_b_array() { - // Simulate the exact pa tag as produced by fgumi zipper: pa:B:i,0,27_056_961,0,207,60005,1 + // The legacy `pa:B:i,0,27_056_961,0,207,60005,1` sort key that `fgumi zipper` wrote + // before 0.2.0, when the tag was renamed to `tc`. The name is incidental to the bug — + // what defeats the dedup check is the `B:i` array type, which `tc` still uses. let aux = make_b_int_array_tag(*b"pa", &[0, 27_056_961, 0, 207, 60005, 1]); let pa_tag_bytes: [u8; 2] = *b"pa"; diff --git a/src/lib/commands/zipper.rs b/src/lib/commands/zipper.rs index 4b5a14a20..3725c496e 100644 --- a/src/lib/commands/zipper.rs +++ b/src/lib/commands/zipper.rs @@ -284,7 +284,7 @@ pub fn build_output_header(unmapped: &Header, mapped: &Header, dict_path: &Path) Ok(header.build()) } -/// Adds `pa` tags to secondary/supplementary reads. +/// Adds `tc` tags to secondary/supplementary reads. fn add_template_coordinate_tags_raw(mapped: &mut Template) { let rr = &mapped.records; @@ -1150,12 +1150,12 @@ mod tests { .expect("raw_record_to_record_buf failed") } - /// Extract `TemplateCoordinateInfo` from a raw BAM record's `pa` tag. + /// Extract `TemplateCoordinateInfo` from a raw BAM record's `tc` tag. /// - /// Returns `None` if the `pa` tag is absent or malformed. + /// Returns `None` if the `tc` tag is absent or malformed. fn tc_info_from_raw(rec: &RawRecord) -> Option { let arr = rec.tags().find_array(SamTag::TC)?; - // pa tag is B:i with 6 int32 elements + // tc tag is B:i with 6 int32 elements if arr.elem_type != b'i' || arr.count != 6 { return None; } @@ -2299,7 +2299,7 @@ mod tests { } // ========================================================================= - // Primary Alignment Tag (pa) Tests + // Template Coordinate Tag (tc) Tests // ========================================================================= // Helper for creating records with specific flags @@ -2310,7 +2310,7 @@ mod tests { const FLAG_SUPPLEMENTARY: u16 = 0x800; const FLAG_REVERSE: u16 = 0x10; - /// Tests that the pa tag is added to supplementary reads with template sort key + /// Tests that the tc tag is added to supplementary reads with template sort key #[test] fn test_add_pa_tag_to_supplementary_r1() -> Result<()> { use crate::template::Template; @@ -2347,15 +2347,15 @@ mod tests { // Call add_template_coordinate_tags_raw add_template_coordinate_tags_raw(&mut template); - // Check that supplementary has pa tag + // Check that supplementary has tc tag let supp = template .records() .iter() .find(|r| r.is_supplementary()) .expect("Should have supplementary"); - // Parse and verify the pa tag - let tc_info = tc_info_from_raw(supp).expect("Supplementary should have pa tag"); + // Parse and verify the tc tag + let tc_info = tc_info_from_raw(supp).expect("Supplementary should have tc tag"); // Only R1 mapped, so both positions should be the same assert_eq!(tc_info.tid1, 0, "tid1 should be R1's reference"); @@ -2365,14 +2365,14 @@ mod tests { assert_eq!(tc_info.pos2, 100, "pos2 should equal pos1 (single read)"); assert!(!tc_info.neg2, "neg2 should equal neg1 (single read)"); - // Check that primary does NOT have pa tag + // Check that primary does NOT have tc tag let primary = template.r1().expect("Should have primary R1"); - assert!(tc_info_from_raw(primary).is_none(), "Primary should not have pa tag"); + assert!(tc_info_from_raw(primary).is_none(), "Primary should not have tc tag"); Ok(()) } - /// Tests that the pa tag is added to secondary reads with correct strand info + /// Tests that the tc tag is added to secondary reads with correct strand info #[test] fn test_add_pa_tag_to_secondary_reverse_strand() -> Result<()> { use crate::template::Template; @@ -2411,8 +2411,8 @@ mod tests { let secondary = template.records().iter().find(|r| r.is_secondary()).expect("Should have secondary"); - // Parse and verify the pa tag - let tc_info = tc_info_from_raw(secondary).expect("Secondary should have pa tag"); + // Parse and verify the tc tag + let tc_info = tc_info_from_raw(secondary).expect("Secondary should have tc tag"); // Primary is on reverse strand with 8M cigar // Unclipped 5' position for reverse strand = alignment_start + alignment_span - 1 @@ -2424,7 +2424,7 @@ mod tests { Ok(()) } - /// Tests that pa tag contains full template sort key for paired-end data + /// Tests that tc tag contains full template sort key for paired-end data #[test] fn test_add_pa_tag_paired_end_r2_supplementary() -> Result<()> { use crate::template::Template; @@ -2483,10 +2483,10 @@ mod tests { .find(|r| r.is_supplementary()) .expect("Should have supplementary"); - // Parse and verify the pa tag - let tc_info = tc_info_from_raw(supp).expect("Supplementary R2 should have pa tag"); + // Parse and verify the tc tag + let tc_info = tc_info_from_raw(supp).expect("Supplementary R2 should have tc tag"); - // pa tag should contain BOTH primaries' unclipped 5' positions (sorted by position) + // tc tag should contain BOTH primaries' unclipped 5' positions (sorted by position) // R1 forward strand at 100 with 8M -> unclipped 5' = 100 // R2 reverse strand at 300 with 8M -> unclipped 5' = 300 + 8 - 1 = 307 assert_eq!(tc_info.tid1, 0, "tid1 should be R1's reference (earlier)"); @@ -2499,7 +2499,7 @@ mod tests { Ok(()) } - /// Tests that no pa tag is added when there's no corresponding primary + /// Tests that no tc tag is added when there's no corresponding primary #[test] fn test_no_pa_tag_when_no_primary() -> Result<()> { use crate::template::Template; @@ -2528,16 +2528,16 @@ mod tests { .find(|r| r.is_supplementary()) .expect("Should have supplementary"); - // Should NOT have pa tag since there's no primary + // Should NOT have tc tag since there's no primary assert!( tc_info_from_raw(supp).is_none(), - "Supplementary without primary should not have pa tag" + "Supplementary without primary should not have tc tag" ); Ok(()) } - /// Tests that pa tag is added during full merge operation + /// Tests that tc tag is added during full merge operation #[test] fn test_pa_tag_added_during_merge() -> Result<()> { let mut unmapped = FgSamBuilder::new_unmapped(); @@ -2552,7 +2552,7 @@ mod tests { // R1 at 100, R2 at 200 let _ = mapped.add_pair().name("q1").start1(100).start2(200).build(); - // Add supplementary R1 (same ref but different pos to test pa tag) + // Add supplementary R1 (same ref but different pos to test tc tag) let supp = { let mut b = RawSamBuilder::new(); b.read_name(b"q1") @@ -2575,15 +2575,15 @@ mod tests { .find(|r| r.flags().is_supplementary()) .expect("Should have supplementary in output"); - // Check pa tag was added + // Check tc tag was added let tc_value = - supp_record.data().get(&TC_TAG).expect("Supplementary should have pa tag after merge"); + supp_record.data().get(&TC_TAG).expect("Supplementary should have tc tag after merge"); - // Parse and verify the pa tag + // Parse and verify the tc tag let tc_info = TemplateCoordinateInfo::from_tag_value(tc_value) - .expect("Should be able to parse pa tag"); + .expect("Should be able to parse tc tag"); - // pa tag should contain both primaries' unclipped 5' positions + // tc tag should contain both primaries' unclipped 5' positions // R1: forward strand at 100 with 100M -> unclipped 5' = 100 // R2: reverse strand at 200 with 100M -> unclipped 5' = 200 + 100 - 1 = 299 assert_eq!(tc_info.tid1, 0, "tid1 should be 0"); @@ -2639,15 +2639,15 @@ mod tests { // Call add_template_coordinate_tags_raw - should return early add_template_coordinate_tags_raw(&mut template); - // Verify no pa tags were added (primaries don't get pa tags) + // Verify no tc tags were added (primaries don't get tc tags) for record in template.records() { - assert!(tc_info_from_raw(record).is_none(), "Primary reads should not have pa tag"); + assert!(tc_info_from_raw(record).is_none(), "Primary reads should not have tc tag"); } Ok(()) } - /// Tests that `add_template_coordinate_tags_raw` only adds pa tag to secondary/supplementary, + /// Tests that `add_template_coordinate_tags_raw` only adds tc tag to secondary/supplementary, /// not to primary reads, even when secondary/supplementary are present. #[test] fn test_add_pa_tag_only_to_secondary_supplementary() -> Result<()> { @@ -2687,9 +2687,9 @@ mod tests { // Check each record for record in template.records() { if record.is_secondary() { - assert!(tc_info_from_raw(record).is_some(), "Secondary should have pa tag"); + assert!(tc_info_from_raw(record).is_some(), "Secondary should have tc tag"); } else { - assert!(tc_info_from_raw(record).is_none(), "Primary should not have pa tag"); + assert!(tc_info_from_raw(record).is_none(), "Primary should not have tc tag"); } } @@ -2746,11 +2746,11 @@ mod tests { // Test with skip_tc_tags = true merge_raw(&unmapped, &mut template, &tag_info, true)?; - // Supplementary should NOT have pa tag when skip_tc_tags is true + // Supplementary should NOT have tc tag when skip_tc_tags is true for record in template.records() { assert!( tc_info_from_raw(record).is_none(), - "No records should have pa tag when skip_tc_tags=true" + "No records should have tc tag when skip_tc_tags=true" ); } @@ -2807,13 +2807,13 @@ mod tests { // Test with skip_tc_tags = false merge_raw(&unmapped, &mut template, &tag_info, false)?; - // Supplementary SHOULD have pa tag when skip_tc_tags is false - let has_pa_tag = template + // Supplementary SHOULD have tc tag when skip_tc_tags is false + let has_tc_tag = template .records() .iter() .any(|r| r.is_supplementary() && tc_info_from_raw(r).is_some()); - assert!(has_pa_tag, "Supplementary should have pa tag when skip_tc_tags=false"); + assert!(has_tc_tag, "Supplementary should have tc tag when skip_tc_tags=false"); Ok(()) }