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.

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

Grayscale property passthrough #99

Merged
merged 3 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
10 changes: 7 additions & 3 deletions sleap_io/io/video.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ def num_frames(self) -> int:
@property
def img_shape(self) -> Tuple[int, int, int]:
"""Shape of a single frame in the video."""
height, width, channels = self.get_frame(0).shape
height, width, channels = self.read_test_frame().shape
if self.grayscale is False:
channels = 3
elif self.grayscale is True:
channels = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Refine the implementation of img_shape property.

The method to determine the img_shape based on the grayscale property is overly complex and can be simplified. Additionally, this method should not directly read a frame to determine the shape; it should utilize cached values to improve efficiency.

@property
def img_shape(self) -> tuple[int, int, int]:
-    height, width, channels = self.read_test_frame().shape
+    if self._cached_shape is not None:
+        return self._cached_shape[1:]  # Extract height, width, channels from cached shape
-    if self.grayscale is False:
-        channels = 3
-    elif self.grayscale is True:
-        channels = 1
-    return int(height), int(width), int(channels)
+    raise ValueError("Shape not cached")

Committable suggestion was skipped due to low confidence.

Tools
Ruff

161-161: Use tuple instead of Tuple for type annotation (UP006)

Replace with tuple

return int(height), int(width), int(channels)

@property
Expand Down Expand Up @@ -578,7 +582,7 @@ def read_test_frame(self) -> np.ndarray:
frame_idx = list(self.frame_map.keys())[0]
else:
frame_idx = 0
return self.read_frame(frame_idx)
return self._read_frame(frame_idx)

@property
def has_embedded_images(self) -> bool:
Expand Down Expand Up @@ -699,7 +703,7 @@ class ImageVideo(VideoBackend):
This backend supports reading videos stored as a list of images.

Attributes:
filename: Path to video files.
filename: Path to image files.
grayscale: Whether to force grayscale. If None, autodetect on first frame load.
"""

Expand Down
10 changes: 9 additions & 1 deletion sleap_io/model/video.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def _get_shape(self) -> Tuple[int, int, int, int] | None:
return self.backend_metadata["shape"]
return None

@property
@property.getter
def grayscale(self) -> bool | None:
"""Return whether the video is grayscale.

Expand All @@ -135,6 +135,14 @@ def grayscale(self) -> bool | None:
return self.backend_metadata["grayscale"]
return None

@property.setter
def grayscale(self, value: bool):
"""Set the grayscale value and adjust the backend."""
self.backend.grayscale = value
self.backend._cached_shape = None
if "grayscale" in self.backend_metadata:
self.backend_metadata["grayscale"] = value

def __len__(self) -> int:
"""Return the length of the video as the number of frames."""
shape = self.shape
Expand Down
9 changes: 9 additions & 0 deletions tests/model/test_video.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,12 @@ def test_video_replace_filename(
video.replace_filename(centered_pair_frame_paths)
assert type(video.backend) == ImageVideo
assert video.exists(check_all=True) is True


def test_grayscale(centered_pair_low_quality_path):
video = Video.from_filename(centered_pair_low_quality_path)
assert video.grayscale == True
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize boolean check.

Avoid direct equality comparisons to True; use truthiness checks for a cleaner and more Pythonic approach.

-    assert video.grayscale == True
+    assert video.grayscale
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
assert video.grayscale == True
assert video.grayscale
Tools
Ruff

129-129: Avoid equality comparisons to True; use if video.grayscale: for truth checks

Replace with video.grayscale

(E712)

assert video.shape[-1] == 1

video.grayscale = False
assert video.shape[-1] == 3
Comment on lines +127 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper boolean checks and optimize the test function.

The test for the grayscale property should avoid direct equality checks against boolean values. Instead, use the truthiness of the value for a cleaner and more Pythonic approach.

- assert video.grayscale == True
+ assert video.grayscale

Additionally, consider adding assertions to ensure that the grayscale property toggles correctly between True and False states, affecting the shape as expected.

Committable suggestion was skipped due to low confidence.

Tools
Ruff

129-129: Avoid equality comparisons to True; use if video.grayscale: for truth checks (E712)

Replace with video.grayscale

Loading