Merge pull request #260 from ncsa/fix/contig-variants-remove-variant

Fix remove_variant to use position1 instead of position

Author: joshfactorial

neat/variants/contig_variants.py

@@ -49,7 +49,7 @@ def check_if_del(self, other):
 
     def check_if_ins(self, other):
         for insert in self.all_ins:
-            if np.array_equal(other.genotype, insert.genotype) and insert.contains(other):
+            if np.array_equal(other.genotype, insert.genotype) and insert.contains(other.position1):
                 return insert
         return None
 
@@ -150,11 +150,11 @@ def get_sample_info(variant):
             return get_genotype_string(variant.genotype)
 
     def remove_variant(self, variant):
-        if variant.position in self.variant_locations:
-            if variant in self.contig_variants[variant.position]:
-                self.contig_variants[variant.position].remove(variant)
-            if not self.contig_variants[variant.position]:
-                self.variant_locations.remove(variant.position)
+        if variant.position1 in self.variant_locations:
+            if variant in self.contig_variants[variant.position1]:
+                self.contig_variants[variant.position1].remove(variant)
+            if not self.contig_variants[variant.position1]:
+                self.variant_locations.remove(variant.position1)
 
     def __getitem__(self, input_location: int) -> list:
         """

tests/test_variants/test_check_if_ins.py

@@ -0,0 +1,72 @@
+"""
+Regression test for fix/contig-variants-check-if-ins.
+
+check_if_ins was passing the variant object to Insertion.contains() which
+expects an int (a position). This caused the method to always return None
+even when a variant's position falls within an insertion's span.
+"""
+import numpy as np
+
+from neat.variants.contig_variants import ContigVariants
+from neat.variants import Insertion, SingleNucleotideVariant
+
+
+_GT = np.array([0, 1])
+
+
+def _ins(pos, alt="ACGTT", length=4, gt=None):
+    return Insertion(pos, length, alt, gt if gt is not None else _GT.copy(), "42")
+
+
+def _snv(pos, alt="T", gt=None):
+    return SingleNucleotideVariant(pos, alt, gt if gt is not None else _GT.copy(), "42")
+
+
+def test_check_if_ins_finds_containing_insertion():
+    """SNV inside insertion span is detected."""
+    cv = ContigVariants()
+    ins = _ins(10, "ACGTT", 4)
+    cv.add_variant(ins)
+    snv = _snv(11)  # position1=11 is within [10, 14)
+    result = cv.check_if_ins(snv)
+    assert result is ins
+
+
+def test_check_if_ins_at_insertion_start():
+    """SNV at the insertion's own position is detected."""
+    cv = ContigVariants()
+    ins = _ins(10, "ACGTT", 4)
+    cv.add_variant(ins)
+    snv = _snv(10)
+    assert cv.check_if_ins(snv) is ins
+
+
+def test_check_if_ins_at_insertion_end_exclusive():
+    """Position at insertion start + length is outside the span."""
+    cv = ContigVariants()
+    ins = _ins(10, "ACGTT", 4)  # spans [10, 14)
+    cv.add_variant(ins)
+    snv = _snv(14)
+    assert cv.check_if_ins(snv) is None
+
+
+def test_check_if_ins_outside_span_returns_none():
+    cv = ContigVariants()
+    ins = _ins(10, "ACGTT", 4)
+    cv.add_variant(ins)
+    snv = _snv(50)
+    assert cv.check_if_ins(snv) is None
+
+
+def test_check_if_ins_empty_returns_none():
+    cv = ContigVariants()
+    assert cv.check_if_ins(_snv(10)) is None
+
+
+def test_check_if_ins_genotype_mismatch_returns_none():
+    """Even if position matches, different genotype means no match."""
+    cv = ContigVariants()
+    ins = _ins(10, "ACGTT", 4, gt=np.array([1, 0]))
+    cv.add_variant(ins)
+    snv = _snv(11, gt=np.array([0, 1]))  # different genotype
+    assert cv.check_if_ins(snv) is None

tests/test_variants/test_remove_variant.py

@@ -0,0 +1,68 @@
+"""
+Regression test for fix/contig-variants-remove-variant.
+
+remove_variant was using variant.position instead of variant.position1,
+causing it to silently no-op on any variant (since base variants only
+define position1, not position).
+"""
+import numpy as np
+from Bio.Seq import Seq
+from Bio.SeqRecord import SeqRecord
+
+from neat.variants.contig_variants import ContigVariants
+from neat.variants import SingleNucleotideVariant
+
+
+_GT = np.array([0, 1])
+
+
+def _snv(pos, alt="T"):
+    return SingleNucleotideVariant(pos, alt, _GT.copy(), "42")
+
+
+def test_remove_variant_removes_existing():
+    cv = ContigVariants()
+    v = _snv(10, "T")
+    cv.add_variant(v)
+    assert 10 in cv.variant_locations
+    cv.remove_variant(v)
+    assert 10 not in cv.variant_locations
+
+
+def test_remove_variant_empties_contig_variants_list():
+    cv = ContigVariants()
+    v = _snv(5, "C")
+    cv.add_variant(v)
+    cv.remove_variant(v)
+    # The location is removed from variant_locations; the dict entry is empty
+    assert v not in cv.contig_variants.get(5, [])
+
+
+def test_remove_variant_leaves_other_locations_intact():
+    cv = ContigVariants()
+    v1 = _snv(5, "C")
+    v2 = _snv(20, "G")
+    cv.add_variant(v1)
+    cv.add_variant(v2)
+    cv.remove_variant(v1)
+    assert 20 in cv.variant_locations
+    assert 5 not in cv.variant_locations
+
+
+def test_remove_variant_with_multiple_at_same_location():
+    """Removing one of two variants at the same position leaves the other."""
+    cv = ContigVariants()
+    v1 = SingleNucleotideVariant(10, "T", np.array([1, 0]), "42")
+    v2 = SingleNucleotideVariant(10, "G", np.array([0, 1]), "42")
+    cv.add_variant(v1)
+    cv.add_variant(v2)
+    cv.remove_variant(v1)
+    assert 10 in cv.variant_locations
+    assert v2 in cv.contig_variants[10]
+
+
+def test_remove_variant_noop_when_absent():
+    """Calling remove_variant on a variant not in ContigVariants is safe."""
+    cv = ContigVariants()
+    v = _snv(99, "A")
+    cv.remove_variant(v)  # should not raise