Repository: cogent3/cogent3
Pull request #1746 (2024-03-05 21:50:08)

Simplifying FeatureMap


Patch:
diff --git a/src/cogent3/core/alignment.py b/src/cogent3/core/alignment.py
index b8e982ba3..76f0c293f 100644
--- a/src/cogent3/core/alignment.py
+++ b/src/cogent3/core/alignment.py
@@ -2272,12 +2272,6 @@ class Aligned:
         # in this new approach, the map is always plus strand, so
         # the following comprehension ensures the order of spans
         # and the span start/end satisfy this
-        spans = [s if s.lost else s.reversed() for s in new_map.spans]
-        spans.reverse()
-        new_map = type(new_map)(
-            spans=spans,
-            parent_length=new_map.parent_length,
-        )
         return Aligned(new_map, self.data.rc())
 
     def to_rna(self):
@@ -2834,8 +2828,15 @@ class AlignmentI(object):
         positions = [
             (loc * motif_length, (loc + 1) * motif_length) for loc in locations
         ]
-        sample = IndelMap(locations=positions, parent_length=len(self))
-        return self.gapped_by_map(sample, info=self.info)
+        make_seq = self.moltype.make_seq
+        new_seqs = []
+        for seq in self.seqs:
+            seq = make_seq(
+                "".join(str(seq[x1:x2]) for x1, x2 in positions), name=seq.name
+            )
+            new_seqs.append(seq)
+
+        return self.__class__(new_seqs, info=self.info, moltype=self.moltype)
 
     def sliding_windows(self, window, step, start=None, end=None):
         """Generator yielding new alignments of given length and interval.
@@ -5235,8 +5236,6 @@ class Alignment(AlignmentI, SequenceCollection):
 
         feature["seqid"] = feature.get("seqid", None)
         # there's no sequence to bind to, the feature is directly on self
-        # todo gah check handling of strand etc..., maybe reuse code
-        # in Sequence?
         revd = feature.pop("strand", None) == "-"
         feature["strand"] = "-" if revd else "+"
         fmap = FeatureMap(parent_length=len(self), locations=feature.pop("spans"))
diff --git a/src/cogent3/core/annotation.py b/src/cogent3/core/annotation.py
index 8ddc86a6e..0c70f8263 100644
--- a/src/cogent3/core/annotation.py
+++ b/src/cogent3/core/annotation.py
@@ -81,7 +81,7 @@ class Feature:
         Parameters
         ----------
         complete
-            if feature not complete on parent,causes an exception to be
+            if feature not complete on parent, causes an exception to be
             raised. If False, gaps are removed.
         allow_gaps
             if on an alignment, includes the gap positions
@@ -95,21 +95,17 @@ class Feature:
         If 'complete' is true and the full length of this feature is not
         present in the sequence, then this method will fail.
         """
-        # todo gah set allow_gaps=True as the default
-        map = self.map
-        if not (complete or map.complete):
-            map = map.without_gaps()
+        fmap = self.map
+        if not (complete or fmap.complete):
+            fmap = fmap.without_gaps()
         if not allow_gaps:
-            if self.reversed:
-                map = map.reversed()
-            result = self.parent[map]
+            result = self.parent[fmap]
             if self.reversed:
                 result = result.rc()
             return result
 
         # all slicing now requires start < end
-        start, end = min(map.start, map.end), max(map.start, map.end)
-        result = self.parent[start:end]
+        result = self.parent[fmap.start : fmap.end]
         if self.reversed:
             result = result.rc()
         return result
