Repository: cogent3/cogent3
Pull request #2111 (2024-11-14 23:28:11)

API: add SliceRecordABC.plus_start and plus_stop
[NEW] these return start and stop in plus strand coordinates,
     irrespective of whether reversed or not. This is most critical
     in SeqDataView where previous code was passing just parent_start
     and parent_stop. The latter use the offset, which should be
     employed only for feature querying. Adding comments to that effect
     to the docstrings.

## Summary by Sourcery

Add plus_start and plus_stop properties to SliceRecordABC to provide consistent plus strand coordinates, refactor SeqDataView to utilize these properties, and update documentation and tests accordingly.

New Features:
- Introduce plus_start and plus_stop properties in SliceRecordABC to return start and stop in plus strand coordinates, regardless of strand orientation.

Enhancements:
- Refactor SeqDataView to use plus_start and plus_stop for sequence retrieval, ensuring consistent handling of strand orientation.

Documentation:
- Add notes to docstrings clarifying the intended use of parent_start and parent_stop properties, emphasizing they should not be used for slicing on the parent object.

Tests:
- Add tests for the new plus_start, plus_stop, and plus_step properties to ensure they return correct values for both forward and reverse strand orientations.


Patch:
diff --git a/src/cogent3/core/new_alignment.py b/src/cogent3/core/new_alignment.py
index 6fda78d94..51b292164 100644
--- a/src/cogent3/core/new_alignment.py
+++ b/src/cogent3/core/new_alignment.py
@@ -180,6 +180,11 @@ class SeqDataView(new_sequence.SeqView):
     @property
     def str_value(self) -> str:
         """returns the sequence as a string"""
+        return self.alphabet.from_indices(self.array_value)
+
+    @property
+    def array_value(self) -> numpy.ndarray:
+        """returns the sequence as a numpy array"""
         # todo: kath, in ADV, the .get_seq_str method gets passed the step and
         # the returned sequence is sliced by the step. In SDV, the step is not
         # applied in the get_seq_str method, but is applied in this method here.
@@ -187,32 +192,17 @@ class SeqDataView(new_sequence.SeqView):
 
         # also, keep using parent_start and parent_stop or to use start and stop?
         # this is another inconsistency between SDV and ADV
-        raw = self.parent.get_seq_str(
-            seqid=self.seqid,
-            start=self.slice_record.parent_start,
-            stop=self.slice_record.parent_stop,
-        )
-        return raw if self.slice_record.step == 1 else raw[:: self.slice_record.step]
-
-    @property
-    def array_value(self) -> numpy.ndarray:
-        """returns the sequence as a numpy array"""
         raw = self.parent.get_seq_array(
             seqid=self.seqid,
-            start=self.slice_record.parent_start,
-            stop=self.slice_record.parent_stop,
+            start=self.slice_record.plus_start,
+            stop=self.slice_record.plus_stop,
         )
         return raw if self.slice_record.step == 1 else raw[:: self.slice_record.step]
 
     @property
     def bytes_value(self) -> bytes:
         """returns the sequence as bytes"""
-        raw = self.parent.get_seq_bytes(
-            seqid=self.seqid,
-            start=self.slice_record.parent_start,
-            stop=self.slice_record.parent_stop,
-        )
-        return raw if self.slice_record.step == 1 else raw[:: self.slice_record.step]
+        return self.str_value.encode("utf8")
 
     def __repr__(self) -> str:
         seq = f"{self[:10]!s}...{self[-5:]}" if len(self) > 15 else str(self)
@@ -2344,7 +2334,7 @@ def _(data: dict, moltype: new_moltype.MolType, seq_namer: _SeqNamer) -> CT:
     for name, seq in data.items():
         name = seq_namer(seq=seq, name=name)
         seq_data = coerce_to_raw_seq_data(seq, moltype, name=name)
-        offsets[name] = seq_data.offset
+        offsets[seq_data.parent_name or name] = seq_data.offset
         seqs[seq_data.parent_name or seq_data.name] = seq_data.seq
         rvd += 1 if seq_data.is_reversed else 0
         name_map[name] = seq_data.parent_name or name
