Merge pull request #107 from robomics/fix/stripe-class

Improve parameter validation in Stripe class

Author: rea1991

src/stripepy/utils/stripe.py

@@ -99,6 +99,8 @@ def __init__(
 
         self._left_bound = None
         self._right_bound = None
+        self._bottom_bound = None
+        self._top_bound = None
         if horizontal_bounds is not None:
             self.set_horizontal_bounds(*horizontal_bounds)
 
@@ -127,6 +129,18 @@ def _infer_location(seed: int, top_bound: int, bottom_bound: int) -> str:
 
         NotImplementedError
 
+    @staticmethod
+    def _validate_vertical_bounds(left_bound: int, right_bound: int, top_bound: int, bottom_bound: int, location: str):
+        assert location in {"upper_triangular", "lower_triangular"}
+        if location == "lower_triangular" and not (left_bound <= top_bound <= right_bound):
+            raise ValueError(
+                f"top bound is not enclosed between the left and right bounds: {left_bound=}, {right_bound=}, {top_bound=}"
+            )
+        elif location == "upper_triangular" and not (left_bound <= bottom_bound <= right_bound):
+            raise ValueError(
+                f"bottom bound is not enclosed between the left and right bounds: {left_bound=}, {right_bound=}, {bottom_bound=}"
+            )
+
     def _compute_convex_comp(self) -> int:
         cfx1 = 0.99
         cfx2 = 0.01
@@ -190,7 +204,13 @@ def _compute_rmean(self, I: ss.csr_matrix, window: int) -> float:
         return submatrix.mean()
 
     def _all_bounds_set(self) -> bool:
-        return all((x is not None for x in [self._left_bound, self._right_bound, self._bottom_bound, self._top_bound]))
+        return self._horizontal_bounds_set() and self._vertical_bounds_set()
+
+    def _horizontal_bounds_set(self) -> bool:
+        return self._left_bound is not None and self._right_bound is not None
+
+    def _vertical_bounds_set(self) -> bool:
+        return self._top_bound is not None and self._bottom_bound is not None
 
     @property
     def seed(self) -> int:
@@ -313,11 +333,17 @@ def set_horizontal_bounds(self, left_bound: int, right_bound: int):
             assert self._right_bound is not None
             raise RuntimeError("horizontal stripe bounds have already been set")
 
+        if left_bound < 0 or right_bound < 0:
+            raise ValueError("stripe bounds must be positive integers")
+
         if not left_bound <= self._seed <= right_bound:
             raise ValueError(
                 f"horizontal bounds must enclose the seed position: seed={self._seed}, {left_bound=}, {right_bound=}"
             )
 
+        if self._vertical_bounds_set():
+            Stripe._validate_vertical_bounds(left_bound, right_bound, self._top_bound, self._bottom_bound, self._where)
+
         self._left_bound = left_bound
         self._right_bound = right_bound
 
@@ -336,21 +362,29 @@ def set_vertical_bounds(self, top_bound: int, bottom_bound: int):
             assert self._top_bound is not None
             raise RuntimeError("vertical stripe bounds have already been set")
 
+        if top_bound < 0 or bottom_bound < 0:
+            raise ValueError("stripe bounds must be positive integers")
+
         if top_bound > bottom_bound:
             raise ValueError(
                 f"the lower vertical bound must be greater than the upper vertical bound: {top_bound=}, {bottom_bound=}"
             )
 
-        self._top_bound = top_bound
-        self._bottom_bound = bottom_bound
-
-        computed_where = self._infer_location(self._seed, self._top_bound, self._bottom_bound)
+        computed_where = self._infer_location(self._seed, top_bound, bottom_bound)
 
         if self._where is not None and computed_where != self._where:
             raise RuntimeError(
                 f"computed location does not match the provided stripe location: computed={computed_where}, expected={self._where}"
             )
 
+        if self._horizontal_bounds_set():
+            Stripe._validate_vertical_bounds(
+                self._left_bound, self._right_bound, top_bound, bottom_bound, computed_where
+            )
+
+        self._top_bound = top_bound
+        self._bottom_bound = bottom_bound
+
         self._where = computed_where
 
     def compute_biodescriptors(self, I: ss.csr_matrix, window: int = 3):

test/unit/test_IO.py

@@ -95,7 +95,7 @@ def test_getters(self):
     def test_stripe_getters(self):
         res = Result("chr1", 123)
 
-        stripes = [Stripe(10, 1.23)]
+        stripes = [Stripe(10, 1.23, where="upper_triangular")]
         stripes[0].set_vertical_bounds(5, 10)
         stripes[0].set_horizontal_bounds(8, 12)
         stripes[0].compute_biodescriptors(csr_matrix((15, 15), dtype=float))