Remove sort_key parameter in favour of boolean sort option

Author: Chris Bridge

docs/legacy.rst

@@ -159,9 +159,12 @@ multiframe instance. The default behavior sorts first by the "SeriesNumber"
 attribute (for cases with multiple series), then by the "InstanceNumber"
 attribute, then by the "SOPInstanceUID" attribute (to give a predictable sort
 order in cases where instance number is not present). If you prefer an
-alternative sorting scheme, you can pass a callable to the ``sort_key``
-argument of the form accepted by the Python built-in ``sorted`` function. For
-example, to sort by ``KVP`` and then ``SliceLocation``:
+alternative sorting scheme, you can disable the default sorting by passing
+``sort=False`` and then pass the datasets in whatever order you prefer. There
+is no strict requirement that frames be sorted, but it would be best practice
+to use some sort of logical sorting scheme.
+
+For example, to sort by ``KVP`` and then ``SliceLocation``:
 
 .. code-block:: python
 
@@ -180,6 +183,48 @@ example, to sort by ``KVP`` and then ``SliceLocation``:
   # Read in the files
   ct_series = [dcmread(f) for f in legacy_ct_files]
 
+  # Sort using KVP and slice location
+  ct_series = sorted(
+      ct_series, key=lambda dcm: (dcm.KVP, dcm.SliceLocation)
+  )
+
+  # Create multiframe instance with custom sort key
+  multiframe = hd.legacy.LegacyConvertedEnhancedCTImage(
+      ct_series,
+      series_number=1,
+      instance_number=1,
+      series_instance_uid=hd.UID(),
+      sop_instance_uid=hd.UID(),
+      series_description="Enhanced Test Files",
+      sort=False,  # disable default sorting
+  )
+
+  # Save out the new multiframe conversion
+  multiframe.save_as("legacy_converted_ct.dcm")
+
+Alternatively, you could use highdicom's ``sort_datasets`` function to sort the
+datasets based purely on spatial location:
+
+.. code-block:: python
+
+  import highdicom as hd
+  from pydicom import dcmread
+  from pydicom.data import get_testdata_file
+
+
+  # Use this series of files from the pydicom test data
+  legacy_ct_files = [
+      get_testdata_file('dicomdirtests/77654033/CT2/17136'),
+      get_testdata_file('dicomdirtests/77654033/CT2/17196'),
+      get_testdata_file('dicomdirtests/77654033/CT2/17166'),
+  ]
+
+  # Read in the files
+  ct_series = [dcmread(f) for f in legacy_ct_files]
+
+  # Sort frames spatially
+  ct_series = hd.spatial.sort_datasets(ct_series)
+
   # Create multiframe instance with custom sort key
   multiframe = hd.legacy.LegacyConvertedEnhancedCTImage(
       ct_series,
@@ -188,7 +233,7 @@ example, to sort by ``KVP`` and then ``SliceLocation``:
       series_instance_uid=hd.UID(),
       sop_instance_uid=hd.UID(),
       series_description="Enhanced Test Files",
-      sort_key=lambda dcm: (dcm.KVP, dcm.SliceLocation),
+      sort=False,  # disable default sorting
   )
 
   # Save out the new multiframe conversion

src/highdicom/legacy/sop.py

@@ -198,7 +198,7 @@ def get_source_keywords(self) -> list[str]:
         return [self.dest_kw]
 
 
-def default_sort_key(x: Dataset) -> Tuple[Union[int, str, UID], ...]:
+def _default_sort_key(x: Dataset) -> Tuple[Union[int, str, UID], ...]:
     """The default sort key to sort single frames before conversion.
 
     Parameters
@@ -241,7 +241,7 @@ def __init__(
         transfer_syntax_uid: str | None = None,
         use_extended_offset_table: bool = False,
         require_volume: bool = False,
-        sort_key: Callable | None = None,
+        sort: bool = True,
         workers: int | Executor = 0,
     ) -> None:
         """