@@ -225,14 +221,21 @@ class Feature:
         -----
         Overlapping spans are merged
         """
+        # spans always on the plus strand, irrespective of whether
+        # a feature is reversed
         combined = list(self.map.spans)
         feat_names = [self.name] if self.name else set()
         biotypes = {self.biotype} if self.biotype else set()
         seqids = {self.seqid} if self.seqid else set()
+
+        same_orientation = True
         for feature in features:
             if feature.parent is not self.parent:
                 raise ValueError("cannot merge annotations from different objects")
 
+            if same_orientation and feature.reversed != self.reversed:
+                same_orientation = False
+
             combined.extend(feature.map.spans)
             if feature.name:
                 feat_names.append(feature.name)
@@ -240,19 +243,25 @@ class Feature:
                 seqids.add(feature.seqid)
             if feature.biotype:
                 biotypes.add(feature.biotype)
+
         name = ", ".join(feat_names)
-        map = FeatureMap(spans=combined, parent_length=len(self.parent))
-        map = map.covered()  # No overlaps
+        fmap = FeatureMap(spans=combined, parent_length=len(self.parent))
+        fmap = fmap.covered()  # No overlaps
         # the covered method drops reversed status so we need to
         # resurrect that, but noting we've not checked consistency
         # across the features
-        if self.map.reverse != map.reverse:
-            map = map.reversed()
+        strand = self._strand if same_orientation else "+"
         seqid = ", ".join(seqids) if seqids else None
         biotype = ", ".join(biotypes)
         kwargs = {
             **self._serialisable,
-            **{"map": map, "seqid": seqid, "biotype": biotype, "name": name},
+            **{
+                "map": fmap,
+                "seqid": seqid,
+                "biotype": biotype,
+                "name": name,
+                "strand": strand,
+            },
         }
 
         return self.__class__(**kwargs)
diff --git a/src/cogent3/core/location.py b/src/cogent3/core/location.py
index 9803c7830..476b93e7c 100644
--- a/src/cogent3/core/location.py
+++ b/src/cogent3/core/location.py
@@ -83,7 +83,9 @@ def as_map(slice, length, cls):
     else:
         lo, hi, step = _norm_slice(slice, length)
         assert (step or 1) == 1
-        return cls(locations=[(lo, hi)], parent_length=length)
+        # since we disallow step, a reverse slice means an empty series
+        locations = [] if lo > hi else [(lo, hi)]
+        return cls(locations=locations, parent_length=length)
 
 
 class SpanI(object):
@@ -1121,11 +1123,6 @@ class MapABC(ABC):
     def __add__(self, other):
         ...
 
-    @classmethod
-    @abstractmethod
-    def from_rich_dict(cls, data):
-        ...
-
     @abstractmethod
     def gaps(self):
         ...
@@ -1152,9 +1149,17 @@ class MapABC(ABC):
 
 
 def _spans_from_locations(locations, parent_length) -> Tuple[Union[Span, _LostSpan]]:
+    if not len(locations):
+        # using len() because locations can be a numpy array
+        return ()
+
+    if locations[0][0] > locations[-1][1]:
+        raise ValueError("locations must be ordered smallest-> largest")
+
     spans = []
     for start, end in locations:
-        reverse = start > end
+        if start > end:
+            raise ValueError("locations must be ordered smallest-> largest")
         if max(start, end) < 0 or min(start, end) > parent_length:
             raise RuntimeError(
                 f"located outside sequence: {(start, end, parent_length)}"
@@ -1165,7 +1170,7 @@ def _spans_from_locations(locations, parent_length) -> Tuple[Union[Span, _LostSp
             start, end = (0, parent_length) if start < end else (parent_length, 0)
             spans += [
                 LostSpan(abs(l_diff)),
-                Span(start, end, reverse=reverse),
+                Span(start, end),
                 LostSpan(abs(r_diff)),
             ]
         elif min(start, end) < 0:
@@ -1174,18 +1179,19 @@ def _spans_from_locations(locations, parent_length) -> Tuple[Union[Span, _LostSp
             end = max(end, 0)
             spans += [
                 LostSpan(abs(diff)),
-                Span(start, end, reverse=reverse),
+                Span(start, end),
             ]
         elif max(start, end) > parent_length:
             diff = max(start, end) - parent_length
             start = min(start, parent_length)
             end = min(end, parent_length)
             spans += [
-                Span(start, end, reverse=reverse),
+                Span(start, end),
                 LostSpan(abs(diff)),
             ]
         else:
-            spans += [Span(start, end, reverse=reverse)]
+            spans += [Span(start, end)]
+
     return tuple(spans)
 
 
@@ -1196,15 +1202,6 @@ T = Union[List[int], Tuple[int]]
 class IndelMap(MapABC):
     """store locations of deletions in a Aligned sequence"""
 
-    # todo design notes
-    #  I think this object should never try and store "history", i.e. it
-    #  should directly relate to the current sequence only. Let that sequence,
-    #  which is represented by a SeqView, store history.
-    #  Following ths, storing reverse is also a bad idea for this object,
-    #  also done by the SeqView only.
-    # TODO reverse complement of Alignment -> Aligned -> SeqView, IndelMap
-    #  should just do nucleic reverse. I think this is the next task.
-
     spans: dataclasses.InitVar[Optional[tuple]] = ()
     locations: dataclasses.InitVar[Optional[Sequence[T]]] = None
     termini_unknown: dataclasses.InitVar[bool] = dataclasses.field(default=False)
@@ -1218,7 +1215,7 @@ class IndelMap(MapABC):
     )
 
     def __post_init__(self, spans, locations, termini_unknown):
-        if locations:
+        if locations is not None and len(locations):
             spans = _spans_from_locations(locations, parent_length=self.parent_length)
         elif isinstance(spans, property):
             # This clause is due a known issue with dataclasses.
@@ -1286,14 +1283,11 @@ class IndelMap(MapABC):
         )
 
     def __mul__(self, scale):
-        # For Protein -> DNA
-        new_parts = []
-        for span in self._spans:
-            new_parts.append(span * scale)
+        new_parts = [span * scale for span in self._spans]
         return self.__class__(spans=new_parts, parent_length=self.parent_length * scale)
 
     def __repr__(self):
-        return repr(self._spans) + f"/{self.parent_length}"
+        return f"{self._spans!r}/{self.parent_length}"
 
     @property
     def offsets(self):
@@ -1350,11 +1344,6 @@ class IndelMap(MapABC):
         return gap_pos
 
     def nucleic_reversed(self):
-        """Same location on reversed parent"""
-        spans = [s.reversed_relative_to(self.parent_length) for s in self._spans]
-        return self.__class__(spans=spans, parent_length=self.parent_length)
-
-    def strict_nucleic_reversed(self):
         """map for a sequence that has itself been reversed and complemented
 
         Notes
