Minor cleanups to rois API (#112)

* improve roi repr

* make pixel size a pydantic model

* polish roi to slicing dict api

* update change log

* implement roi union

* Roi simplification

* add union testing

* Revert "implement roi union"

This reverts commit 360275f.

* improve handling of sequence slicing

* improve um to pixel conversion

* update changelog

Author: lorenzocerrone

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## [v0.4.2]
+
+### API Changes
+
+- Make roi.to_slicing_dict(pixel_size) always require pixel_size argument for consistency with other roi methods.
+- Make PixelSize object a Pydantic model to allow for serialization.
+
+### Bug Fixes
+
+- Improve robustness when rounding Rois to pixel coordinates. 
+
 ## [v0.4.1]
 
 ### Bug Fixes

src/ngio/common/_roi.py

@@ -14,9 +14,22 @@
 from ngio.utils import NgioValueError
 
 
-def _to_raster(value: float, length: float, pixel_size: float) -> tuple[float, float]:
+def _world_to_raster(value: float, pixel_size: float, eps: float = 1e-6) -> float:
     raster_value = value / pixel_size
-    raster_length = length / pixel_size
+
+    # If the value is very close to an integer, round it
+    # This ensures that we don't have floating point precision issues
+    # When loading ROIs that were originally defined in pixel coordinates
+    _rounded = round(raster_value)
+    if abs(_rounded - raster_value) < eps:
+        return _rounded
+    return raster_value
+
+
+def _to_raster(value: float, length: float, pixel_size: float) -> tuple[float, float]:
+    """Convert to raster coordinates."""
+    raster_value = _world_to_raster(value, pixel_size)
+    raster_length = _world_to_raster(length, pixel_size)
     return raster_value, raster_length
 
 
@@ -29,7 +42,7 @@ def _to_slice(start: float | None, length: float | None) -> slice:
     return slice(start, end)
 
 
-def _to_world(value: int | float, pixel_size: float) -> float:
+def _raster_to_world(value: int | float, pixel_size: float) -> float:
     """Convert to world coordinates."""
     return value * pixel_size
 
@@ -60,16 +73,28 @@ def intersection(self, other: "GenericRoi") -> "GenericRoi | None":
 
     def _nice_str(self) -> str:
         if self.t is not None:
-            t_str = f"t={self.t}->{self.t_length}"
+            t_start = self.t
+        else:
+            t_start = None
+        if self.t_length is not None and t_start is not None:
+            t_end = t_start + self.t_length
         else:
-            t_str = "t=None"
+            t_end = None
+
+        t_str = f"t={t_start}->{t_end}"
+
         if self.z is not None:
-            z_str = f"z={self.z}->{self.z_length}"
+            z_start = self.z
+        else:
+            z_start = None
+        if self.z_length is not None and z_start is not None:
+            z_end = z_start + self.z_length
         else:
-            z_str = "z=None"
+            z_end = None
+        z_str = f"z={z_start}->{z_end}"
 
-        y_str = f"y={self.y}->{self.y_length}"
-        x_str = f"x={self.x}->{self.x_length}"
+        y_str = f"y={self.y}->{self.y + self.y_length}"
+        x_str = f"x={self.x}->{self.x + self.x_length}"
 
         if self.label is not None:
             label_str = f", label={self.label}"
@@ -90,6 +115,9 @@ def __repr__(self) -> str:
     def __str__(self) -> str:
         return self._nice_str()
 
+    def to_slicing_dict(self, pixel_size: PixelSize) -> dict[str, slice]:
+        raise NotImplementedError
+
 
 def _1d_intersection(
     a: T | None, a_length: T | None, b: T | None, b_length: T | None
@@ -251,6 +279,11 @@ def zoom(self, zoom_factor: float = 1) -> "Roi":
         """
         return zoom_roi(self, zoom_factor)
 
+    def to_slicing_dict(self, pixel_size: PixelSize) -> dict[str, slice]:
+        """Convert to a slicing dictionary."""
+        roi_pixels = self.to_roi_pixels(pixel_size)
+        return roi_pixels.to_slicing_dict(pixel_size)
+
 
 class RoiPixels(GenericRoi):
     """Region of interest (ROI) in pixel coordinates."""
@@ -261,30 +294,30 @@ class RoiPixels(GenericRoi):
 
     def to_roi(self, pixel_size: PixelSize) -> "Roi":
         """Convert to raster coordinates."""
-        x = _to_world(self.x, pixel_size.x)
-        x_length = _to_world(self.x_length, pixel_size.x)
-        y = _to_world(self.y, pixel_size.y)
-        y_length = _to_world(self.y_length, pixel_size.y)
+        x = _raster_to_world(self.x, pixel_size.x)
+        x_length = _raster_to_world(self.x_length, pixel_size.x)
+        y = _raster_to_world(self.y, pixel_size.y)
+        y_length = _raster_to_world(self.y_length, pixel_size.y)
 
         if self.z is None:
             z = None
         else:
-            z = _to_world(self.z, pixel_size.z)
+            z = _raster_to_world(self.z, pixel_size.z)
 
         if self.z_length is None:
             z_length = None
         else:
-            z_length = _to_world(self.z_length, pixel_size.z)
+            z_length = _raster_to_world(self.z_length, pixel_size.z)
 
         if self.t is None:
             t = None
         else:
-            t = _to_world(self.t, pixel_size.t)
+            t = _raster_to_world(self.t, pixel_size.t)
 
         if self.t_length is None:
             t_length = None
         else:
-            t_length = _to_world(self.t_length, pixel_size.t)
+            t_length = _raster_to_world(self.t_length, pixel_size.t)
 
         extra_dict = self.model_extra if self.model_extra else {}
         return Roi(
@@ -302,7 +335,7 @@ def to_roi(self, pixel_size: PixelSize) -> "Roi":
             **extra_dict,
         )
 
-    def to_slicing_dict(self) -> dict[str, slice]:
+    def to_slicing_dict(self, pixel_size: PixelSize) -> dict[str, slice]:
         """Convert to a slicing dictionary."""
         x_slice = _to_slice(self.x, self.x_length)
         y_slice = _to_slice(self.y, self.y_length)

src/ngio/io_pipes/_io_pipes_roi.py

@@ -13,24 +13,18 @@
 from ngio.io_pipes._ops_slices import SlicingInputType
 from ngio.io_pipes._ops_transforms import TransformProtocol
 from ngio.ome_zarr_meta.ngio_specs._pixel_size import PixelSize
-from ngio.utils import NgioValueError
 
 
 def roi_to_slicing_dict(
     *,
     roi: Roi | RoiPixels,
-    pixel_size: PixelSize | None = None,
+    pixel_size: PixelSize,
     slicing_dict: dict[str, SlicingInputType] | None = None,
 ) -> dict[str, SlicingInputType]:
     """Convert a ROI to a slicing dictionary."""
-    if isinstance(roi, Roi):
-        if pixel_size is None:
-            raise NgioValueError(
-                "pixel_size must be provided when converting a Roi to slice_kwargs."
-            )
-        roi = roi.to_roi_pixels(pixel_size=pixel_size)
-
-    roi_slicing_dict: dict[str, SlicingInputType] = roi.to_slicing_dict()  # type: ignore
+    roi_slicing_dict: dict[str, SlicingInputType] = roi.to_slicing_dict(
+        pixel_size=pixel_size
+    )  # type: ignore
     if slicing_dict is None:
         return roi_slicing_dict
 

src/ngio/io_pipes/_ops_slices.py

@@ -14,7 +14,7 @@
 from ngio.utils import NgioValueError
 
 SlicingInputType: TypeAlias = slice | Sequence[int] | int | None
-SlicingType: TypeAlias = slice | tuple[int, ...] | int
+SlicingType: TypeAlias = slice | list[int] | int
 
 ##############################################################
 #
@@ -60,7 +60,7 @@ def _slicing_tuple_boundary_check(
         elif isinstance(sl, int):
             _int_boundary_check(sl, shape=sh)
             out_slicing_tuple.append(sl)
-        elif isinstance(sl, tuple):
+        elif isinstance(sl, list):
             [_int_boundary_check(i, shape=sh) for i in sl]
             out_slicing_tuple.append(sl)
         else:
@@ -115,32 +115,31 @@ def get(self, ax_name: str, normalize: bool = False) -> SlicingType:
         return slicing_tuple[ax_index]
 
 
-def _check_tuple_in_slicing_tuple(
+def _check_list_in_slicing_tuple(
     slicing_tuple: tuple[SlicingType, ...],
-) -> tuple[None, None] | tuple[int, tuple[int, ...]]:
-    """Check if there are any tuple in the slicing tuple.
+) -> tuple[None, None] | tuple[int, list[int]]:
+    """Check if there are any lists in the slicing tuple.
 
-    The zarr python api only supports int or slices, not tuples.
-    Ngio support a single tuple in the slicing tuple to allow non-contiguous
+    Dask regions when setting data do not support non-contiguous
+    selections natively.
+    Ngio support a single list in the slicing tuple to allow non-contiguous
     selection (main use case: selecting multiple channels).
     """
-    # Find if the is any tuple in the slicing tuple
+    # Find if the is any list in the slicing tuple
     # If there is one we need to handle it differently
-    tuple_in_slice = [
-        (i, s) for i, s in enumerate(slicing_tuple) if isinstance(s, tuple)
-    ]
-    if not tuple_in_slice:
-        # No tuple in the slicing tuple
+    list_in_slice = [(i, s) for i, s in enumerate(slicing_tuple) if isinstance(s, list)]
+    if not list_in_slice:
+        # No list in the slicing tuple
         return None, None
 
-    if len(tuple_in_slice) > 1:
+    if len(list_in_slice) > 1:
         raise NotImplementedError(
             "Slicing with multiple non-contiguous tuples/lists "
             "is not supported yet in Ngio. Use directly the "
             "zarr.Array api to get the correct array slice."
         )
     # Complex case, we have exactly one tuple in the slicing tuple
-    ax, first_tuple = tuple_in_slice[0]
+    ax, first_tuple = list_in_slice[0]
     if len(first_tuple) > 100:
         warn(
             "Performance warning: "
@@ -164,38 +163,14 @@ def get_slice_as_numpy(zarr_array: zarr.Array, slicing_ops: SlicingOps) -> np.nd
     slicing_tuple = slicing_ops.normalized_slicing_tuple
     # Find if the is any tuple in the slicing tuple
     # If there is one we need to handle it differently
-    ax, first_tuple = _check_tuple_in_slicing_tuple(slicing_tuple)
-    if ax is None:
-        # Simple case, no tuple in the slicing tuple
-        return zarr_array[slicing_tuple]
-
-    assert first_tuple is not None
-    slices = [
-        zarr_array[(*slicing_tuple[:ax], idx, *slicing_tuple[ax + 1 :])]
-        for idx in first_tuple
-    ]
-    out_array = np.stack(slices, axis=ax)
-    return out_array
+    return zarr_array[slicing_tuple]
 
 
 def get_slice_as_dask(zarr_array: zarr.Array, slicing_ops: SlicingOps) -> da.Array:
     """Get a slice of a zarr array as a dask array."""
     da_array = da.from_zarr(zarr_array)
     slicing_tuple = slicing_ops.normalized_slicing_tuple
-    # Find if the is any tuple in the slicing tuple
-    # If there is one we need to handle it differently
-    ax, first_tuple = _check_tuple_in_slicing_tuple(slicing_tuple)
-    if ax is None:
-        # Base case, no tuple in the slicing tuple
-        return da_array[slicing_tuple]
-
-    assert first_tuple is not None
-    slices = [
-        da_array[(*slicing_tuple[:ax], idx, *slicing_tuple[ax + 1 :])]
-        for idx in first_tuple
-    ]
-    out_array = da.stack(slices, axis=ax)
-    return out_array
+    return da_array[slicing_tuple]
 
 
 def set_slice_as_numpy(
@@ -204,17 +179,7 @@ def set_slice_as_numpy(
     slicing_ops: SlicingOps,
 ) -> None:
     slice_tuple = slicing_ops.normalized_slicing_tuple
-    ax, first_tuple = _check_tuple_in_slicing_tuple(slice_tuple)
-    if ax is None:
-        # Base case, no tuple in the slicing tuple
-        zarr_array[slice_tuple] = patch
-        return
-
-    # Complex case, we have exactly one tuple in the slicing tuple
-    assert first_tuple is not None
-    for i, idx in enumerate(first_tuple):
-        _sub_slice = (*slice_tuple[:ax], idx, *slice_tuple[ax + 1 :])
-        zarr_array[_sub_slice] = np.take(patch, indices=i, axis=ax)
+    zarr_array[slice_tuple] = patch
 
 
 def handle_int_set_as_dask(
@@ -237,7 +202,7 @@ def set_slice_as_dask(
     zarr_array: zarr.Array, patch: da.Array, slicing_ops: SlicingOps
 ) -> None:
     slice_tuple = slicing_ops.normalized_slicing_tuple
-    ax, first_tuple = _check_tuple_in_slicing_tuple(slice_tuple)
+    ax, first_tuple = _check_list_in_slicing_tuple(slice_tuple)
     patch, slice_tuple = handle_int_set_as_dask(patch, slice_tuple)
     if ax is None:
         # Base case, no tuple in the slicing tuple
@@ -261,13 +226,13 @@ def set_slice_as_dask(
 ##############################################################
 
 
-def _try_to_slice(value: Sequence[int]) -> slice | tuple[int, ...]:
+def _try_to_slice(value: Sequence[int]) -> slice | list[int]:
     """Try to convert a list of integers into a slice if they are contiguous.
 
     - If the input is empty, return an empty tuple.
     - If the input is sorted, and contains contiguous integers,
       return a slice from the minimum to the maximum integer.
-    - Otherwise, return the input as a tuple.
+    - Otherwise, return the input as a list of integers.
 
     This is useful for optimizing array slicing operations
     by allowing the use of slices when possible, which can be more efficient.
@@ -293,7 +258,7 @@ def _try_to_slice(value: Sequence[int]) -> slice | tuple[int, ...]:
     if sorted(value) == list(range(min_input, max_input + 1)):
         return slice(min_input, max_input + 1)
 
-    return tuple(value)
+    return list(value)
 
 
 def _remove_channel_slicing(
@@ -393,7 +358,7 @@ def _normalize_slicing_tuple(
     The output types are:
     - slice
     - int
-    - tuple of int (for non-contiguous selection)
+    - list of int (for non-contiguous selection)
     """
     axis_name = axis.name
     if axis_name not in slicing_dict:
@@ -408,7 +373,7 @@ def _normalize_slicing_tuple(
     elif isinstance(value, Sequence):
         # If a contiguous sequence of integers is provided,
         # convert it to a slice for simplicity.
-        # Alternatively, it will be converted to a tuple of ints
+        # Alternatively, it will be converted to a list of ints
         return _try_to_slice(value)
 
     raise NgioValueError(

src/ngio/io_pipes/_ops_slices_utils.py

@@ -22,15 +22,15 @@ def _pairs_stream(iterable: Iterable[T]) -> Iterator[tuple[T, T]]:
         seen.append(a)
 
 
-SlicingType: TypeAlias = slice | tuple[int, ...] | int
+SlicingType: TypeAlias = slice | list[int] | int
 
 
 def check_elem_intersection(s1: SlicingType, s2: SlicingType) -> bool:
     """Compare if two SlicingType elements intersect.
 
     If they are a slice, check if they overlap.
     If they are integers, check if they are equal.
-    If they are tuples, check if they have any common elements.
+    If they are lists, check if they have any common elements.
     """
     if not isinstance(s1, type(s2)):
         raise NgioValueError(
@@ -56,7 +56,7 @@ def check_elem_intersection(s1: SlicingType, s2: SlicingType) -> bool:
     elif isinstance(s1, int) and isinstance(s2, int):
         # Handle integer indices
         return s1 == s2
-    elif isinstance(s1, tuple) and isinstance(s2, tuple):
+    elif isinstance(s1, list) and isinstance(s2, list):
         if set(s1) & set(s2):
             return True
         return False
@@ -130,7 +130,7 @@ def _chunk_indices_for_axis(sel: SlicingType, size: int, csize: int) -> list[int
             raise IndexError(f"index {sel} out of bounds for axis of size {size}")
         return [sel // csize]
 
-    if isinstance(sel, tuple):
+    if isinstance(sel, list):
         if not sel:
             return []
         chunks_hit = {}