From 69169ff5a0e144c445f29677bed0b18fd6d8d0d4 Mon Sep 17 00:00:00 2001 From: David Waterman Date: Wed, 26 Jul 2023 16:58:34 +0100 Subject: [PATCH] Make Imageset slices consistently index from 0 Without this, slices sometimes needed to be indexed by their original image index, even inside the slice. The previous workaround in 67aab2 (#485) for negative offset was reacting to the erroneous behaviour introduced by including batch offsets in imagesequence slicing. --- newsfragments/633.bugfix | 1 + src/dxtbx/imageset.py | 23 +++-------------------- src/dxtbx/model/experiment_list.py | 3 +-- tests/test_imageset.py | 8 +++++++- 4 files changed, 12 insertions(+), 23 deletions(-) create mode 100644 newsfragments/633.bugfix diff --git a/newsfragments/633.bugfix b/newsfragments/633.bugfix new file mode 100644 index 000000000..7ce5110e5 --- /dev/null +++ b/newsfragments/633.bugfix @@ -0,0 +1 @@ +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. 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: diff --git a/src/dxtbx/model/experiment_list.py b/src/dxtbx/model/experiment_list.py index d415e1ccd..ec836614f 100644 --- a/src/dxtbx/model/experiment_list.py +++ b/src/dxtbx/model/experiment_list.py @@ -625,8 +625,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( diff --git a/tests/test_imageset.py b/tests/test_imageset.py index 9285db95d..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) @@ -396,11 +401,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):