Add better support for exporting and loading RGB videos from .pkg.slp files #125
Conversation
|
Warning Rate limit exceeded@talmo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/io/test_slp.py (1)
359-370: LGTM! Consider enhancing the test coverage.The new test function
test_embed_rgbeffectively verifies the basic functionality of embedding RGB video data. It aligns well with the PR objective of improving support for RGB videos. However, consider the following enhancements to make the test more robust:
- Verify that the actual RGB data is preserved correctly after embedding and reading back.
- Test different embedding options (e.g., 'all', 'suggestions') to ensure they work correctly with RGB data.
- Check if the
grayscaleattribute is preserved after reading the embedded data.- Add assertions to verify that the video backend type changes as expected (from
MediaVideotoHDF5Video).Here's a suggested enhancement to the test function:
def test_embed_rgb(tmpdir, slp_real_data): base_labels = read_labels(slp_real_data) base_labels.video.grayscale = False assert base_labels.video.shape == (1100, 384, 384, 3) assert base_labels.video[0].shape == (384, 384, 3) # Store original data for comparison original_frame = base_labels.video[0].copy() for embed_option in ['user', 'all', 'suggestions']: labels_path = str(tmpdir / f"labels_{embed_option}.pkg.slp") write_labels(labels_path, base_labels, embed=embed_option) labels = read_labels(labels_path) assert labels.video[0].shape == (384, 384, 3) assert labels.video.grayscale == False assert type(labels.video.backend) == HDF5Video # Check if the RGB data is preserved np.testing.assert_array_equal(labels.video[0], original_frame) # Test source embedding source_labels_path = str(tmpdir / "labels_source.slp") write_labels(source_labels_path, labels, embed='source') source_labels = read_labels(source_labels_path) assert type(source_labels.video.backend) == MediaVideoThis enhanced version tests multiple embedding options, verifies data preservation, and checks backend types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sleap_io/io/slp.py (3 hunks)
- tests/io/test_slp.py (1 hunks)
🔇 Additional comments (1)
sleap_io/io/slp.py (1)
273-274: Ensure proper handling of single-channel frames before savingThe code squeezes the last axis of the frame if
frame.shape[-1] == 1to handle single-channel images appropriately. Verify that this operation behaves correctly for all expected frame shapes and does not inadvertently alter multi-channel images.Run the following script to confirm that frames with different shapes are handled correctly:
✅ Verification successful
Proper handling of single-channel frames confirmed
The squeeze operation is correctly applied only to single-channel frames (
frame.shape[-1] == 1), ensuring that multi-channel images are not inadvertently altered.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check frame shapes after squeezing operation. # Test: List unique frame shapes in the dataset. Expect: Correct shapes without unintended squeezing. ast-grep --lang python --pattern $'for frame_idx in $_:\n frame = video[$_]\n if frame.shape[-1] == 1:\n frame = frame.squeeze(axis=-1)\n $$$\n' \ | xargs -I{} echo "Found squeezing operation in {}"Length of output: 210
Script:
#!/bin/bash # Description: Search for squeeze operations in sleap_io/io/slp.py and ensure proper handling. # Search for lines where squeeze is called with axis=-1 rg 'frame\.squeeze\s*\(\s*axis\s*=\s*-1\s*\)' sleap_io/io/slp.py -C 2Length of output: 304
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 96.21% 96.48% +0.26%
==========================================
Files 15 15
Lines 2033 2046 +13
==========================================
+ Hits 1956 1974 +18
+ Misses 77 72 -5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
sleap_io/model/video.py (1)
Line range hint
9-9: Remove duplicate import ofOptionalfrom typing moduleThe
Optionaltype is imported twice in the import statement. This is redundant and can be removed to clean up the code.Apply this diff to remove the duplicate import:
-from typing import Tuple, Optional, Optional +from typing import Tuple, Optional
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- sleap_io/io/slp.py (3 hunks)
- sleap_io/model/video.py (2 hunks)
- tests/io/test_slp.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sleap_io/io/slp.py
- tests/io/test_slp.py
sleap_io/model/video.py
Outdated
| grayscale = None | ||
| if "grayscale" in self.backend_metadata: | ||
| grayscale = self.backend_metadata["grayscale"] | ||
| elif "shape" in self.backend_metadata: | ||
| grayscale = self.backend_metadata["shape"][-1] == 1 | ||
| return grayscale |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor grayscale determination logic into a helper method
The logic for determining grayscale from backend_metadata is duplicated in both the grayscale property and the open method. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider refactoring this logic into a private helper method.
Apply this diff to lines 146-151 to use the helper method:
else:
grayscale = None
- if "grayscale" in self.backend_metadata:
- grayscale = self.backend_metadata["grayscale"]
- elif "shape" in self.backend_metadata:
- grayscale = self.backend_metadata["shape"][-1] == 1
+ grayscale = self._determine_grayscale_from_metadata()
return grayscaleAdd the following helper method to the class:
def _determine_grayscale_from_metadata(self) -> Optional[bool]:
"""Helper method to determine grayscale from backend_metadata."""
if "grayscale" in self.backend_metadata:
return self.backend_metadata["grayscale"]
elif "shape" in self.backend_metadata:
return self.backend_metadata["shape"][-1] == 1
return None| if grayscale is None: | ||
| if "grayscale" in self.backend_metadata: | ||
| grayscale = self.backend_metadata["grayscale"] | ||
| elif "shape" in self.backend_metadata: | ||
| grayscale = self.backend_metadata["shape"][-1] == 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor grayscale determination logic into a helper method
As previously mentioned, the logic for determining grayscale from backend_metadata is duplicated. Reusing the helper method improves code readability and maintainability.
Apply this diff to lines 279-283 to use the helper method:
else:
if grayscale is None:
- if "grayscale" in self.backend_metadata:
- grayscale = self.backend_metadata["grayscale"]
- elif "shape" in self.backend_metadata:
- grayscale = self.backend_metadata["shape"][-1] == 1
+ grayscale = self._determine_grayscale_from_metadata()Committable suggestion was skipped due to low confidence.
greyscaleattribute is now stored in the backend metadataSummary by CodeRabbit
New Features
Bug Fixes
Tests