Repository: cogent3/cogent3
Pull request #1545 (2023-07-15 04:06:09)

MAINT: improve robustness of Features


Patch:
diff --git a/src/cogent3/core/annotation.py b/src/cogent3/core/annotation.py
index 8ab071d74..836310cfa 100644
--- a/src/cogent3/core/annotation.py
+++ b/src/cogent3/core/annotation.py
@@ -1,27 +1,25 @@
-import copy
-import json
-
 from typing import Iterable, Optional
 
-from cogent3._version import __version__
+from numpy import array
+
 from cogent3.util import warning as c3warn
-from cogent3.util.misc import get_object_provenance
 
 from .location import Map
 
 
-# todo gah implement __repr__ and __str__ methods
 # todo gah write docstrings!
 class Feature:
     """new style annotation, created on demand"""
 
+    # we make the object immutable by making public attributes a property
     __slots__ = (
-        "parent",
-        "seqid",
-        "map",
-        "biotype",
-        "name",
+        "_parent",
+        "_seqid",
+        "_map",
+        "_biotype",
+        "_name",
         "_serialisable",
+        "_id",
     )
 
     # todo gah implement a __new__ to trap args for serialisation purposes?
@@ -30,12 +28,42 @@ class Feature:
         d = locals()
         exclude = ("self", "__class__", "kw")
         self._serialisable = {k: v for k, v in d.items() if k not in exclude}
-        self.biotype = biotype
-        self.name = name
-        self.parent = parent
-        self.seqid = seqid
+        self._parent = parent
+        self._seqid = seqid
         assert map.parent_length == len(parent), (map, len(parent))
-        self.map = map
+        self._map = map
+        self._biotype = biotype
+        self._name = name
+        data = [id(self.parent), tuple(self.map.get_coordinates())]
+        data.extend((self.seqid, self.biotype, self.name))
+        self._id = hash(tuple(data))
+
+    def __eq__(self, other):
+        return self._id == other._id
+
+    def __hash__(self):
+        """Features can be used in a dictionary!"""
+        return self._id
+
+    @property
+    def parent(self):
+        return self._parent
+
+    @property
+    def seqid(self):
+        return self._seqid
+
+    @property
+    def map(self):
+        return self._map
+
+    @property
+    def biotype(self):
+        return self._biotype
+
+    @property
+    def name(self):
+        return self._name
 
     def get_slice(self, complete: bool = False, allow_gaps: bool = False):
         """
@@ -126,25 +154,50 @@ class Feature:
 
     def get_children(self, biotype: Optional[str] = None, **kwargs):
         """generator returns sub-features of self optionally matching biotype"""
+        offset = getattr(self.parent, "annotation_offset", 0)
+        start = self.map.start + offset
+        end = self.map.end + offset
+
         make_feature = self.parent.make_feature
         db = self.parent.annotation_db
-        for child in db.get_feature_children(
+        for record in db.get_feature_children(
             biotype=biotype,
             name=self.name,
-            start=self.map.start,
-            end=self.map.end,
+            start=start,
+            end=end,
             **kwargs,
         ):
-            yield make_feature(feature=child)
+            record["spans"] = array(record["spans"]) - offset
+            feature = make_feature(feature=record)
+            # For GenBank, the names can be shared between parent and child,
+            # but we don't want to return self. The cleanest way to not
+            # return self is to just check.
+            if feature == self:
+                continue
+            yield feature
 
     def get_parent(self, **kwargs):
         """generator returns parent features of self optionally matching biotype"""
+        offset = getattr(self.parent, "annotation_offset", 0)
+        start = self.map.start + offset
+        end = self.map.end + offset
+
         make_feature = self.parent.make_feature
         db = self.parent.annotation_db
-        for child in db.get_feature_parent(
-            name=self.name, start=self.map.start, end=self.map.end, **kwargs
+        for record in db.get_feature_parent(
+            name=self.name,
+            start=start,
+            end=end,
+            **kwargs,
         ):
-            yield make_feature(feature=child)
+            record["spans"] = array(record["spans"]) - offset
+            feature = make_feature(feature=record)
+            # For GenBank, the names can be shared between parent and child,
+            # but we don't want to return self. The cleanest way to not
+            # return self is to just check.
+            if feature == self:
+                continue
+            yield feature
 
     def union(self, features: Iterable):
         """return as a single Feature
diff --git a/src/cogent3/core/sequence.py b/src/cogent3/core/sequence.py
index c0efa5fe6..cffab9fec 100644
--- a/src/cogent3/core/sequence.py
+++ b/src/cogent3/core/sequence.py
@@ -856,7 +856,7 @@ class Sequence(SequenceI):
             int: The offset between annotation coordinates and sequence coordinates.
         """
 
-        return self._seq.offset
+        return self._seq.offset + self._seq.start
 
     @annotation_offset.setter
     def annotation_offset(self, value):
@@ -1565,8 +1565,10 @@ class Sequence(SequenceI):
             passed to get_features(biotype). Can be a single biotype or
             series. Only features matching this will be included.
         """
