Update volume settings for segmentation and MSOT Acuity Echo #349
Conversation
…t the new settings
…ions # Conflicts: # simpa/utils/tags.py
…ions # Conflicts: # simpa/core/device_digital_twins/__init__.py # simpa/core/device_digital_twins/detection_geometries/__init__.py # simpa/core/device_digital_twins/illumination_geometries/__init__.py # simpa/core/device_digital_twins/pa_devices/__init__.py # simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py
…ions # Conflicts: # simpa/core/device_digital_twins/__init__.py # simpa/core/device_digital_twins/detection_geometries/__init__.py # simpa/core/device_digital_twins/illumination_geometries/__init__.py # simpa/core/device_digital_twins/pa_devices/__init__.py # simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for segmentation-based volume creation for the MSOT Acuity Echo device. It introduces a new method to handle volume settings when using segmentation-based volume creators, including the ability to automatically add layers of ultrasound gel, mediprene, and heavy water to match the physical device configuration.
Key Changes
- Added new configuration tags (
VOLUME_FRACTION,ADD_US_GEL,ADD_MEDIPRENE,ADD_HEAVY_WATER) for segmentation layer management - Implemented
update_settings_for_use_of_segmentation_based_volume_creator()method across the device twin hierarchy - Added type hints to various device geometry classes for improved code clarity
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| simpa/utils/tags.py | Added new tags for volume fraction and layer addition controls |
| simpa/core/device_digital_twins/pa_devices/photoacoustic_device.py | Added empty abstract method for segmentation-based volume creator |
| simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py | Implemented segmentation volume adjustment logic with layer addition |
| simpa/core/device_digital_twins/pa_devices/ithera_rsom.py | Added type hints to constructor parameters |
| simpa/core/device_digital_twins/digital_device_twin_base.py | Added abstract method for segmentation-based volume creator and type hints |
| simpa/core/device_digital_twins/illumination_geometries/*.py | Added type hints and implemented segmentation-based volume creator methods |
| simpa/core/device_digital_twins/detection_geometries/*.py | Added type hints and implemented segmentation-based volume creator methods |
Comments suppressed due to low confidence (3)
simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py:325
- Variable old_volume_fraction is not used.
old_volume_fraction = getattr(molecule, Tags.VOLUME_FRACTION)
simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py:336
- Variable device_change_in_height is not used.
device_change_in_height = mediprene_layer_height_mm + heavy_water_layer_height_mm
simpa/core/device_digital_twins/detection_geometries/detection_geometry_base.py:134
- Call to DetectionGeometryBase.init with too few arguments; should be no fewer than 6.
deserialized_device = DetectionGeometryBase()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_layers: list = [Tags.ADD_US_GEL, | ||
| Tags.ADD_MEDIPRENE, | ||
| Tags.ADD_HEAVY_WATER], |
There was a problem hiding this comment.
The type hint list for add_layers is too restrictive. Since the parameter is used with in operator for membership testing (lines 253, 264, 273), it should accept any iterable. Consider using list[str] for better type safety or Iterable[str] if you want to support other iterables.
| mediprene_layer_height_pix = int(round(mediprene_layer_height_mm/spacing_mm)) | ||
| padding_dims = ((0, 0), (0, 0), (mediprene_layer_height_pix, 0)) | ||
| segmentation_map = np.pad(segmentation_map, padding_dims, mode='constant', constant_values=128) | ||
| segmentation_class_mapping[128] = TISSUE_LIBRARY.mediprene() |
There was a problem hiding this comment.
TISSUE_LIBRARY is undefined. The import on line 14 uses TissueLibrary (camelCase), not TISSUE_LIBRARY (uppercase). Change this to TissueLibrary.mediprene() to match the import.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
simpa/core/device_digital_twins/pa_devices/ithera_msot_acuity.py
Outdated
Show resolved
Hide resolved
| padded_vol_sos = np.pad(padded_up_sos, padding_width, mode='edge') | ||
| setattr(molecule, key, torch.tensor(padded_vol_sos, dtype=torch.float32)) | ||
|
|
||
| device_change_in_height = mediprene_layer_height_mm + heavy_water_layer_height_mm |
There was a problem hiding this comment.
The variable device_change_in_height is computed but never used. Consider removing it if it's not needed, or using it in the device position calculations if it was intended to be used.
| device_change_in_height = mediprene_layer_height_mm + heavy_water_layer_height_mm |
| add_layers: list = [Tags.ADD_US_GEL, | ||
| Tags.ADD_MEDIPRENE, | ||
| Tags.ADD_HEAVY_WATER], | ||
| current_heavy_water_depth: (float, int) = 0, |
There was a problem hiding this comment.
The type hint (float, int) uses outdated tuple syntax. For consistency with the rest of the codebase (e.g., lines 17-19 in curved_array.py, line 18 in gaussian_beam_illumination.py), use Union[float, int] instead. You'll also need to add from typing import Union to the imports.
| }) | ||
| volume_creator_settings[Tags.STRUCTURES][Tags.BACKGROUND] = background_settings | ||
|
|
||
| def update_settings_for_use_of_segmentation_based_volume_creator(self, global_settings, |
There was a problem hiding this comment.
Missing type hint for global_settings parameter. For consistency with other methods in this file and the abstract method signature in the base class (digital_device_twin_base.py line 87), add : Settings type hint.
| def update_settings_for_use_of_segmentation_based_volume_creator(self, global_settings, | |
| def update_settings_for_use_of_segmentation_based_volume_creator(self, global_settings: Settings, |
| VOLUME_FRACTION = "volume_fraction" | ||
| """ | ||
| Identifier for the volume fraction for the simulation | ||
| Usage: simpa.core.digital_device_twins.pa_devices.ithera_msot_acuity | ||
| """ |
There was a problem hiding this comment.
This assignment to 'VOLUME_FRACTION' is unnecessary as it is redefined before this value is used.
| VOLUME_FRACTION = "volume_fraction" | |
| """ | |
| Identifier for the volume fraction for the simulation | |
| Usage: simpa.core.digital_device_twins.pa_devices.ithera_msot_acuity | |
| """ |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: jgroehl <7871475+jgroehl@users.noreply.github.com>
Fix TISSUE_LIBRARY undefined name error in MSOTAcuityEcho
Please check the following before creating the pull request (PR):
Provide issue / feature request fixed by this PR
Fixes #348