Legacy conversion revamp #388
Conversation
| """ | ||
| self.mixed_frames = single_frame_list | ||
| self.mixed_frames_copy = self.mixed_frames[:] | ||
| self._distinguishing_attribute_keywords = [ |
There was a problem hiding this comment.
Pull request overview
This PR revamps the legacy conversion functionality for DICOM images, building on work from PR #34. It introduces a comprehensive framework for converting single-frame legacy DICOM images (CT, MR, PET) to multi-frame enhanced images, inspired by David Clunie's PixelMed implementation.
Changes:
- Complete rewrite of the legacy conversion module with new class architecture (
_FrameSet,_FrameSetCollection,_CommonLegacyConvertedEnhancedImage) - Enhanced test suite with new
DicomGeneratorhelper class for generating test DICOM datasets - Support for frame set detection, shared/per-frame attribute classification, and modular conversion through build blocks
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| tests/test_legacy.py | Adds comprehensive test infrastructure with DicomGenerator class and new test cases for conversion, frame set detection, and attribute handling |
| src/highdicom/legacy/sop.py | Complete rewrite introducing new architecture for legacy-to-enhanced DICOM conversion with frame set management and modular attribute copying |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setattr(pffg, seq_kw, [inner_item]) | ||
|
|
||
| def _add_composite_instance_contex_module(self) -> None: | ||
| """Copies/adds a `composite_instance_contex` multiframe module to |
There was a problem hiding this comment.
Typo in docstring: "composite_instance_contex" should be "composite_instance_context" (missing 't' in 'context').
| """Copies/adds a `composite_instance_contex` multiframe module to | |
| """Copies/adds a `composite_instance_context` multiframe module to |
| tmp_dataset.PhotometricInterpretation = 'MONOCHROME2' | ||
| tmp_dataset.PixelData = pixel_array | ||
| tmp_dataset.PositionReferenceIndicator = 'XY' | ||
| tmp_dataset.ProtocolName = 'some protocole' |
There was a problem hiding this comment.
Typo: 'protocole' should be 'protocol' (extra 'e').
| tmp_dataset.ProtocolName = 'some protocole' | |
| tmp_dataset.ProtocolName = 'some protocol' |
| ) | ||
| setattr(pffg, seq_kw, [inner_item]) | ||
|
|
||
| def _add_composite_instance_contex_module(self) -> None: |
There was a problem hiding this comment.
Typo in method name: "_add_composite_instance_contex_module" should be "_add_composite_instance_context_module" (missing 't' in 'context').
| self._build_blocks.extend( | ||
| [ | ||
| [self._add_image_pixel_module, None], | ||
| [self._add_composite_instance_contex_module, None], |
There was a problem hiding this comment.
Typo in method name reference: "_add_composite_instance_contex_module" should be "_add_composite_instance_context_module" (missing 't' in 'context').
| [self._add_composite_instance_contex_module, None], | |
| [self._add_composite_instance_context_module, None], |
| self.PerFrameFunctionalGroupsSequence, | ||
| self._legacy_datasets | ||
| ): | ||
| self._add_referenced_image_module_to_dataset(legacy, item) |
There was a problem hiding this comment.
Bug: The method is calling "_add_referenced_image_module_to_dataset" instead of "_add_pixel_value_transformation_module_to_dataset". This appears to be a copy-paste error. The loop should be calling the pixel value transformation method, not the referenced image method.
| self._add_referenced_image_module_to_dataset(legacy, item) | |
| self._add_pixel_value_transformation_module_to_dataset( | |
| legacy, | |
| item, | |
| ) |
| tmp_dataset.AdditionalPatientHistory = 'UTERINE CA PRE-OP EVAL' | ||
| tmp_dataset.ContentDate = date_ | ||
| tmp_dataset.ContentTime = datetime.now().time() | ||
| tmp_dataset.Manufacturer = 'Mnufacturer' |
There was a problem hiding this comment.
Typo: 'Mnufacturer' should be 'Manufacturer' (missing 'a').
| tmp_dataset.Manufacturer = 'Mnufacturer' | |
| tmp_dataset.Manufacturer = 'Manufacturer' |
| Parameters | ||
| ---------- | ||
| source: pydicom.Dataset | ||
| Source fataset from which the module's attribute values are copied |
There was a problem hiding this comment.
Typo in docstring: 'fataset' should be 'dataset' (incorrect letter 'f').
| Source fataset from which the module's attribute values are copied | |
| Source dataset from which the module's attribute values are copied |
| legacy_datasets | ||
| ) | ||
|
|
||
| super(_Image, self).__init__( |
There was a problem hiding this comment.
First argument to super() should be _CommonLegacyConvertedEnhancedImage.
| super(_Image, self).__init__( | |
| super(_CommonLegacyConvertedEnhancedImage, self).__init__( |
| def _find_per_frame_and_shared_tags(self) -> None: | ||
| """Detects and collects all shared and perframe attributes""" | ||
| rough_shared: Dict[BaseTag, List[DataElement]] = defaultdict(list) | ||
| sh_tgs = set() |
| ratio = fr_ds.LossyImageCompressionRatio | ||
| try: | ||
| sum_compression_ratio += float(ratio) | ||
| except BaseException: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except BaseException: | |
| except (TypeError, ValueError): |
Working PR to update legacy conversion, building on the work done by @afshinmessiah in #34. Supersedes #34