+        # make sure the drawables are unique by adding to a set
+        features = set(self.get_features(biotype=biotype, allow_partial=True))
         result = defaultdict(list)
-        for f in self.get_features(biotype=biotype, allow_partial=True):
+        for f in features:
             result[f.biotype].append(f.get_drawable())
         return result
 
@@ -1592,17 +1594,25 @@ class Sequence(SequenceI):
         Returns
         -------
         a Drawable instance
+
+        Notes
+        -----
+        If provided, the biotype is used for plot order.
         """
         from cogent3.draw.drawable import Drawable
 
         drawables = self.get_drawables(biotype=biotype)
         if not drawables:
             return None
+
+        biotype = list(drawables) if biotype is None else biotype
+        biotypes = (biotype,) if isinstance(biotype, str) else biotype
+
         # we order by tracks
         top = 0
         space = 0.25
         annotes = []
-        for feature_type in drawables:
+        for feature_type in biotypes:
             new_bottom = top + space
             for i, annott in enumerate(drawables[feature_type]):
                 annott.shift(y=new_bottom - annott.bottom)


Test patch:
diff --git a/tests/test_core/test_annotation.py b/tests/test_core/test_annotation.py
index 5c8bff2af..890048003 100644
--- a/tests/test_core/test_annotation.py
+++ b/tests/test_core/test_annotation.py
@@ -294,8 +294,8 @@ def test_seq_slice_seqfeat_invalid(ann_aln):
 
 def test_gbdb_get_children_get_parent(DATA_DIR):
     seq = load_seq(DATA_DIR / "annotated_seq.gb")
+    seq = seq[2900:6000]
     (orig,) = list(seq.get_features(biotype="gene", name="CNA00110"))
     (child,) = list(orig.get_children("CDS"))
-    parent, *_ = list(child.get_parent(exclude_biotype=child.biotype))
-    assert parent.name == orig.name
-    assert parent.biotype != child.biotype
+    parent, *_ = list(child.get_parent())
+    assert parent == orig
diff --git a/tests/test_core/test_features.py b/tests/test_core/test_features.py
index cbe7e8895..20ae08f6c 100755
--- a/tests/test_core/test_features.py
+++ b/tests/test_core/test_features.py
@@ -3,6 +3,7 @@ from unittest import TestCase
 import pytest
 
 from cogent3 import ASCII, DNA, get_moltype, make_aligned_seqs
+from cogent3.core.annotation import Feature
 from cogent3.core.annotation_db import BasicAnnotationDb, GffAnnotationDb
 # Complete version of manipulating sequence annotations
 from cogent3.util.deserialise import deserialise_object
@@ -148,7 +149,7 @@ def test_feature_residue():
     assert rc_exons.as_one_span().get_slice().to_dict() == dict(x="C-CCC", y="-T---")
 
 
-@pytest.fixture()
+@pytest.fixture(scope="function")
 def ann_seq():
     # A Sequence with a couple of exons on it.
     s = DNA.make_seq("AAGAAGAAGACCCCCAAAAAAAAAATTTTTTTTTTAAAAAAAAAAAAA", name="Orig")
@@ -667,3 +668,40 @@ def test_rc_feature_on_wrong_moltype(moltype):
     )
     with pytest.raises(TypeError):
         cds.get_slice()
+
+
+def test_feature_equal(ann_seq):
+    (f1,) = list(ann_seq.get_features(biotype="gene"))
+    (f2,) = list(ann_seq.get_features(biotype="gene"))
+    assert f1 is not f2
+    assert f1 == f2
+
+
+def test_feature_not_equal(ann_seq):
+    (f1,) = list(ann_seq.get_features(biotype="gene"))
+    (f2,) = list(ann_seq.get_features(biotype="exon"))
+    assert f1 is not f2
+    assert f1 != f2
+    # same attributes except parent different parent seq
+    nseq = ann_seq.copy()
+    (nf1,) = list(nseq.get_features(biotype="gene"))
+    assert nf1 != f1
+
+
+@pytest.mark.parametrize("attr", ("seqid", "biotype", "name", "map"))
+def test_feature_not_equal_attr(ann_seq, attr):
+    (f1,) = list(ann_seq.get_features(biotype="gene"))
+    attrs = dict(
+        parent=f1.parent, seqid=f1.seqid, biotype=f1.biotype, map=f1.map, name=f1.name
+    )
+    value = attrs["map"][:4] if attr == "map" else "different"
+    attrs[attr] = value
+    f2 = Feature(**attrs)
+    assert f1 != f2
+
+
+def test_hash_feature(ann_seq):
+    (f1,) = list(ann_seq.get_features(biotype="gene"))
+    (f2,) = list(ann_seq.get_features(biotype="gene"))
+    got = {f1, f2}
+    assert len(got) == 1