Add error message for impossible roi request to ZeissDataReader and fix sign error when negative values passed to ROI

CHANGELOG.md

@@ -8,6 +8,7 @@
     - `PaganinProcessor` now correctly applies scaling with magnification for cone-beam geometry (#2225)
     - `cilacc` path lookup no longer broken for editable installations (#2257)
     - update `version.py` to use `importlib` & fix tagless installation #2255 (#2269)
+    - Fixed behaviour of `ZeissDataReader` when negative values are passed in the ROI (#2244)
   - Dependencies:
     - olefile and dxchange are optional dependencies, instead of required (#2209)
     - dxchange minimum version set to 0.2.1 to fix #2256 (#2268)

Wrappers/Python/cil/io/NikonDataReader.py

@@ -54,6 +54,8 @@ class NikonDataReader(object):
     Notes
     -----
     `roi` behaviour:
+        The indices provided are start inclusive, stop exclusive.
+
         Files are stacked along axis_0. axis_1 and axis_2 correspond
         to row and column dimensions, respectively.
 
@@ -67,7 +69,16 @@ class NikonDataReader(object):
 
         ``start`` and ``end`` can be specified as ``None`` which is equivalent
         to ``start = 0`` and ``end = load everything to the end``, respectively.
-        Start and end also can be negative.
+        Start and end also can be negative. i.e. {'axis_name1':(10, -10)} will 
+        crop the dimension symmetrically
+
+        The following two examples are equivalent, if the size of the horizontal dimension is 2000:
+
+        >>> reader1 = NikonDataReader(file_name, roi={'horizontal': (10, -10)})
+
+        >>> reader2 = NikonDataReader(file_name, roi={'horizontal': (10, 1990)})
+
+        For more info on negative indexing in python see: https://numpy.org/doc/stable/user/basics.indexing.html
 
     '''
 

Wrappers/Python/cil/io/ZEISSDataReader.py

@@ -16,6 +16,7 @@
 # Authors:
 # CIL Developers, listed at: https://github.com/TomographicImaging/CIL/blob/master/NOTICE.txt
 # Andrew Shartis (UES, Inc.)
+
 from cil.framework import AcquisitionData, AcquisitionGeometry, ImageData, ImageGeometry
 from cil.framework.labels import AngleUnit, AcquisitionDimension, ImageDimension
 import numpy as np
@@ -37,18 +38,41 @@ class ZEISSDataReader:
         dictionary with roi to load for each axis:
         ``{'axis_labels_1': (start, end, step),'axis_labels_2': (start, end, step)}``.
         ``axis_labels`` are defined by ImageGeometry and AcquisitionGeometry dimension labels.
+        This accepts negative indexing.
 
     Notes
     -----
     `roi` behaviour:
-        For ImageData to skip files or to change number of files to load,
-        adjust ``vertical``. E.g. ``'vertical': (100, 300)`` will skip first 100 files
-        and will load 200 files.
+        The indices provided are start inclusive, stop exclusive.
 
         ``'axis_label': -1`` is a shortcut to load all elements along axis.
 
         ``start`` and ``end`` can be specified as ``None`` which is equivalent
         to ``start = 0`` and ``end = load everything to the end``, respectively.
+
+        The ROI accepts negative indexing. i.e. {'axis_name1':(10, -10)} will 
+        crop the dimension symmetrically
+        The following two examples are equivalent, if the size of the horizontal dimension is 2000:
+
+        >>> reader1 = ZeissDataReader(file_name, roi={'horizontal': (10, -10)})
+
+        >>> reader2 = ZeissDataReader(file_name, roi={'horizontal': (10, 1990)})
+
+        For more info on negative indexing in python see: https://numpy.org/doc/stable/user/basics.indexing.html
+
+        **Acquisition Data**
+
+        The axis labels in the `roi` dict for `AcquisitionData` will be:
+        ``{'angle':(...),'vertical':(...),'horizontal':(...)}``
+
+        **Image Data**
+
+        The axis labels in the `roi` dict for `ImageData` will be:
+        ``{'angle':(...),'vertical':(...),'horizontal':(...)}``
+
+        To skip files or to change number of files to load,
+        adjust ``vertical``. E.g. ``'vertical': (100, 300)`` will skip first 100 files
+        and will load 200 files.
     '''
 
     def __init__(self, file_name=None, roi=None):
@@ -78,28 +102,12 @@ def set_up(self,
             dictionary with roi to load for each axis:
             ``{'axis_labels_1': (start, end, step),'axis_labels_2': (start, end, step)}``.
             ``axis_labels`` are defined by ImageGeometry and AcquisitionGeometry dimension labels.
-
+            This accepts negative indexing.
+
         Notes
         -----
-        `roi` behaviour:
-            ``'axis_label': -1`` is a shortcut to load all elements along axis.
-
-            ``start`` and ``end`` can be specified as ``None`` which is equivalent
-            to ``start = 0`` and ``end = load everything to the end``, respectively.
-
-            **Acquisition Data**
+        For more info on the parameters see the class docstring.
 
-            The axis labels in the `roi` dict for `AcquisitionData` will be:
-            ``{'angle':(...),'vertical':(...),'horizontal':(...)}``
-
-            **Image Data**
-
-            The axis labels in the `roi` dict for `ImageData` will be:
-            ``{'angle':(...),'vertical':(...),'horizontal':(...)}``
-
-            To skip files or to change number of files to load,
-            adjust ``vertical``. E.g. ``'vertical': (100, 300)`` will skip first 100 files
-            and will load 200 files.
         '''
 
         # check if file exists
@@ -131,17 +139,20 @@ def set_up(self,
 
             # check roi labels and create tuple for slicing
             for key in roi.keys():
+                if key not in zeiss_data_order:
+                    raise ValueError('Invalid roi key: {}. Keys should be one of {}'.format(key, list(zeiss_data_order.keys())))
                 idx = zeiss_data_order[key]
                 if roi[key] != -1:
                     for i, x in enumerate(roi[key]):
                         if x is None:
                             continue
-
                         if i != 2: #start and stop
-                            default_roi[idx][i] = x if x >= 0 else default_roi[idx][1] - x
+                            adjusted_x = x if x >= 0 else default_roi[idx][1] + x
+                            if adjusted_x < 0 or adjusted_x > default_roi[idx][1]:
+                                raise ValueError('Invalid roi value: {} for key: {}. Values should be between -{} and {}'.format(x, key, default_roi[idx][1], default_roi[idx][1]))
+                            default_roi[idx][i] = x if x >= 0 else default_roi[idx][1] + x
                         else: #step
                             default_roi[idx][i] =  x if x > 0 else 1
-
             self._roi = default_roi
             self._metadata = self.slice_metadata(metadata)
         else:

Wrappers/Python/test/test_io.py

@@ -94,6 +94,65 @@
 
 
 class TestZeissDataReader(unittest.TestCase):
+    @unittest.skipIf(not (has_file and has_olefile and has_dxchange), 
+                     f"Missing prerequisites: has_file {has_file}, has_olefile {has_olefile} has_dxchange {has_dxchange}")
+    def test_roi(self):
+        # want to pass an roi
+        reader = ZEISSDataReader(file_name=test_txrm_file)
+        metadata = reader.get_metadata()
+        horizontal_max = metadata['image_width']
+        vertical_max = metadata['image_height']
+        angles_max = len(metadata['thetas'])
+        valid_roi = {'angle': (0,1000, 1), 'vertical': (0,500), 'horizontal': (0, 400)}
+        reader.set_up(file_name=test_txrm_file, roi=valid_roi)
+        data3d = reader.read()
+
+        expected_shape=[1000,500, 400]
+        # angle, vertical, horizontal
+
+        self.assertEqual(data3d.shape, tuple(expected_shape))
+
+        valid_roi_negative_endpoint = {'angle': (0,-(angles_max-1000), 1), 'vertical': (0,-(vertical_max-500)), 'horizontal': (0, -(horizontal_max-400))}
+
+        reader.set_up(file_name=test_txrm_file, roi=valid_roi_negative_endpoint)
+        data3d = reader.read()
+        self.assertEqual(data3d.shape, tuple(expected_shape))
+
+        valid_roi_negative_startpoint = {'angle': (-1000, None, 1), 'vertical': (-500, None), 'horizontal': (-400, None)}
+        reader.set_up(file_name=test_txrm_file, roi=valid_roi_negative_startpoint)
+        data3d = reader.read()
+        self.assertEqual(data3d.shape, tuple(expected_shape))
+
+
+        invalid_rois_negative_startpoint = [{'angle': (-10000, None, 1), 'vertical': (-500, None), 'horizontal': (-400, None)},
+                                            {'angle': (-1000, None, 1), 'vertical': (-5000, None), 'horizontal': (-400, None)},
+                                            {'angle': (-1000, None, 1), 'vertical': (-500, None), 'horizontal': (-4000, None)}]
+        for invalid_roi in invalid_rois_negative_startpoint:
+            with self.assertRaises(ValueError):
+                reader.set_up(file_name=test_txrm_file, roi=invalid_roi)
+
+        invalid_rois_negative_endpoint = [{'angle': (0,-10000, 1), 'vertical': (0,-(vertical_max-500)), 'horizontal': (0, -(horizontal_max-400))},
+                                            {'angle': (0,-(angles_max-1000), 1), 'vertical': (0,-5000), 'horizontal': (0, -(horizontal_max-400))},
+                                            {'angle': (0,-(angles_max-1000), 1), 'vertical': (0,-(vertical_max-500)), 'horizontal': (0, -4000)}]
+        for invalid_roi in invalid_rois_negative_endpoint:
+            with self.assertRaises(ValueError):
+                reader.set_up(file_name=test_txrm_file, roi=invalid_roi)
+
+        invalid_rois_positive_endpoints = [{'angle': (0,10000, 1), 'vertical': (0,500), 'horizontal': (0, 400)},
+                                            {'angle': (0,1000, 1), 'vertical': (0,5000), 'horizontal': (0, 400)},
+                                            {'angle': (0,1000, 1), 'vertical': (0,500), 'horizontal': (0, 4000)}]
+        for invalid_roi in invalid_rois_positive_endpoints:
+            with self.assertRaises(ValueError):
+                reader.set_up(file_name=test_txrm_file, roi=invalid_roi)
+
+        invalid_rois_positive_startpoints = [{'angle': (10000, None, 1), 'vertical': (-500, None), 'horizontal': (-400, None)},
+                                            {'angle': (1000, None, 1), 'vertical': (5000, None), 'horizontal': (-400, None)},
+                                            {'angle': (1000, None, 1), 'vertical': (-500, None), 'horizontal': (4000, None)}]
+        for invalid_roi in invalid_rois_positive_startpoints:
+            with self.assertRaises(ValueError):
+                reader.set_up(file_name=test_txrm_file, roi=invalid_roi)
+
+
 
     @unittest.skipIf(not (has_file and has_olefile and has_dxchange), 
                      f"Missing prerequisites: has_file {has_file}, has_olefile {has_olefile} has_dxchange {has_dxchange}")