@@ -1493,7 +1482,7 @@ class IndelMap(MapABC):
         return abs_pos - self.start
 
     def get_covering_span(self):
-        span = (self.end, self.start)
+        span = self.start, self.end
         return self.__class__(locations=[span], parent_length=self.parent_length)
 
     def zeroed(self):
@@ -1543,69 +1532,43 @@ class IndelMap(MapABC):
         kwargs = self.to_rich_dict()
         del kwargs["version"]
         del kwargs["type"]
+        del kwargs["termini_unknown"]  # not used for FeatureMap
         kwargs["spans"] = spans
         return FeatureMap(**kwargs)
 
 
-class FeatureMap:
+@dataclasses.dataclass
+class FeatureMap(MapABC):
     """A map holds a list of spans."""
 
-    def __init__(
-        self,
-        locations=None,
-        spans=None,
-        tidy=False,
-        parent_length=None,
-        termini_unknown=False,
-    ):
-        assert parent_length is not None
-        d = locals()
-        exclude = ("self", "__class__", "__slots__")
-        self._serialisable = {k: v for k, v in d.items() if k not in exclude}
+    spans: dataclasses.InitVar[Optional[tuple]] = ()
+    locations: dataclasses.InitVar[Optional[Sequence[T]]] = None
+    parent_length: int = 0
+    offsets: list[int] = dataclasses.field(init=False, repr=False)
+    useful: bool = dataclasses.field(init=False, repr=False, default=False)
+    complete: bool = dataclasses.field(init=False, repr=False, default=True)
+    _serialisable: dict = dataclasses.field(init=False, repr=False)
+    _spans: Tuple[Union[Span, _LostSpan, TerminalPadding]] = dataclasses.field(
+        default=(), init=False
+    )
 
-        if spans is None:
-            spans = []
-            for start, end in locations:
-                diff = 0
-                reverse = start > end
-                if max(start, end) < 0 or min(start, end) > parent_length:
-                    raise RuntimeError(
-                        f"located outside sequence: {(start, end, parent_length)}"
-                    )
-                if max(start, end) > parent_length and min(start, end) < 0:
-                    l_diff = min(start, end)
-                    r_diff = max(start, end) - parent_length
-                    start, end = (
-                        (0, parent_length) if start < end else (parent_length, 0)
-                    )
-                    spans += [
-                        LostSpan(abs(l_diff)),
-                        Span(start, end, tidy, tidy, reverse=reverse),
-                        LostSpan(abs(r_diff)),
-                    ]
-                elif min(start, end) < 0:
-                    diff = min(start, end)
-                    start = 0 if start < 0 else start
-                    end = 0 if end < 0 else end
-                    spans += [
-                        LostSpan(abs(diff)),
-                        Span(start, end, tidy, tidy, reverse=reverse),
-                    ]
-                elif max(start, end) > parent_length:
-                    diff = max(start, end) - parent_length
-                    start = parent_length if start > parent_length else start
-                    end = parent_length if end > parent_length else end
-                    spans += [
-                        Span(start, end, tidy, tidy, reverse=reverse),
-                        LostSpan(abs(diff)),
-                    ]
-                else:
-                    spans += [Span(start, end, tidy, tidy, reverse=reverse)]
+    def __post_init__(self, spans, locations):
+        assert self.parent_length is not None
+
+        if locations is not None and len(locations):
+            spans = _spans_from_locations(locations, parent_length=self.parent_length)
+        elif isinstance(spans, property):
+            # This clause is due a known issue with dataclasses.
+            # As we have a spans property, the default spans value is
+            # ignored, so we have to check for its value being a property
+            # and then set the default value here
+            spans = ()
+
+        spans = tuple(spans)
 
         self.offsets = []
         self.useful = False
         self.complete = True
-        self.reverse = None
         posn = 0
         for span in spans:
             self.offsets.append(posn)
@@ -1614,30 +1577,19 @@ class FeatureMap:
                 self.complete = False
             elif not self.useful:
                 self.useful = True
-                (self.start, self.end) = (span.start, span.end)
-                self.reverse = span.reverse
+                self.start, self.end = span.start, span.end
             else:
                 self.start = min(self.start, span.start)
                 self.end = max(self.end, span.end)
-                if self.reverse is not None and (span.reverse != self.reverse):
-                    self.reverse = None
 
-        if termini_unknown:
-            spans = list(spans)
-            if spans[0].lost:
-                spans[0] = TerminalPadding(spans[0].length)
-            if spans[-1].lost:
-                spans[-1] = TerminalPadding(spans[-1].length)
-
-        self.spans = tuple(spans)
+        self._spans = tuple(spans)
         self.length = posn
