From eccc2ea4a347c182ee9a545f747fe4113508dfec Mon Sep 17 00:00:00 2001 From: David Waterman Date: Thu, 4 May 2023 16:59:39 +0100 Subject: [PATCH 1/6] What happens if we remove the batch offset stuff from imageset slicing? --- src/dxtbx/imageset.py | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/dxtbx/imageset.py b/src/dxtbx/imageset.py index 574779956..85fd2200b 100644 --- a/src/dxtbx/imageset.py +++ b/src/dxtbx/imageset.py @@ -300,29 +300,12 @@ def __getitem__(self, item): if not isinstance(item, slice): return self.get_corrected_data(item) else: - offset = self.get_scan().get_batch_offset() if item.step is not None: raise IndexError("Sequences must be sequential") - # nasty workaround for https://github.com/dials/dials/issues/1153 - # slices with -1 in them are meaningful :-/ so grab the original - # constructor arguments of the slice object. - # item.start and item.stop may have been compromised at this point. - if offset < 0: - start, stop, step = item.__reduce__()[1] - if start is None: - start = 0 - else: - start -= offset - if stop is None: - stop = len(self) - else: - stop -= offset - else: - start = item.start or 0 - stop = item.stop or (len(self) + offset) - start -= offset - stop -= offset + start = item.start or 0 + stop = item.stop or len(self) + if self.data().has_single_file_reader(): reader = self.reader().copy(self.reader().paths(), stop - start) else: From 8ad4f0ee103f897fc967bd52b6a4f177d673ef50 Mon Sep 17 00:00:00 2001 From: David Waterman Date: Fri, 5 May 2023 09:34:00 +0100 Subject: [PATCH 2/6] The workaround in #485 for negative offset was reacting to the erroneous behaviour introduced by including batch offsets in imagesequence slicing. The ImageSequence class provides access to images using a 0-based array index, no matter what the array_range is set to! --- tests/test_imageset.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_imageset.py b/tests/test_imageset.py index 9285db95d..4f3d2ab93 100644 --- a/tests/test_imageset.py +++ b/tests/test_imageset.py @@ -396,11 +396,12 @@ def tst_get_item(self, sequence): with pytest.raises(IndexError): _ = sequence[3:7:2] + # Batch offset should not affect slicing of imagesequence # Simulate a scan starting from image 0 sequence_ = copy.deepcopy(sequence) sequence_.get_scan().set_batch_offset(-1) sequence3 = sequence_[3:7] - assert sequence3.get_array_range() == (4, 8) + assert sequence3.get_array_range() == (3, 7) @staticmethod def tst_paths(sequence, filenames1): From 78e63ee549a9249ef39d0e18d83e5ec54ffebba6 Mon Sep 17 00:00:00 2001 From: David Waterman Date: Fri, 5 May 2023 12:42:35 +0100 Subject: [PATCH 3/6] Add a test that shows that data access is as expected after slicing --- tests/test_imageset.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_imageset.py b/tests/test_imageset.py index 4f3d2ab93..7c54d65b7 100644 --- a/tests/test_imageset.py +++ b/tests/test_imageset.py @@ -387,6 +387,11 @@ def tst_get_item(self, sequence): with pytest.raises(RuntimeError): _ = sequence2[5] + # Check data access matches expected images from the slice + panel_data1 = sequence[3][0] + panel_data2 = sequence2[0][0] + assert panel_data1.all_eq(panel_data2) + assert len(sequence2) == 4 assert_can_get_detectorbase(sequence2, range(0, 4), 5) self.tst_get_models(sequence2, range(0, 4), 5) From d9efffcde9f4939d5f1126337894621ea7ad5013 Mon Sep 17 00:00:00 2001 From: David Waterman Date: Fri, 5 May 2023 18:44:17 +0100 Subject: [PATCH 4/6] We do not slice using the "array_range"! --- src/dxtbx/model/experiment_list.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/dxtbx/model/experiment_list.py b/src/dxtbx/model/experiment_list.py index 9770129ed..e943818ee 100644 --- a/src/dxtbx/model/experiment_list.py +++ b/src/dxtbx/model/experiment_list.py @@ -627,8 +627,7 @@ def from_sequence_and_crystal(imageset, crystal, load_models=True): # if imagesequence is still images, make one experiment for each # all referencing into the same image set if imageset.get_scan().is_still(): - start, end = imageset.get_scan().get_array_range() - for j in range(start, end): + for j in range(len(imageset)): subset = imageset[j : j + 1] experiments.append( Experiment( From 6efa805b0df041018603df170cd5b491b9110fe0 Mon Sep 17 00:00:00 2001 From: David Waterman Date: Fri, 5 May 2023 18:56:18 +0100 Subject: [PATCH 5/6] News --- newsfragments/633.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/633.bugfix diff --git a/newsfragments/633.bugfix b/newsfragments/633.bugfix new file mode 100644 index 000000000..9d82ab792 --- /dev/null +++ b/newsfragments/633.bugfix @@ -0,0 +1 @@ +Slicing of imageset objects is fixed so that the slice indices are 0-based, as is the data accessor. From 5b8afafa8269221a56e2692dd17717fa779a9277 Mon Sep 17 00:00:00 2001 From: Nicholas Devenish Date: Wed, 26 Jul 2023 11:37:00 +0100 Subject: [PATCH 6/6] Update 633.bugfix --- newsfragments/633.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/633.bugfix b/newsfragments/633.bugfix index 9d82ab792..7ce5110e5 100644 --- a/newsfragments/633.bugfix +++ b/newsfragments/633.bugfix @@ -1 +1 @@ -Slicing of imageset objects is fixed so that the slice indices are 0-based, as is the data accessor. +Slicing of imageset objects is now consistently 0-based, including for the sliced data accessor. Previously, the data accessor had to be accessed with the original index offsets.