@@ -270,18 +270,24 @@ def __init__(
         require_volume: bool, optional
             Raise an error if the legacy datasets do not form a regularly-spaced
             volume.
-        sort_key: Callable | None, optional
-            A function by which the single-frame instances will be sorted to
-            determine the order of frames in the newly created instance.
+        sort: bool, optional
+            Whether to sort datasets before placing them into the legacy
+            instance. If True, datasets will be sorted using the default sort
+            key. If False, frames of the new instance will be in the same order
+            as the list of legacy instances were passed.
         workers: int | concurrent.futures.Executor, optional
             Number of worker processes (or executor object) to use for frame
             compression, if compression or transcoding is needed.
 
         """
-        if sort_key is None:
-            sort_key = default_sort_key
+        if sort:
+            self._legacy_datasets = sorted(
+                list(legacy_datasets),
+                key=_default_sort_key
+            )
+        else:
+            self._legacy_datasets = list(legacy_datasets)
 
-        self._legacy_datasets = sorted(list(legacy_datasets), key=sort_key)
         self._destination = destination
         self._transfer_syntax_uid = transfer_syntax_uid
         self._workers = workers
@@ -2079,7 +2085,7 @@ def __init__(
         transfer_syntax_uid: str | None = None,
         use_extended_offset_table: bool = False,
         require_volume: bool = False,
-        sort_key: Callable | None = None,
+        sort: bool = True,
         contributing_equipment: Sequence[ContributingEquipment] | None = None,
         workers: int | Executor = 0,
         **kwargs: Any,
@@ -2121,9 +2127,15 @@ def __init__(
             the standard, but users may wish to additionally impose this
             stricter requirement to ensure the resulting files are easier
             to work with.
-        sort_key: Callable | None, optional
-            A function by which the single-frame instances will be sorted to
-            determine the order of frames in the newly created instance.
+        sort: bool, optional
+            Sort the legacy datasets before placing them into the new instance
+            as frames. If False, frames are placed in the same order legacy
+            datasets were passed. When True, sorting is performed first by
+            Series Number, then Instance Number, then SOP Instance UID. To use
+            an alternative sort order, pre-sort the legacy datasets, and pass
+            ``sort=False``. Though there are no requirements on sort order in
+            the standard, it is generally a best practice to use some sensible
+            sort order.
         contributing_equipment: Sequence[highdicom.ContributingEquipment] | None, optional
             Additional equipment that has contributed to the acquisition,
             creation or modification of this instance.
@@ -2227,7 +2239,7 @@ def __init__(
             workers=workers,
             transfer_syntax_uid=transfer_syntax_uid,
             use_extended_offset_table=use_extended_offset_table,
-            sort_key=sort_key,
+            sort=sort,
             require_volume=require_volume,
         )
 

tests/test_legacy.py

@@ -1090,6 +1090,57 @@ def test_mixed_series(modality: Modality):
     )
 
 
+def test_unsorted():
+    # Combining two series is valid if other attributes are consistent
+    data_generator = DicomGenerator(5)
+    legacy_datasets = data_generator.generate_mixed_framesets(
+        Modality.PT, 1, True, True
+    )
+
+    # Reverse the order
+    reversed_legacy_datasets = legacy_datasets[::-1]
+
+    converted_sorted = LegacyConvertedEnhancedPETImage(
+        legacy_datasets=reversed_legacy_datasets,
+        series_instance_uid=UID(),
+        series_number=1,
+        sop_instance_uid=UID(),
+        instance_number=1,
+        sort=True,
+    )
+
+    # Frames should be sorted by instance number
+    for ds, pffg in zip(
+        legacy_datasets,
+        converted_sorted.PerFrameFunctionalGroupsSequence,
+    ):
+        frame_source_uid = (
+            pffg.ConversionSourceAttributesSequence[0]
+            .ReferencedSOPInstanceUID
+        )
+        assert frame_source_uid == ds.SOPInstanceUID
+
+    converted_unsorted = LegacyConvertedEnhancedPETImage(
+        legacy_datasets=reversed_legacy_datasets,
+        series_instance_uid=UID(),
+        series_number=1,
+        sop_instance_uid=UID(),
+        instance_number=1,
+        sort=False,
+    )
+
+    # Frames should be in same order they were passed
+    for ds, pffg in zip(
+        reversed_legacy_datasets,
+        converted_unsorted.PerFrameFunctionalGroupsSequence,
+    ):
+        frame_source_uid = (
+            pffg.ConversionSourceAttributesSequence[0]
+            .ReferencedSOPInstanceUID
+        )
+        assert frame_source_uid == ds.SOPInstanceUID
+
+
 def test_body_part_mapping(modality: Modality):
     """Test that BodyPartExamined is correctly mapped to a coded
     AnatomicRegionSequence.