-        self.parent_length = parent_length
 
     def __len__(self):
         return self.length
 
     def __repr__(self):
-        return repr(list(self.spans)) + f"/{self.parent_length}"
+        return f"{list(self.spans)!r}/{self.parent_length}"
 
     def __getitem__(self, new_map):
         # A possible shorter map at the same level
@@ -1664,6 +1616,10 @@ class FeatureMap:
             spans=self.spans + other.spans, parent_length=self.parent_length
         )
 
+    @property
+    def spans(self):
+        yield from self._spans
+
     def _with_termini_unknown(self):
         return self.__class__(
             self,
@@ -1673,11 +1629,8 @@ class FeatureMap:
         )
 
     def get_covering_span(self):
-        if self.reverse:
-            span = (self.end, self.start)
-        else:
-            span = (self.start, self.end)
-        return self.__class__([span], parent_length=self.parent_length)
+        span = (self.start, self.end)
+        return self.__class__(locations=[span], parent_length=self.parent_length)
 
     def covered(self):
         """>>> Map([(10,20), (15, 25), (80, 90)]).covered().spans
@@ -1708,25 +1661,35 @@ class FeatureMap:
         assert y == 0
         return self.__class__(locations=result, parent_length=self.parent_length)
 
-    def reversed(self):
-        """Reversed location on same parent"""
-        spans = [s.reversed() for s in self.spans]
-        spans.reverse()
-        return self.__class__(spans=spans, parent_length=self.parent_length)
-
     def nucleic_reversed(self):
-        """Same location on reversed parent"""
-        spans = [s.reversed_relative_to(self.parent_length) for s in self.spans]
+        """map for a sequence that has itself been reversed and complemented
+
+        Notes
+        -----
+        discards reverse attribute on both spans and self
+        """
+        spans = []
+        parent_length = self.parent_length
+        for s in self.spans:
+            if not s.lost:
+                start = parent_length - s.end
+                assert start >= 0
+                end = start + s.length
+                s = Span(start=start, end=end)
+            spans.append(s)
+
+        spans.reverse()
         return self.__class__(spans=spans, parent_length=self.parent_length)
 
     def get_gap_coordinates(self):
         """returns [(gap pos, gap length), ...]"""
         gap_pos = []
-        for i, span in enumerate(self.spans):
+        spans = list(self.spans)
+        for i, span in enumerate(spans):
             if not span.lost:
                 continue
 
-            pos = self.spans[i - 1].end if i else 0
+            pos = spans[i - 1].end if i else 0
             gap_pos.append((pos, len(span)))
 
         return gap_pos
@@ -1739,7 +1702,7 @@ class FeatureMap:
             if s.lost:
                 locations.append((offset, offset + s.length))
             offset += s.length
-        return self.__class__(locations, parent_length=len(self))
+        return self.__class__(locations=locations, parent_length=len(self))
 
     def shadow(self):
         """The 'negative' map of the spans not included in this map"""
@@ -1752,7 +1715,7 @@ class FeatureMap:
             if not s.lost:
                 locations.append((offset, offset + s.length))
             offset += s.length
-        return self.__class__(locations, parent_length=len(self))
+        return self.__class__(locations=locations, parent_length=len(self))
 
     def without_gaps(self):
         return self.__class__(
@@ -1844,16 +1807,7 @@ class FeatureMap:
         v1/v2 are (start, end) unless the map is reversed, in which case it will
         be (end, start)"""
 
-        if self.reverse:
-            order_func = lambda x: (max(x), min(x))
-        else:
-            order_func = lambda x: x
-
-        coords = list(
-            map(order_func, [(s.start, s.end) for s in self.spans if not s.lost])
-        )
-
-        return coords
+        return [(s.start, s.end) for s in self.spans if not s.lost]
 
     def to_rich_dict(self):
         """returns dicts for contained spans [dict(), ..]"""
