Repository: antismash/antismash

Issue:
record.get_cds_features_within_location() with CompoundLocations ignores overlap argument
Retrieving cdses by location doesn't behave as expected when the location of the cds is a CompoundLocation (for example for a fungal cds)
E.g. this test (which is a copy of the existing test `TestCDSFetchByLocation()`, but the cds locations are CompoundLocations instead of FeatureLocations) fails:
```
class TestFungalCDSFetchByLocation(unittest.TestCase):
    def setUp(self):
        self.record = Record(Seq("A"*140))
        self.record.add_cds_feature(DummyCDS(strand=1, location=CompoundLocation([
            FeatureLocation(start=10, end=20, strand=1),
            FeatureLocation(start=30, end=40, strand=1)
        ])))
        self.record.add_cds_feature(DummyCDS(strand=-1, location=CompoundLocation([
            FeatureLocation(start=110, end=120, strand=-1),
            FeatureLocation(start=130, end=140, strand=-1)
        ])))
        self.func = self.record.get_cds_features_within_location

    def test_no_hits(self):
        assert not self.func(FeatureLocation(50, 90))

    def test_multiple_hits(self):
        assert self.func(FeatureLocation(10, 140)) == list(self.record.get_cds_features())

    def test_self_hits_same_strand(self):
        for cds in self.record.get_cds_features():
            assert self.func(cds.location, with_overlapping=False) == [cds]
            assert self.func(cds.location, with_overlapping=True) == [cds]

    def test_self_hits_opposite_strand(self):
        for cds in self.record.get_cds_features():
            loc = FeatureLocation(cds.location.start, cds.location.end, cds.location.strand * -1)
            assert self.func(loc, with_overlapping=False) == [cds]
            assert self.func(loc, with_overlapping=True) == [cds]

    def test_self_hits_no_strand(self):
        for cds in self.record.get_cds_features():
            loc = FeatureLocation(cds.location.start, cds.location.end)
            assert self.func(loc, with_overlapping=False) == [cds]
            assert self.func(loc, with_overlapping=True) == [cds]

    def test_shared_starts(self):
        starter = self.record.get_cds_features()[0]
        longer = DummyCDS(10, 80)
        shorter = DummyCDS(10, 20)
        self.record.add_cds_feature(longer)
        self.record.add_cds_feature(shorter)
        assert self.func(starter.location, with_overlapping=False) == [shorter, starter]
        assert self.func(starter.location, with_overlapping=True) == list(self.record.get_cds_features()[:3])

    def test_inception_cds(self):
        inner = self.record.get_cds_features()[1]
        outer = DummyCDS(90, 200, strand=1)
        self.record.add_cds_feature(outer)
        loc = FeatureLocation(110, 140)
        assert self.func(loc, with_overlapping=False) == [inner]
```
The cause is that `record.get_cds_features_within_location()` assumes that `record._cds_features` is sorted by start position of the cds location, which is true for FeatureLocations but not for CompoundLocations.
When adding a cds feature with CompoundLocation to the record, `bisect.bisect_left()` returns index 0, so (as far as I can tell) it is always just added towards the left of the list.
Don't know if this is something that should simply be fixed in `record.get_cds_features_within_location()`, or if the issue has more far fetching consequences.

Discussion:
@SJShaw: ```python
        self.record.add_cds_feature(DummyCDS(strand=-1, location=CompoundLocation([
            FeatureLocation(start=110, end=120, strand=-1),
            FeatureLocation(start=130, end=140, strand=-1)
        ])))
```

That's a cross-origin/invalid location. Cross-origin locations are always sorted to the beginning (arbitrarily chosen over the other option, always sorting to the end).@lisavader: Hi Simon, thanks for clearing that up.
I completely misunderstood how CompoundLocations work when they're on the reverse strand, so my bad on writing a wrong test!

However, seems the issue that made me look into this function in the first place, is that when the given location is a CompoundLocation and with_overlapping=True, overlapping CDSs aren't actually returned.
This works:
```
    location = FeatureLocation(start=10, end=20, strand=1)
    record = Record(Seq("A"*140))
    record.add_cds_feature(DummyCDS(9, 20))
    assert record.get_cds_features_within_location(location, with_overlapping=True) == list(record.get_cds_features())
```
This fails:
```
    location = CompoundLocation([
        FeatureLocation(start=10, end=20, strand=1),
        FeatureLocation(start=30, end=40, strand=1)
        ])
    record = Record(Seq("A"*140))
    record.add_cds_feature(DummyCDS(9, 20))
    assert record.get_cds_features_within_location(location, with_overlapping=True) == list(record.get_cds_features())
```@SJShaw: The problem is this bit, in which filtering takes places for compound inputs as if `with_overlapping` isn't set.

https://github.com/antismash/antismash/blob/5a796231478093c50ebcbf138c0ef4ff590d4432/antismash/common/secmet/record.py#L525-L531

Solution patch:
diff --git a/antismash/common/secmet/record.py b/antismash/common/secmet/record.py
index 62df3bcd..9be84363 100644
--- a/antismash/common/secmet/record.py
+++ b/antismash/common/secmet/record.py
@@ -528,7 +528,9 @@ class Record:
             for part in location.parts:
                 found = self.get_cds_features_within_location(part, with_overlapping=True)
                 features.extend(f for f in found if f not in features)
-            return [f for f in features if f.is_contained_by(location)]
+            if not with_overlapping:
+                return [f for f in features if f.is_contained_by(location)]
+            return features
 
         def find_start_in_list(location: Location, features: List[CDSFeature],
                                include_overlaps: bool) -> int:


Test patch:
diff --git a/antismash/common/secmet/test/test_secmet.py b/antismash/common/secmet/test/test_secmet.py
index 6289c9e8..9f08abdc 100644
--- a/antismash/common/secmet/test/test_secmet.py
+++ b/antismash/common/secmet/test/test_secmet.py
@@ -517,6 +517,18 @@ class TestCDSFetchByLocation(unittest.TestCase):
         loc = FeatureLocation(110, 140)
         assert self.func(loc, with_overlapping=False) == [inner]
 
+    def test_compound_with_overlapping(self):
+        location = CompoundLocation([
+            FeatureLocation(20, 40, 1),
+            FeatureLocation(60, 80, 1),
+        ])
+        for cds, overlap_expected in zip(self.record.get_cds_features(), [True, False]):
+            assert not cds.is_contained_by(location)
+            assert cds.overlaps_with(location) == overlap_expected
+
+        assert self.func(location, with_overlapping=False) == []
+        assert self.func(location, with_overlapping=True) == [self.record.get_cds_features()[0]]
+
 
 class TestClusterManipulation(unittest.TestCase):
     def setUp(self):