Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add reading skew from czi files #1205

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

feat: add reading skew from czi files #1205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3e8a54
feat: add reading skew from czi files
Czaki Oct 9, 2024
7fdfeef
Merge branch 'develop' into add_read_skew
Czaki Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions package/PartSegCore/napari_plugins/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ def add_color(image: Image, idx: int) -> dict: # noqa: ARG001
return {}


def _image_to_layers(project_info, scale, translate):
def _image_to_layers(project_info, scale, translate, shear):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure 'shear' parameter is compatible with all targeted napari versions

The addition of the shear parameter to the _image_to_layers function enhances functionality. However, it's important to verify that the shear parameter is supported in all versions of napari that your application targets. Using unsupported parameters could lead to runtime errors in older versions of napari.

Consider adding a version check, similar to the one used for the add_color function, to conditionally include the shear parameter based on the napari version.

res_layers = []
if project_info.image.name == "ROI" and project_info.image.channels == 1:
res_layers.append(
(
project_info.image.get_channel(0),
{"scale": scale, "name": project_info.image.channel_names[0], "translate": translate},
{"scale": scale, "name": project_info.image.channel_names[0], "translate": translate, "shear": shear},
"labels",
)
)
Expand All @@ -71,6 +71,7 @@ def _image_to_layers(project_info, scale, translate):
"blending": "additive",
"translate": translate,
"metadata": project_info.image.metadata,
"shear": shear,
**add_color(project_info.image, i),
},
"image",
Expand All @@ -85,14 +86,15 @@ def project_to_layers(project_info: typing.Union[ProjectTuple, MaskProjectTuple]
res_layers = []
if project_info.image is not None and not isinstance(project_info.image, str):
scale = project_info.image.normalized_scaling()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Extract code out into function (extract-method)

shear = project_info.image.shear
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential None values for shear

When assigning shear = project_info.image.shear, there's a possibility that project_info.image.shear might be None or not defined, especially if the image doesn't have shear information. Passing None to layer properties could cause unexpected behavior or errors.

Consider adding a check to handle None values or provide a default shear value to ensure robust code execution.

Apply this diff to handle possible None values:

- shear = project_info.image.shear
+ shear = project_info.image.shear if project_info.image.shear is not None else np.zeros((ndim, ndim))

Replace ndim with the appropriate number of dimensions for your shear matrix.

Committable suggestion was skipped due to low confidence.

translate = project_info.image.shift
translate = (0,) * (len(project_info.image.axis_order.replace("C", "")) - len(translate)) + translate
res_layers.extend(_image_to_layers(project_info, scale, translate))
res_layers.extend(_image_to_layers(project_info, scale, translate, shear))
if project_info.roi_info.roi is not None:
res_layers.append(
(
project_info.image.fit_array_to_image(project_info.roi_info.roi),
{"scale": scale, "name": "ROI", "translate": translate},
{"scale": scale, "name": "ROI", "translate": translate, "shear": shear},
"labels",
)
)
Expand All @@ -105,6 +107,7 @@ def project_to_layers(project_info: typing.Union[ProjectTuple, MaskProjectTuple]
"name": name,
"translate": translate,
"visible": False,
"shear": shear,
},
"labels",
)
Expand All @@ -115,7 +118,7 @@ def project_to_layers(project_info: typing.Union[ProjectTuple, MaskProjectTuple]
res_layers.append(
(
project_info.image.fit_array_to_image(project_info.mask),
{"scale": scale, "name": "Mask", "translate": translate},
{"scale": scale, "name": "Mask", "translate": translate, "shear": shear},
"labels",
)
)
Expand Down
6 changes: 6 additions & 0 deletions package/PartSegImage/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ def __init__(
channel_info: list[ChannelInfo | ChannelInfoFull] | None = None,
axes_order: str | None = None,
shift: Spacing | None = None,
shear: np.ndarray | None = None,
name: str = "",
metadata_dict: dict | None = None,
):
Expand All @@ -243,6 +244,7 @@ def __init__(
self._image_spacing = tuple(el if el > 0 else 10**-6 for el in self._image_spacing)

self._shift = tuple(shift) if shift is not None else (0,) * len(self._image_spacing)
self._shear = shear
self.name = name

self.file_path = file_path
Expand Down Expand Up @@ -369,6 +371,10 @@ def _merge_channel_names(base_channel_names: list[str], new_channel_names: list[
base_channel_names.append(new_name)
return base_channel_names

@property
def shear(self) -> np.ndarray | None:
return self._shear

@property
def channel_info(self) -> list[ChannelInfoFull]:
return [copy(x) for x in self._channel_info]
Expand Down
29 changes: 29 additions & 0 deletions package/PartSegImage/image_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import inspect
import os.path
import typing
import warnings
from abc import abstractmethod
from contextlib import suppress
from importlib.metadata import version
Expand Down Expand Up @@ -389,8 +390,36 @@
axes_order=self.return_order(),
metadata_dict=metadata,
channel_info=self._get_channel_info(),
shear=self._read_shear(metadata),
)

def _read_shear(self, metadata: dict):
skew = self._read_skew(metadata)
shear = np.diag([1.0] * len(skew))
for i, val in enumerate(skew):
if val == 0:
continue
shear[i, i + 1] = np.tan(np.radians(val))

Check warning on line 402 in package/PartSegImage/image_reader.py

View check run for this annotation

Codecov / codecov/patch

package/PartSegImage/image_reader.py#L402 

Added line #L402 was not covered by tests
return shear

Comment on lines +396 to +404
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential IndexError in _read_shear due to out-of-bounds access.

In the _read_shear method, the loop may attempt to access shear[i, i + 1] when i + 1 equals the size of the matrix, resulting in an IndexError. This occurs when i reaches the last index.

Apply this diff to prevent the IndexError by adjusting the loop:

 def _read_shear(self, metadata: dict):
     skew = self._read_skew(metadata)
     shear = np.diag([1.0] * len(skew))
-    for i, val in enumerate(skew):
+    for i, val in enumerate(skew[:-1]):
         if val == 0:
             continue
         shear[i, i + 1] = np.tan(np.radians(val))
     return shear

Alternatively, add a boundary check within the loop:

 def _read_shear(self, metadata: dict):
     skew = self._read_skew(metadata)
     shear = np.diag([1.0] * len(skew))
     for i, val in enumerate(skew):
         if val == 0:
             continue
+        if i + 1 >= len(skew):
+            continue
         shear[i, i + 1] = np.tan(np.radians(val))
     return shear
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _read_shear(self, metadata: dict):
skew = self._read_skew(metadata)
shear = np.diag([1.0] * len(skew))
for i, val in enumerate(skew):
if val == 0:
continue
shear[i, i + 1] = np.tan(np.radians(val))
return shear
def _read_shear(self, metadata: dict):
skew = self._read_skew(metadata)
shear = np.diag([1.0] * len(skew))
for i, val in enumerate(skew[:-1]):
if val == 0:
continue
shear[i, i + 1] = np.tan(np.radians(val))
return shear

@staticmethod
def _read_skew(metadata: dict):
dimensions = metadata["ImageDocument"]["Metadata"]["Information"]["Image"]["Dimensions"]
res = [0.0] * 4
for i, dim in enumerate("TZYX"):
if dim not in dimensions:
continue
if f"{dim}AxisShear" not in dimensions[dim]:
continue

shear_value = dimensions[dim][f"{dim}AxisShear"]
if not shear_value.startswith("Skew"):
warnings.warn(f"Unknown shear value {shear_value}", stacklevel=1)
continue
res[i] = float(shear_value[4:])

Check warning on line 419 in package/PartSegImage/image_reader.py

View check run for this annotation

Codecov / codecov/patch

package/PartSegImage/image_reader.py#L415-L419 

Added lines #L415 - L419 were not covered by tests
Comment on lines +416 to +419
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential ValueError when converting shear value to float.

Converting shear_value[4:] to float may raise a ValueError if the substring cannot be converted to a number. Adding error handling will prevent the program from crashing in such cases.

Apply this diff to add a try-except block:

 if not shear_value.startswith("Skew"):
     warnings.warn(f"Unknown shear value {shear_value}", stacklevel=2)
     continue
+try:
     res[i] = float(shear_value[4:])
+except ValueError:
+    warnings.warn(f"Invalid shear value '{shear_value}' cannot be converted to float.", stacklevel=2)
+    res[i] = 0.0
     continue
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not shear_value.startswith("Skew"):
warnings.warn(f"Unknown shear value {shear_value}", stacklevel=1)
continue
res[i] = float(shear_value[4:])
if not shear_value.startswith("Skew"):
warnings.warn(f"Unknown shear value {shear_value}", stacklevel=2)
continue
try:
res[i] = float(shear_value[4:])
except ValueError:
warnings.warn(f"Invalid shear value '{shear_value}' cannot be converted to float.", stacklevel=2)
res[i] = 0.0
continue


return res

@classmethod
def update_array_shape(cls, array: np.ndarray, axes: str):
if "B" in axes:
Expand Down