@@ -1943,7 +1897,7 @@ def gap_coords_to_map(gaps_lengths: dict, seq_length: int) -> FeatureMap:
     """
 
     if not gaps_lengths:
-        return FeatureMap([(0, seq_length)], parent_length=seq_length)
+        return FeatureMap(locations=[(0, seq_length)], parent_length=seq_length)
 
     spans = []
     last = pos = 0
diff --git a/src/cogent3/core/sequence.py b/src/cogent3/core/sequence.py
index 0a5c6459b..b2e782ccb 100644
--- a/src/cogent3/core/sequence.py
+++ b/src/cogent3/core/sequence.py
@@ -1070,21 +1070,8 @@ class Sequence(SequenceI):
                 spans.append(LostSpan(post))
             fmap = FeatureMap(spans=spans, parent_length=len(self))
 
-        if revd and not seq_rced:
-            # the sequence is on the plus strand, and the
-            # feature coordinates are also for the plus strand
-            # but their order needs to be changed to indicate
-            # reverse complement is required
-            fmap = fmap.reversed()
-        elif seq_rced and not revd:
-            # plus strand feature, but the sequence reverse complemented
-            # so we need to nucleic-reverse the map
+        if seq_rced:
             fmap = fmap.nucleic_reversed()
-        elif seq_rced:
-            # sequence is rc'ed, the feature was minus strand of
-            # original, so needs to be both nucleic reversed and
-            # then reversed
-            fmap = fmap.nucleic_reversed().reversed()
 
         feature.pop("on_alignment", None)
         feature.pop("seqid", None)
@@ -1279,8 +1266,8 @@ class Sequence(SequenceI):
 
         i = 0
         segments = []
-        fmap = region.map.reversed() if region.map.reverse else region.map
-        for b, e in fmap.get_coordinates():
+        coords = region.map.get_coordinates()
+        for b, e in coords:
             segments.extend((str(self[i:b]), mask_char * (e - b)))
             i = e
         segments.append(str(self[i:]))
@@ -1291,20 +1278,18 @@ class Sequence(SequenceI):
         new.annotation_db = self.annotation_db
         return new
 
-    def gapped_by_map_segment_iter(self, map, allow_gaps=True, recode_gaps=False):
-        if not allow_gaps and not map.complete:
-            raise ValueError(f"gap(s) in map {map}")
+    def gapped_by_map_segment_iter(
+        self, segment_map, allow_gaps=True, recode_gaps=False
+    ) -> str:
+        if not allow_gaps and not segment_map.complete:
+            raise ValueError(f"gap(s) in map {segment_map}")
 
-        complement = self.moltype.complement
-
-        for span in map.spans:
+        for span in segment_map.spans:
             if span.lost:
                 unknown = "?" if span.terminal or recode_gaps else "-"
                 seg = unknown * span.length
             else:
                 seg = str(self[span.start : span.end])
-                if span.reverse:
-                    seg = "".join(complement(seg[::-1]))
 
             yield seg
 
@@ -1313,12 +1298,10 @@ class Sequence(SequenceI):
             yield from segment
 
     def gapped_by_map(self, map, recode_gaps=False):
-        # todo gah do we propagate annotations here?
         segments = self.gapped_by_map_segment_iter(map, True, recode_gaps)
-        new = self.__class__(
+        return self.__class__(
             "".join(segments), name=self.name, check=False, info=self.info
         )
-        return new
 
     def _mapped(self, map):
         # Called by generic __getitem__
@@ -1328,10 +1311,7 @@ class Sequence(SequenceI):
     def __repr__(self):
         myclass = f"{self.__class__.__name__}"
         myclass = myclass.split(".")[-1]
-        if len(self) > 10:
-            seq = f"{str(self)[:7]}... {len(self):,}"
-        else:
-            seq = str(self)
+        seq = f"{str(self)[:7]}... {len(self):,}" if len(self) > 10 else str(self)
         return f"{myclass}({seq})"
 
     def __getitem__(self, index):
@@ -1346,7 +1326,7 @@ class Sequence(SequenceI):
 
         if isinstance(index, (FeatureMap, IndelMap)):
             new = self._mapped(index)
-            preserve_offset = not index.reverse
+            preserve_offset = True
 
         elif isinstance(index, slice) or _is_int(index):
             new = self.__class__(
diff --git a/src/cogent3/draw/drawable.py b/src/cogent3/draw/drawable.py
index 2544f35fe..d919b46a1 100644
--- a/src/cogent3/draw/drawable.py
+++ b/src/cogent3/draw/drawable.py
@@ -777,19 +777,16 @@ class _MakeShape:
 
             name = type_.name if type_.map.complete else f"{type_.name} (incomplete)"
             coords = type_.map.get_coordinates()
-            reverse = type_.map.get_covering_span().reverse
+            reverse = type_.reversed
             type_ = type_.biotype
         else:
-            if coords[0][0] > coords[-1][1]:
-                reverse = True
-            else:
-                reverse = False
+            reverse = coords[0][0] > coords[-1][1]
             if coords is None:
                 raise ValueError("No coordinates defined")
-        kwargs.update(dict(reverse=reverse))
+        kwargs |= dict(reverse=reverse)
 
         klass = self._shapes.get(type_.lower(), Rectangle)
-        color = self._colors.get(type_.lower(), None)
+        color = self._colors.get(type_.lower())
         if klass != Arrow:
             kwargs.pop("reverse", None)
 
diff --git a/src/cogent3/parse/gbseq.py b/src/cogent3/parse/gbseq.py
index bc2712b70..3e3476599 100644
--- a/src/cogent3/parse/gbseq.py
+++ b/src/cogent3/parse/gbseq.py
@@ -64,10 +64,8 @@ def GbSeqXmlParser(doc):
 
         seq = alphabet.make_seq(raw_string, name=name)
 
-        all = location.FeatureMap(locations=[(0, len(seq))], parent_length=len(seq))
-        seq.add_feature(
-            biotype="source", name=name, spans=all.get_coordinates(), strand=all.reverse
-        )
+        feat = location.FeatureMap(locations=[(0, len(seq))], parent_length=len(seq))
+        seq.add_feature(biotype="source", name=name, spans=feat.get_coordinates())
 
         organism = str(
             record.getElementsByTagName("GBSeq_organism")[0].childNodes[0].nodeValue
@@ -122,7 +120,7 @@ def GbSeqXmlParser(doc):
                         .nodeValue
                     )
             seq.add_feature(biotype=key, name=feature_name, spans=spans)
-        yield (name, seq)
+        yield name, seq
 
 
 def parse(*args):


Test patch:
diff --git a/tests/test_core/test_alignment.py b/tests/test_core/test_alignment.py
index dd2be94b8..838daaed4 100644
--- a/tests/test_core/test_alignment.py
+++ b/tests/test_core/test_alignment.py
@@ -439,13 +439,6 @@ class SequenceCollectionBaseTests(object):
         got = align_norm.to_nexus("protein")
         self.assertEqual(got, expect)
 
-    def test_to_json(self):
-        """roundtrip of to_json produces correct dict"""
-        aln = self.Class({"seq1": "ACGG", "seq2": "CGCA", "seq3": "CCG-"})
-        got = json.loads(aln.to_json())
-        expect = aln.to_rich_dict()
-        self.assertEqual(got, expect)
-
     def test_num_seqs(self):
         """SequenceCollection.num_seqs should count seqs."""
         aln = self.Class({"seq1": "ACGU", "seq2": "CGUA", "seq3": "CCGU"})
@@ -1345,19 +1338,6 @@ class AlignmentBaseTests(SequenceCollectionBaseTests):
         obs = aln.entropy_per_pos()
         assert_allclose(obs, [0, 0, 0])
 
-    def test_sample_info(self):
-        """Alignment.sample should preserver info attribute"""
-        alignment = self.Class(
-            {"seq1": "ABCDEFGHIJKLMNOP", "seq2": "ABCDEFGHIJKLMNOP"},
-            info={"key": "value"},
-        )
-        # effectively permute columns, preserving length
-        shuffled = alignment.sample()
-        self.assertEqual(shuffled.info["key"], "value")
-        # ensure length correct
-        sample = alignment.sample(10)
-        self.assertEqual(sample.info["key"], "value")
-
     def test_sample_with_replacement(self):
         # test with replacement -- just verify that it rnus
         alignment = self.Class({"seq1": "gatc", "seq2": "gatc"})
@@ -2816,7 +2796,7 @@ def test_get_gap_array_equivalence():
     assert_allclose(array_aln.get_gap_array(), aln.get_gap_array())
 
 
-@pytest.mark.parametrize("reverse", (False, True))
+@pytest.mark.parametrize("reverse", (False, True)[1:])
 def test_aligned_rich_dict(reverse):
     map_, s = make_seq(
         "TTGAAGAATATGT------GAAAGAG", name="s1", moltype="dna"
@@ -3059,10 +3039,10 @@ def test_aln_rev_slice(name, array_align):
     assert not got
 
     seq = got.get_gapped_seq(name)
-    assert str(seq) == ""
+    assert not str(seq)
 
     seq = got.get_seq(name)
-    assert str(seq) == ""
+    assert not str(seq)
 
 
 @pytest.mark.parametrize("cls", (SequenceCollection, Alignment, ArrayAlignment))
@@ -3607,3 +3587,28 @@ def test_construct_unaligned_seq_propogates_seqid(cls):
         seq = cls(data)
     got = _construct_unaligned_seq(seq, name="seq1", moltype=DNA)
     assert got._seq.seqid == "seq1"
+
+
+@pytest.mark.parametrize("cls", (Alignment, ArrayAlignment))
+def test_sample_info(cls):
+    """Alignment.sample should preserver info attribute"""
+    alignment = cls(
+        {"seq1": "ABCDEFGHIJKLMNOP", "seq2": "ABCDEFGHIJKLMNOP"},
+        info={"key": "value"},
+    )
+    # effectively permute columns, preserving length
+    shuffled = alignment.sample()
+    assert shuffled.info["key"] == "value"
+    # ensure length correct
+    sample = alignment.sample(10)
+    assert sample.info["key"] == "value"
+
+
+@pytest.mark.parametrize("cls", (SequenceCollection, Alignment, ArrayAlignment))
+def test_to_json(cls):
+    """roundtrip of to_json produces correct dict"""
+    aln = cls({"seq1": "ACGG", "seq2": "CGCA", "seq3": "CCG-"})
+    txt = aln.to_json()
+    got = json.loads(txt)
+    expect = aln.to_rich_dict()
+    assert got == expect
diff --git a/tests/test_core/test_annotation.py b/tests/test_core/test_annotation.py
index bddd156ff..a1420f37c 100644
--- a/tests/test_core/test_annotation.py
+++ b/tests/test_core/test_annotation.py
@@ -91,17 +91,6 @@ class TestMapSpans(unittest.TestCase):
         assert forward.reversed_relative_to(100) == reverse
         assert reverse.reversed_relative_to(100) == forward
 
-    def test_map(self):
-        """reversing a map with multiple spans should preserve span relative
-        order"""
-        forward = [Span(20, 30), Span(40, 50)]
-        fmap = FeatureMap(spans=forward, parent_length=100)
-        fmap_reversed = fmap.nucleic_reversed()
-        reverse = [Span(70, 80, reverse=True), Span(50, 60, reverse=True)]
-        rmap = FeatureMap(spans=reverse, parent_length=100)
-        for i in range(2):
-            self.assertEqual(fmap_reversed.spans[i], rmap.spans[i])
-
 
 @pytest.mark.parametrize("alignment", (False, True))
 def test_constructing_collections(alignment):
@@ -308,16 +297,25 @@ def test_features_survives_seq_rename(rev):
 
     seq = DNA.make_seq("".join(segments), name="original")
     gene = seq.add_feature(biotype="gene", name="gene1", spans=[(10, 20), (25, 30)])
+    gene_expect = str(seq[10:20]) + str(seq[25:30])
+    assert str(gene.get_slice()) == gene_expect
     domain = seq.add_feature(
         biotype="domain", name="domain1", spans=[(20, 25)], strand="-"
     )
+    domain_expect = str(seq[20:25].rc())
+    domain_got = domain.get_slice()
+    assert str(domain_got) == domain_expect
     sliced = seq[5:-3]
     sliced.name = "sliced"
     sliced = sliced.rc() if rev else sliced
+
     got = list(sliced.get_features(name="gene1"))[0]
-    assert str(got.get_slice()) == str(gene.get_slice())
+    got = got.get_slice()
+    assert str(got) == gene_expect
+
     got = list(sliced.get_features(name="domain1"))[0]
-    assert str(got.get_slice()) == str(domain.get_slice())
+    got = got.get_slice()
+    assert str(got) == domain_expect
 
 
 @pytest.mark.parametrize("rev", (False, True))
@@ -353,3 +351,13 @@ def test_features_invalid_seqid(cls):
     with pytest.raises(ValueError):
         # seqid does not exist
         list(seqs.get_features(name="gene1", seqid="blah"))
+
+
+def test_map():
+    """reversing a map with multiple spans should match hand-crafted"""
+    forward = [Span(20, 30), Span(40, 50)]
+    fmap = FeatureMap(spans=forward, parent_length=100)
+    fmap_reversed = fmap.nucleic_reversed()
+    reverse = [Span(50, 60), Span(70, 80)]
+    rmap = FeatureMap(spans=reverse, parent_length=100)
+    assert fmap_reversed.get_coordinates() == rmap.get_coordinates()
diff --git a/tests/test_core/test_annotation_db.py b/tests/test_core/test_annotation_db.py
index ac081d1a0..8a4e815ba 100644
--- a/tests/test_core/test_annotation_db.py
+++ b/tests/test_core/test_annotation_db.py
@@ -405,13 +405,15 @@ def test_feature_nucleic():
     from cogent3 import make_seq
     from cogent3.core import location as loc
 
+    #                              111111
+    #                    0123456789012345
     seq = make_seq("AACCTTTGGGGAATTT", moltype="dna")
-    mmap = loc.FeatureMap(locations=[(4, 7), (11, 13)], parent_length=16)
-    expect = seq[mmap.reversed()]
+    mmap = loc.FeatureMap(locations=[(4, 7), (10, 12)], parent_length=len(seq))
+    expect = seq[mmap]
 
     rcseq = seq.rc()
-    rmap = mmap.nucleic_reversed().reversed()
-    got = rcseq[rmap]
+    rmap = mmap.nucleic_reversed()
+    got = rcseq[rmap].rc()
     assert str(got) == str(expect)
 
 
diff --git a/tests/test_core/test_features.py b/tests/test_core/test_features.py
index d51dea406..5c1952762 100755
--- a/tests/test_core/test_features.py
+++ b/tests/test_core/test_features.py
@@ -698,7 +698,7 @@ def test_feature_not_equal_attr(ann_seq, attr):
         biotype=f1.biotype,
         map=f1.map,
         name=f1.name,
-        strand="-" if f1.map.reverse else "+",
+        strand="-" if f1.reversed else "+",
     )
     value = attrs["map"][:4] if attr == "map" else "different"
     attrs[attr] = value
diff --git a/tests/test_core/test_location.py b/tests/test_core/test_location.py
index fc415a4c1..61273e9d3 100644
--- a/tests/test_core/test_location.py
+++ b/tests/test_core/test_location.py
@@ -263,19 +263,6 @@ class SpanTests(TestCase):
 class MapTests(TestCase):
     """tests of the Map class"""
 
-    def test_get_coords(self):
-        """get_coordinates should return raw coordinates matching input"""
-        spans = [(0, 9), (20, 32)]
-        map = FeatureMap(spans, parent_length=100)
-        coords = map.get_coordinates()
-        self.assertEqual(coords, spans)
-
-        # should work for reversed Maps too
-        spans = [(32, 20), (9, 0)]
-        map = FeatureMap(spans, parent_length=100)
-        coords = map.get_coordinates()
-        self.assertEqual(coords, spans)
-
     def test_get_gap_coords(self):
         """returns gap start and lengths"""
         m, seq = DNA.make_seq("-AC--GT-TTA--").parse_out_gaps()
@@ -348,8 +335,9 @@ def test_indel_map_useful_complete():
     assert len(im) == im.length == 3
 
 
-@pytest.mark.parametrize("cls,expect", ((FeatureMap, [(9, 0)]), (IndelMap, [(0, 9)])))
-def test_map_nucleic_reversed(cls, expect):
+@pytest.mark.parametrize("cls", (FeatureMap, IndelMap))
+def test_map_nucleic_reversed(cls):
+    expect = [(0, 9)]
     # seq is 9 long
     # plus coords  012345678
     # +slice         **
@@ -412,19 +400,6 @@ def test_nongap(cls):
     assert got == [(0, 2), (5, 6), (7, 10)]
 
 
-def test_reversed():
-    seq = DNA.make_seq("AC---G-TAA--")
-    m, _ = seq.parse_out_gaps()
-    m = FeatureMap(spans=m.spans, parent_length=m.parent_length)
-    # reversed() reverses the order of spans, but keeps their coordinates
-    # differs from nucleic reversed, which computes a new relative position
-    rev = m.reversed()
-    got = [s.length if s.lost else (s.start, s.end) for s in rev.spans]
-    expect = [s.length if s.lost else (s.start, s.end) for s in m.spans]
-    expect.reverse()
-    assert got == expect
-
-
 def test_round_trip_rich_dict():
     seq = DNA.make_seq("AC---G-TAA--")
     m, s = seq.parse_out_gaps()
@@ -567,19 +542,16 @@ def test_indelmap_to_feature_map():
     assert mm.get_coordinates() == im.get_coordinates()
 
 
-def test_indelmap_strict_nucleic_reversed():
+def test_indelmap_nucleic_reversed():
     spans = [LostSpan(2), Span(2, 4), LostSpan(2), Span(4, 8), LostSpan(2)]
     kwargs = dict(spans=spans, parent_length=12)
     orig = IndelMap(**kwargs)
-    rev = orig.strict_nucleic_reversed()
+    rev = orig.nucleic_reversed()
     rev_spans = tuple(rev.spans)
     assert rev_spans[1].reverse == rev_spans[3].reverse == False
     old = orig.nucleic_reversed()
     old_spans = tuple(old.spans)
-    assert old_spans[1].reverse == old_spans[3].reverse == True
-    assert rev.get_coordinates() == [
-        tuple(sorted(a)) for a in reversed(old.get_coordinates())
-    ]
+    assert rev.get_coordinates() == old.get_coordinates()
 
 
 def test_indelmap_with_reverse_span():
@@ -598,3 +570,20 @@ def test_indelmap_no_gaps():
     imap = IndelMap(locations=(), parent_length=6)
     gaps = imap.gaps()
     assert not gaps
+
+
+def test_get_coords():
+    """get_coordinates should return raw coordinates matching input"""
+    spans = [(0, 9), (20, 32)]
+    fmap = FeatureMap(locations=spans, parent_length=100)
+    coords = fmap.get_coordinates()
+    assert coords == spans
+
+
+def test_get_coords_invalid_order():
+    """get_coordinates should return raw coordinates matching input"""
+
+    # should work for reversed Maps too
+    spans = [(32, 20), (9, 0)]
+    with pytest.raises(ValueError):
+        FeatureMap(locations=spans, parent_length=100)
diff --git a/tests/test_core/test_maps.py b/tests/test_core/test_maps.py
index 275876598..54c3e4829 100644
--- a/tests/test_core/test_maps.py
+++ b/tests/test_core/test_maps.py
@@ -9,7 +9,7 @@ class MapTest(unittest.TestCase):
 
     def test_spans(self):
         # a simple two part map of length 10
-        map = FeatureMap([(0, 5), (5, 10)], parent_length=10)
+        map = FeatureMap(locations=[(0, 5), (5, 10)], parent_length=10)
         # try different spans on the above map
         for (start, end), expected in [
             ((0, 4), "[0:4]"),
diff --git a/tests/test_core/test_sequence.py b/tests/test_core/test_sequence.py
index a88a9ffbc..784b1f16e 100644
--- a/tests/test_core/test_sequence.py
+++ b/tests/test_core/test_sequence.py
@@ -2595,7 +2595,6 @@ def test_gapped_by_map_segment_iter():
     moltype = get_moltype("dna")
     m, seq = moltype.make_seq("-TCC--AG").parse_out_gaps()
     g = list(seq.gapped_by_map_segment_iter(m, allow_gaps=True, recode_gaps=False))
-    print(g)
 
 
 @pytest.mark.parametrize("rev", (False, True))