@@ -2449,6 +2439,8 @@ def make_unaligned_seqs(
     # corrects for any naming differences in data and skip this step
     assign_names = _SeqNamer(name_func=label_to_name)
     seqs_data, offs, rvd, nm = prep_for_seqs_data(data, moltype, assign_names)
+    offset = offset or {}
+    offset = {**offs, **offset}
     # rvd is the count of sequences that have been reversed. It should be equal 0
     # or the number of sequences. If inbetween, the sequences are a mix of the two
     # which for now we do not support. However, if the user has provided a value
diff --git a/src/cogent3/core/new_sequence.py b/src/cogent3/core/new_sequence.py
index 8a2c62db4..0e035bf87 100644
--- a/src/cogent3/core/new_sequence.py
+++ b/src/cogent3/core/new_sequence.py
@@ -2107,6 +2107,33 @@ class SliceRecordABC(ABC):
         value = value or 0
         self._offset = int(value)
 
+    @property
+    def plus_start(self) -> int:
+        """start on plus strand"""
+        if self.is_reversed:
+            # self.stop becomes the start, self.stop will be negative
+            assert self.stop < 0, "expected stop on reverse strand SeqView < 0"
+            start = self.stop + self.parent_len + 1
+        else:
+            start = self.start
+        return start
+
+    @property
+    def plus_stop(self) -> int:
+        """stop on plus strand"""
+        if self.is_reversed:
+            # self.start becomes the stop, self.start will be negative
+            assert self.start < 0, "expected start on reverse strand SeqView < 0"
+            stop = self.start + self.parent_len + 1
+        else:
+            stop = self.stop
+        return stop
+
+    @property
+    def plus_step(self) -> int:
+        """step on plus strand"""
+        return abs(self.step)
+
     @property
     def parent_start(self) -> int:
         """returns the start on the parent plus strand
@@ -2115,15 +2142,12 @@ class SliceRecordABC(ABC):
         -------
         offset + start, taking into account whether reversed. Result
         is positive.
-        """
-        if self.is_reversed:
-            # self.stop becomes the start, self.stop will be negative
-            assert self.stop < 0, "expected stop on reverse strand SeqView < 0"
-            start = self.stop + self.parent_len + 1
-        else:
-            start = self.start
 
-        return self.offset + start
+        Notes
+        -----
+        This should NOT be used for slicing on the parent object.
+        """
+        return self.offset + self.plus_start
 
     @property
     def is_reversed(self):
@@ -2137,14 +2161,12 @@ class SliceRecordABC(ABC):
         -------
         offset + stop, taking into account whether reversed. Result
         is positive.
+
+        Notes
+        -----
+        This should NOT be used for slicing on the parent object.
         """
-        if self.is_reversed:
-            # self.start becomes the stop, self.start will be negative
-            assert self.start < 0, "expected start on reverse strand SeqView < 0"
-            stop = self.start + self.parent_len + 1
-        else:
-            stop = self.stop
-        return self.offset + stop
+        return self.offset + self.plus_stop
 
     def absolute_position(self, rel_index: int, include_boundary: bool = False):
         """Converts an index relative to the current view to be with respect


Test patch:
diff --git a/tests/test_core/test_new_sequence.py b/tests/test_core/test_new_sequence.py
index d254a8f15..13dd8a56c 100644
--- a/tests/test_core/test_new_sequence.py
+++ b/tests/test_core/test_new_sequence.py
@@ -2584,6 +2584,17 @@ def test_seqview_seq_len_init(start, stop, step, length, dna_alphabet):
     assert sv._parent_len == expect
 
 
+@pytest.mark.parametrize("step", (1, 3))
+@pytest.mark.parametrize("rev", (False, True))
+def test_seqview_plus_attrs(rev, step):
+    # always equal values from plkus strand
+    start = 2
+    stop = 12
+    sr = new_sequence.SliceRecord(start=start, stop=stop, step=step, parent_len=30)
+    sr = sr[::-1] if rev else sr
+    assert (sr.plus_start, sr.plus_stop, sr.plus_step) == (start, stop, step)
+
+
 def test_seqview_copy_propagates_seq_len(dna_alphabet):
     seq = "ACGGTGGGAC"
     sv = new_sequence.SeqView(parent=seq, alphabet=dna_alphabet)