chore: suit some of Tim's review

fgpyo/sam/__init__.py

@@ -1222,6 +1222,12 @@ def set_tag(
         for rec in self.all_recs():
             rec.set_tag(tag, value)
 
+    def with_aux_alignments(self) -> "Template":
+        """Rebuild this template by adding auxiliary alignments from primary alignment tags."""
+        r1_aux = iter([]) if self.r1 is None else AuxAlignment.many_pysam_from_primary(self.r1)
+        r2_aux = iter([]) if self.r2 is None else AuxAlignment.many_pysam_from_primary(self.r2)
+        return self.build(recs=chain(self.all_recs(), r1_aux, r2_aux))
+
 
 class TemplateIterator(Iterator[Template]):
     """
@@ -1352,13 +1358,14 @@ def reference_end(self) -> int:
 
     @classmethod
     def from_tag_value(cls, tag: str, value: str) -> Self:
-        """Parse a single auxiliary alignment from a single value from a given SAM tag.
+        """Parse a single auxiliary alignment from a single value in a given SAM tag.
 
         Args:
             tag: The SAM tag used to store the value.
             value: The SAM tag value encoding a single auxiliary alignment.
 
         Raises:
+            ValueError: If `tag` is not one of `SA`, `XA`, or `XB`.
             ValueError: If `tag` is `SA` and `value` does not have 6 comma-separated fields.
             ValueError: If `tag` is `XA` and `value` does not have 4 comma-separated fields.
             ValueError: If `tag` is `XA` and `value` does not have 6 comma-separated fields.
@@ -1370,7 +1377,10 @@ def from_tag_value(cls, tag: str, value: str) -> Self:
 
         fields: list[str] = value.rstrip(",").split(",")
 
-        if tag == "SA" and len(fields) == 6:
+        if tag not in cls.SAM_TAGS:
+            raise ValueError(f"Tag {tag} is not one of {", ".join(cls.SAM_TAGS)}!")
+
+        elif tag == "SA" and len(fields) == 6:
             reference_name, reference_start, strand, cigar, mapq, edit_distance = fields
 
             if strand not in ("+", "-"):
@@ -1525,10 +1535,3 @@ def many_pysam_from_primary(cls, primary: AlignedSegment) -> Iterator[AlignedSeg
             aux.set_tag("rh", True)
 
             yield aux
-
-    @classmethod
-    def add_all_to_template(cls, template: Template) -> Template:
-        """Rebuild a template by adding auxiliary alignments from the primary alignment tags."""
-        r1_aux = iter([]) if template.r1 is None else cls.many_pysam_from_primary(template.r1)
-        r2_aux = iter([]) if template.r2 is None else cls.many_pysam_from_primary(template.r2)
-        return Template.build(recs=chain(template.all_recs(), r1_aux, r2_aux))

tests/fgpyo/sam/test_aux_alignment.py

@@ -5,7 +5,6 @@
 from fgpyo.sam import NO_QUERY_BASES
 from fgpyo.sam import AuxAlignment
 from fgpyo.sam import Cigar
-from fgpyo.sam import Template
 from fgpyo.sam import sum_of_base_qualities
 from fgpyo.sam.builder import SamBuilder
 from fgpyo.sequence import reverse_complement
@@ -172,22 +171,28 @@ def test_auxiliary_alignment_validation(kwargs: dict[str, Any], error: str) -> N
         ],
     ],
 )
-def test_auxiliary_alignment_from_tag_item(tag: str, value: str, expected: AuxAlignment) -> None:
+def test_auxiliary_alignment_from_tag_value(tag: str, value: str, expected: AuxAlignment) -> None:
     """Test that we can build an SA, XA, or XB from a item of the tag value."""
     assert AuxAlignment.from_tag_value(tag, value) == expected
 
 
 @pytest.mark.parametrize("tag", ["SA", "XA", "XB"])
-def test_many_from_tag_item_invalid_number_of_commas(tag: str) -> None:
+def test_from_tag_value_invalid_number_of_commas(tag: str) -> None:
     """Test that we raise an exception if we don't have the right number of fields."""
     with pytest.raises(
-        ValueError, match=rf"{tag} tag value has the incorrect number of fields: chr9\,104599381"
+        ValueError, match=rf"{tag} tag value has the incorrect number of fields: chr9,104599381"
     ):
         AuxAlignment.from_tag_value(tag, "chr9,104599381")
 
 
+def test_from_tag_value_raises_invalid_tag() -> None:
+    """Test that we raise an exception if we receive an unsupported SAM tag."""
+    with pytest.raises(ValueError, match="Tag XF is not one of SA, XA, XB!"):
+        AuxAlignment.from_tag_value("XF", "chr3,+170653467,49M,4")
+
+
 @pytest.mark.parametrize("stranded_start", ["!1", "1"])
-def test_many_from_tag_item_raises_for_invalid_xa_stranded_start(stranded_start: str) -> None:
+def test_from_tag_value_raises_for_invalid_xa_stranded_start(stranded_start: str) -> None:
     """Test that we raise an exception when stranded start is malformed for an XA value."""
     with pytest.raises(
         ValueError, match=f"The stranded start field is malformed: {stranded_start}"
@@ -196,7 +201,7 @@ def test_many_from_tag_item_raises_for_invalid_xa_stranded_start(stranded_start:
 
 
 @pytest.mark.parametrize("stranded_start", ["!1", "1"])
-def test_many_from_tag_item_raises_for_invalid_xb_stranded_start(stranded_start: str) -> None:
+def test_from_tag_value_raises_for_invalid_xb_stranded_start(stranded_start: str) -> None:
     """Test that we raise an exception when stranded start is malformed for an XA value."""
     with pytest.raises(
         ValueError, match=f"The stranded start field is malformed: {stranded_start}"
@@ -591,22 +596,3 @@ def test_many_pysam_from_primary_with_hard_clips() -> None:
     (actual,) = AuxAlignment.many_pysam_from_primary(rec)
 
     assert actual.query_sequence == NO_QUERY_BASES
-
-
-def test_add_to_template() -> None:
-    """Test that we add secondary alignments as SAM records to a template."""
-    supplementary: str = "chr9,104599381,-,39M,50,2"
-    secondary: str = "chr9,-104599381,49M,4,0,30;chr3,+170653467,49M,4,0,20;;;"  # with trailing ';'
-    builder = SamBuilder()
-    rec = builder.add_single(chrom="chr1", start=32)
-    rec.set_tag("RX", "ACGT")
-
-    assert list(AuxAlignment.many_from_primary(rec)) == []
-
-    rec.set_tag("SA", supplementary)
-    rec.set_tag("XB", secondary)
-
-    actual = AuxAlignment.add_all_to_template(Template.build([rec]))
-    expected = Template.build([rec] + list(AuxAlignment.many_pysam_from_primary(rec)))
-
-    assert actual == expected

tests/fgpyo/sam/test_template_iterator.py

@@ -2,6 +2,7 @@
 
 import pytest
 
+from fgpyo.sam import AuxAlignment
 from fgpyo.sam import Template
 from fgpyo.sam import TemplateIterator
 from fgpyo.sam import reader
@@ -219,3 +220,22 @@ def test_set_tag() -> None:
     for bad_tag in ["", "A", "ABC", "ABCD"]:
         with pytest.raises(AssertionError, match="Tags must be 2 characters"):
             template.set_tag(bad_tag, VALUE)
+
+
+def test_with_aux_alignments() -> None:
+    """Test that we add auxiliary alignments as SAM records to a template."""
+    secondary: str = "chr9,-104599381,49M,4,0,30;chr3,+170653467,49M,4,0,20;;;"  # with trailing ';'
+    supplementary: str = "chr9,104599381,-,39M,50,2"
+    builder = SamBuilder()
+    rec = builder.add_single(chrom="chr1", start=32)
+    rec.set_tag("RX", "ACGT")
+
+    assert list(AuxAlignment.many_from_primary(rec)) == []
+
+    rec.set_tag("SA", supplementary)
+    rec.set_tag("XB", secondary)
+
+    actual = Template.build([rec]).with_aux_alignments()
+    expected = Template.build([rec] + list(AuxAlignment.many_pysam_from_primary(rec)))
+
+    assert actual == expected