Extract clip as video, slp and pkg.slp #2059
Conversation
WalkthroughThis pull request introduces functionality for exporting video clips and their associated labels within the SLEAP application. Key changes include the addition of new menu items in the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 4
🧹 Nitpick comments (2)
tests/gui/test_commands.py (2)
1101-1182: Consider adding edge cases to frame range tests.While the current test coverage is good, consider adding these edge cases:
- Empty frame range
- Single frame export
- Frame range at video boundaries
Example test cases to add:
@pytest.mark.parametrize("subset", [ (0, 0), # Single frame (0, 9), # Full range (9, 9), # Last frame ]) def test_ExportVideoClip_edge_cases(subset, tmpdir): # Similar setup as existing test subset_start, subset_end = subset # ... test implementation
12-12: Remove unused import.The
Nodeimport fromsleap.skeletonis not used in this file.-from sleap.skeleton import Node🧰 Tools
🪛 Ruff (0.8.2)
12-12:
sleap.skeleton.Nodeimported but unusedRemove unused import:
sleap.skeleton.Node(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sleap/gui/app.py(1 hunks)sleap/gui/commands.py(2 hunks)sleap/gui/dialogs/export_clip.py(2 hunks)tests/gui/test_commands.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3487-3487: sleap.Labels imported but unused
Remove unused import: sleap.Labels
(F401)
3633-3633: Redefinition of unused ExportVideoClip from line 3476
(F811)
3636-3636: sleap.io.visuals.save_labeled_video imported but unused
Remove unused import: sleap.io.visuals.save_labeled_video
(F401)
3639-3639: numpy imported but unused
Remove unused import: numpy
(F401)
tests/gui/test_commands.py
12-12: sleap.skeleton.Node imported but unused
Remove unused import: sleap.skeleton.Node
(F401)
🔇 Additional comments (3)
sleap/gui/dialogs/export_clip.py (1)
31-63: LGTM! Well-structured dialog initialization.
The dialog setup is clean and follows good practices:
- Proper inheritance from FormBuilderModalDialog
- Reasonable FPS range (1-240)
- Clear user feedback about encoding method
tests/gui/test_commands.py (1)
1034-1100: LGTM! Comprehensive test coverage for file creation.
The test thoroughly verifies:
- Creation of all required files (mp4, slp, pkg.slp)
- Correct frame count in exported labels
sleap/gui/app.py (1)
749-756: LGTM! Clean menu integration.
The new menu item is well-placed and follows the existing menu structure conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
sleap/gui/commands.py (2)
3489-3489: Remove unused import.The
Labelsimport is not used in this implementation.-from sleap import Labels🧰 Tools
🪛 Ruff (0.8.2)
3489-3489:
sleap.Labelsimported but unusedRemove unused import:
sleap.Labels(F401)
3478-3542: Consider separating video export and label rendering concerns.The current implementation mixes video export with label rendering functionality. Consider:
- Separating the video export and label rendering into distinct methods for better maintainability
- Making the
render_labelsparameter more consistent throughout the implementation@staticmethod def do_action(context: CommandContext, params: dict): video = context.state["video"] labels = context.state["labels"] # Extract frames frame_range = context.state.get("frame_range", (0, video.frames)) pruned_labels = labels.extract( inds=range(*frame_range), copy=True, ) # Remap frame indices for labeled_frame in pruned_labels.labeled_frames: labeled_frame.frame_idx -= frame_range[0] if video not in pruned_labels.videos: pruned_labels.add_video(video) + def _export_video(params: dict, render_labels: bool): + save_labeled_video( + filename=params["filename"], + labels=context.state["labels"] if render_labels else None, + video=context.state["video"], + frames=list(params["frames"]), + fps=params["fps"], + color_manager=params["color_manager"], + background=params["background"], + show_edges=params["show_edges"] if render_labels else False, + edge_is_wedge=params["edge_is_wedge"] if render_labels else False, + marker_size=params["marker_size"] if render_labels else 0, + scale=params["scale"], + crop_size_xy=params["crop"], + gui_progress=True, + ) # Export video with optional labels - save_labeled_video(...) + _export_video(params, params["render_labels"])🧰 Tools
🪛 Ruff (0.8.2)
3489-3489:
sleap.Labelsimported but unusedRemove unused import:
sleap.Labels(F401)
tests/gui/test_commands.py (2)
1034-1106: Enhance test coverage for ExportVideoClip.The test verifies basic functionality but could be improved by:
- Testing error cases (e.g., invalid frame ranges, video format issues)
- Verifying video content/quality
- Adding assertions for label content in the exported .slp files
- Testing the render_labels parameter
Example additions:
def test_ExportVideoClip_handles_errors(tmpdir): """Test that ExportVideoClip handles error cases gracefully.""" # Test invalid frame range with pytest.raises(ValueError): # Setup similar to test_ExportVideoClip_creates_files params["frames"] = range(100, 0) # Invalid range ExportVideoClip.do_action(context, params) def test_ExportVideoClip_with_labels(tmpdir): """Test that ExportVideoClip correctly handles label rendering.""" # Setup similar to test_ExportVideoClip_creates_files params["render_labels"] = True ExportVideoClip.do_action(context, params) # Verify label content in exported files exported_labels = Labels.load_file(str(slp_path)) assert len(exported_labels.labeled_frames) == frame_count # Add more specific assertions about label content
1108-1191: Add edge cases to frame and video list size tests.Consider adding test cases for:
- Empty frame ranges
- Single-frame clips
- Frame ranges at video boundaries
- Multiple videos
Example additions:
@pytest.mark.parametrize("frame_range", [ (0, 1), # Single frame (0, 0), # Empty range (8, 10), # End of video (None, None), # Full video ]) def test_ExportVideoClip_frame_ranges(tmpdir, frame_range): """Test that ExportVideoClip handles various frame ranges correctly.""" # Setup similar to test_ExportVideoClip_frame_and_video_list_sizes if frame_range != (None, None): context.state["frame_range"] = frame_range ExportVideoClip.do_action(context, params) # Add assertions specific to each frame range casesleap/gui/dialogs/export_clip.py (3)
44-44: Document the rationale for FPS range limits.The FPS range is hardcoded to 1-240. Consider adding a comment explaining these limits or make them configurable constants.
+ # Standard FPS range: 1 fps (timelapse) to 240 fps (slow motion) self.fps_input.setRange(1, 240)
33-66: Consider optimizing dialog inheritance structure.The current implementation duplicates some logic from
ExportClipDialog. Consider:
- Moving common functionality (like ffmpeg message) to the parent class
- Creating a base class for both dialogs
Example refactor:
class BaseClipExportDialog(FormBuilderModalDialog): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._add_ffmpeg_message() def _add_ffmpeg_message(self): from sleap.io.videowriter import VideoWriter can_use_ffmpeg = VideoWriter.can_use_ffmpeg() message = self._get_encoder_message(can_use_ffmpeg) self.add_message(message) @staticmethod def _get_encoder_message(can_use_ffmpeg): if can_use_ffmpeg: return ("<i><b>MP4</b> file will be encoded using " "system ffmpeg via imageio (preferred option).</i>") return ("<i>Unable to use ffmpeg via imageio. " "<b>AVI</b> file will be encoded using OpenCV.</i>")
37-38: Consider maintaining consistency in form definition approach.While
ExportClipDialoguses YAML-based form definition, this class uses direct widget manipulation. Consider using a consistent approach across dialogs for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sleap/gui/commands.py(2 hunks)sleap/gui/dialogs/export_clip.py(2 hunks)tests/gui/test_commands.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3489-3489: sleap.Labels imported but unused
Remove unused import: sleap.Labels
(F401)
3640-3640: Redefinition of unused ExportVideoClip from line 3478
(F811)
3643-3643: sleap.io.visuals.save_labeled_video imported but unused
Remove unused import: sleap.io.visuals.save_labeled_video
(F401)
3646-3646: numpy imported but unused
Remove unused import: numpy
(F401)
tests/gui/test_commands.py
12-12: sleap.skeleton.Node imported but unused
Remove unused import: sleap.skeleton.Node
(F401)
🔇 Additional comments (2)
sleap/gui/commands.py (1)
3769-3777: Move or remove unused clipboard function.
The copy_to_clipboard function appears to be:
- Unused in the visible code
- Not following the command pattern used throughout the file
- Misplaced after class definitions
sleap/gui/dialogs/export_clip.py (1)
67-81: Refactor duplicate code in result handling methods.
The on_accept and get_results methods contain identical code for creating the results dictionary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2059 +/- ##
===========================================
+ Coverage 73.30% 75.30% +1.99%
===========================================
Files 134 134
Lines 24087 24832 +745
===========================================
+ Hits 17658 18700 +1042
+ Misses 6429 6132 -297 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sleap/gui/commands.py (2)
3481-3485: Remove unused imports.The following imports are not used in the implementation:
save_labeled_videofromsleap.io.visualsnumpyasnp-from sleap.io.visuals import save_labeled_video from sleap.io.video import MediaVideo, Video import cv2 -import numpy as np🧰 Tools
🪛 Ruff (0.8.2)
3481-3481:
sleap.io.visuals.save_labeled_videoimported but unusedRemove unused import:
sleap.io.visuals.save_labeled_video(F401)
3484-3484:
numpyimported but unusedRemove unused import:
numpy(F401)
3506-3511: Consider making the video codec configurable.The code uses a hard-coded 'mp4v' codec which might not be available on all systems. Consider:
- Making the codec configurable
- Providing a fallback option
- Adding error handling for codec initialization
- fourcc = cv2.VideoWriter_fourcc(*"mp4v") + codec = params.get("codec", "mp4v") + try: + fourcc = cv2.VideoWriter_fourcc(*codec) + except Exception as e: + logger.warning(f"Failed to initialize {codec} codec, falling back to XVID") + fourcc = cv2.VideoWriter_fourcc(*"XVID")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/commands.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3481-3481: sleap.io.visuals.save_labeled_video imported but unused
Remove unused import: sleap.io.visuals.save_labeled_video
(F401)
3484-3484: numpy imported but unused
Remove unused import: numpy
(F401)
🔇 Additional comments (2)
sleap/gui/commands.py (2)
630-633: LGTM!
The method follows the established pattern for command execution in the CommandContext class.
3607-3615: Move or remove unused clipboard function.
The copy_to_clipboard function appears to be:
- Unused in the visible code
- Not following the command pattern used throughout the file
- Misplaced after class definitions
Either:
- Remove the function if it's no longer needed
- Move it into a proper command class following the pattern:
class CopyToClipboard(AppCommand):
@staticmethod
def do_action(context: CommandContext, params: dict):
text = params["text"]
clipboard = QtWidgets.QApplication.clipboard()
clipboard.clear(mode=clipboard.Clipboard)
clipboard.setText(text, mode=clipboard.Clipboard)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/app.py(1 hunks)sleap/gui/commands.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sleap/gui/app.py
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/commands.py
52-52: sleap.gui.dialogs.export_clip.ExportClipAndLabelsDialog imported but unused
Remove unused import: sleap.gui.dialogs.export_clip.ExportClipAndLabelsDialog
(F401)
3527-3527: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3563-3563: Redefinition of unused ExportClipAndLabelsDialog from line 52
Remove definition: ExportClipAndLabelsDialog
(F811)
🔇 Additional comments (1)
sleap/gui/commands.py (1)
3597-3606: 🛠️ Refactor suggestion
Move clipboard function into a command class.
The copy_to_clipboard function doesn't follow the command pattern used throughout the file. Convert it to a proper command class:
-def copy_to_clipboard(text: str):
- """Copy a string to the system clipboard.
-
- Args:
- text: String to copy to clipboard.
- """
- clipboard = QtWidgets.QApplication.clipboard()
- clipboard.clear(mode=clipboard.Clipboard)
- clipboard.setText(text, mode=clipboard.Clipboard)
+class CopyToClipboard(AppCommand):
+ @staticmethod
+ def do_action(context: CommandContext, params: dict):
+ """Copy a string to the system clipboard.
+
+ Args:
+ context: The command context
+ params: Dictionary containing:
+ text: String to copy to clipboard
+ """
+ text = params["text"]
+ clipboard = QtWidgets.QApplication.clipboard()
+ clipboard.clear(mode=clipboard.Clipboard)
+ clipboard.setText(text, mode=clipboard.Clipboard)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sleap/gui/commands.py (2)
3520-3533: Add progress feedback for video export.Consider adding a progress dialog to provide feedback during video export, especially for large frame ranges.
+ progress = QtWidgets.QProgressDialog( + "Exporting video...", "Cancel", 0, frame_range[1] - frame_range[0]) + progress.setWindowModality(QtCore.Qt.WindowModal) + # Write frames to the video for frame_idx in range(*frame_range): + if progress.wasCanceled(): + writer.close() + os.remove(params["filename"]) + return + try: frame = video.get_frame(frame_idx) # Convert grayscale to BGR if necessary if frame.ndim == 2: # Grayscale frames frame = cv2.cvtColor(frame, cv2.COLOR_GRAY2BGR) writer.add_frame(frame, bgr=True) + progress.setValue(frame_idx - frame_range[0]) + QtWidgets.QApplication.processEvents() except KeyError as err: + writer.close() + os.remove(params["filename"]) raise KeyError(f"Failed to load frame {frame_idx} from video.") from err🧰 Tools
🪛 Ruff (0.8.2)
3531-3531: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3640-3641: Add progress feedback for package save operation.Consider adding a progress dialog for the package save operation which can be slow with embedded images:
+ progress = QtWidgets.QProgressDialog( + "Saving package with embedded images...", "Cancel", 0, 100) + progress.setWindowModality(QtCore.Qt.WindowModal) + + def update_progress(n, total): + if progress.wasCanceled(): + return False + progress.setValue(int((n/total) * 100)) + QtWidgets.QApplication.processEvents() + return True + + try: pkg_filename = params["filename"] - pruned_labels.save(pkg_filename, with_images=True) + pruned_labels.save( + pkg_filename, + with_images=True, + progress_callback=update_progress + ) + except Exception as err: + if os.path.exists(pkg_filename): + os.remove(pkg_filename) + raise RuntimeError(f"Failed to save package: {err}") from err
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/commands.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3531-3531: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
sleap/gui/commands.py (3)
3599-3608: Move or remove unused clipboard function.
The copy_to_clipboard function appears to be:
- Unused in the visible code
- Not following the command pattern used throughout the file
- Misplaced after class definitions
3640-3641: 🛠️ Refactor suggestion
Add error handling for file operations.
The package save operation should handle potential file I/O errors:
- pkg_filename = params["filename"]
- pruned_labels.save(pkg_filename, with_images=True)
+ try:
+ pkg_filename = params["filename"]
+ pruned_labels.save(pkg_filename, with_images=True)
+ except Exception as err:
+ if os.path.exists(pkg_filename):
+ os.remove(pkg_filename)
+ raise RuntimeError(f"Failed to save package: {err}") from errLikely invalid or redundant comment.
3498-3503: 🛠️ Refactor suggestion
Enhance frame range validation.
The frame range validation could be more robust by checking for valid ranges and bounds:
# Ensure frame range is set; default to all frames if None
frame_range = context.state.get("frame_range", (0, video.frames))
+ # Validate frame range
+ if frame_range[0] < 0 or frame_range[1] > video.frames:
+ raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]")
+ if frame_range[0] >= frame_range[1]:
+ raise ValueError(f"Invalid frame range: {frame_range}")
+
# Check if clip is selected, raise error if no clip selected
if frame_range == (0, video.frames) or frame_range == (0, 1):
raise ValueError("No valid clip frame range selected! Please select a valid frame range using shift + click in the GUI.")Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/gui/test_commands.py (5)
12-12: Remove unused import.The
Nodeimport fromsleap.skeletonis not used in the code.-from sleap.skeleton import Node🧰 Tools
🪛 Ruff (0.8.2)
12-12:
sleap.skeleton.Nodeimported but unusedRemove unused import:
sleap.skeleton.Node(F401)
1074-1074: Document the purpose of the unused variable.The
expected_frame_countvariable is calculated but not used. Consider either removing it or adding a comment explaining its intended purpose for future test cases.🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Local variable
expected_frame_countis assigned to but never usedRemove assignment to unused variable
expected_frame_count(F841)
1035-1109: Improve test coverage and clarity.A few suggestions to enhance the test:
- The test case numbering skips Case 3 (jumps from Case 2 to Case 4).
- Consider adding assertions for:
- Video properties (fps, frame count) of the exported video
- Content verification of the exported video
- Make the test parameters more explicit by using named constants.
def test_ExportVideoClip_creates_files(tmpdir): """Test that ExportVideoClip creates a video clip, .slp, and .pkg.slp.""" + # Test parameters + TEST_FPS = 30 + TEST_FRAME_COUNT = 10 + TEST_FRAME_WIDTH = 100 + TEST_FRAME_HEIGHT = 100 + TEST_FRAME_RANGE = (0, 4) # Step 1: Generate a dummy video file using imageio video_path = Path(tmpdir, "mock_video.mp4") - fps = 30 - frame_count = 10 - width, height = 100, 100 + fps = TEST_FPS + frame_count = TEST_FRAME_COUNT + width, height = TEST_FRAME_WIDTH, TEST_FRAME_HEIGHT # ... rest of the setup ... # Assertions # Case 1: Assert exported video exists assert export_path.exists(), "Exported video file was not created." # Case 2: Assert .slp file exists slp_path = export_path.with_suffix(".slp") assert slp_path.exists(), ".slp file was not created." - # Case 4: Assert that the exported labels match the expected number of frames + # Case 3: Verify exported video properties + exported_video = Video(backend=MediaVideo(filename=str(export_path), grayscale=True)) + assert exported_video.fps == TEST_FPS, f"Expected {TEST_FPS} fps, got {exported_video.fps}" + assert exported_video.num_frames == TEST_FRAME_RANGE[1] - TEST_FRAME_RANGE[0], \ + f"Expected {TEST_FRAME_RANGE[1] - TEST_FRAME_RANGE[0]} frames" + + # Case 4: Assert that the exported labels match the expected number of frames exported_labels = Labels.load_file(str(slp_path))🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Local variable
expected_frame_countis assigned to but never usedRemove assignment to unused variable
expected_frame_count(F841)
1111-1195: Enhance test robustness and readability.Consider the following improvements:
- Use more descriptive variable names (e.g.,
subset_frame_rangeinstead of justsubset_start, subset_end)- Add edge cases:
- Empty frame range
- Single frame range
- Invalid frame range (end < start)
- Organize test parameters at the top for better maintainability
def test_ExportVideoClip_frame_and_video_list_sizes(tmpdir): """Test that ExportVideoClip exports correct length labeled frames and video lists with a subset range.""" + # Test parameters + TEST_PARAMS = { + 'fps': 30, + 'total_frames': 10, + 'frame_range': (2, 7), + 'dimensions': (100, 100), + } # Generate a dummy video file using imageio video_path = Path(tmpdir, "mock_video.mp4") - fps = 30 - total_frames = 10 - subset_start, subset_end = 2, 7 - subset_frame_count = subset_end - subset_start - width, height = 100, 100 + fps = TEST_PARAMS['fps'] + total_frames = TEST_PARAMS['total_frames'] + subset_frame_range = TEST_PARAMS['frame_range'] + subset_frame_count = subset_frame_range[1] - subset_frame_range[0] + width, height = TEST_PARAMS['dimensions'] # ... rest of the test ... + # Additional edge cases + # Test empty frame range + with pytest.raises(ValueError): + context.state["frame_range"] = (5, 5) + ExportClipVideo.do_action(context, params) + + # Test invalid frame range + with pytest.raises(ValueError): + context.state["frame_range"] = (7, 2) + ExportClipVideo.do_action(context, params)
1196-1272: Enhance error handling and validation coverage.Consider the following improvements:
- Add more error cases:
- Frame range exceeding video length
- Non-sequential frame range
- Validate package contents:
- Verify skeleton structure
- Check track information
- Use consistent naming with other test methods for better maintainability
def test_ExportClipPkg_creates_pkg_file(tmpdir): """Test that ExportClipPkg creates a .pkg.slp file with selected frame range.""" + # Test parameters + TEST_PARAMS = { + 'fps': 30, + 'frame_count': 10, + 'dimensions': (100, 100), + 'frame_range': (3, 8), + } # ... test setup ... # Case 4: Test the error when no valid frame range is selected context.state["frame_range"] = (0, 10) with pytest.raises(ValueError, match="No valid clip frame range selected!"): ExportClipPkg.do_action(context, params) + + # Case 5: Test error when frame range exceeds video length + context.state["frame_range"] = (5, 15) + with pytest.raises(ValueError, match="Frame range exceeds video length"): + ExportClipPkg.do_action(context, params) + + # Case 6: Validate package contents + exported_labels = Labels.load_file(str(pkg_path)) + # Verify skeleton structure + assert len(exported_labels.skeletons) == 1 + assert len(exported_labels.skeleton.nodes) == 2 + assert exported_labels.skeleton.node_names == ["node1", "node2"] + # Verify track information + for lf in exported_labels.labeled_frames: + assert len(lf.instances) == 1 + assert lf.instances[0].track is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/gui/test_commands.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/gui/test_commands.py
12-12: sleap.skeleton.Node imported but unused
Remove unused import: sleap.skeleton.Node
(F401)
1074-1074: Local variable expected_frame_count is assigned to but never used
Remove assignment to unused variable expected_frame_count
(F841)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
tests/gui/test_commands.py (6)
12-12: Remove unused import.The
Nodeimport is not used in the code.-from sleap.skeleton import Node🧰 Tools
🪛 Ruff (0.8.2)
12-12:
sleap.skeleton.Nodeimported but unusedRemove unused import:
sleap.skeleton.Node(F401)
1074-1074: Remove unused variable.The
expected_frame_countvariable is assigned but never used.- expected_frame_count = partial_frame_range[1] - partial_frame_range[0] # 4 frames🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Local variable
expected_frame_countis assigned to but never usedRemove assignment to unused variable
expected_frame_count(F841)
1035-1110: Add error handling for video creation.The test should handle potential errors during video creation and cleanup resources properly.
# Step 1: Generate a dummy video file using imageio video_path = Path(tmpdir, "mock_video.mp4") + writer = None try: - with get_writer(video_path, fps=fps, codec="libx264", format="FFMPEG") as writer: - for _ in range(frame_count): - frame = np.zeros((height, width), dtype=np.uint8) # Black frame - writer.append_data(frame) + writer = get_writer(video_path, fps=fps, codec="libx264", format="FFMPEG") + for _ in range(frame_count): + frame = np.zeros((height, width), dtype=np.uint8) # Black frame + writer.append_data(frame) + except Exception as e: + if writer is not None: + writer.close() + raise RuntimeError(f"Failed to create test video: {str(e)}") from e + finally: + if writer is not None: + writer.close()🧰 Tools
🪛 Ruff (0.8.2)
1074-1074: Local variable
expected_frame_countis assigned to but never usedRemove assignment to unused variable
expected_frame_count(F841)
1196-1202: Improve error messages in parameterized test cases.The test IDs could be more descriptive to better explain the test scenarios.
@pytest.mark.parametrize("subset, should_raise_error", [ - pytest.param((0, 0), True, id="single_frame"), # Single frame - pytest.param((9, 9), True, id="last_frame"), # Last frame - pytest.param((0, 1), True, id="invalid_minimal_range"), # Invalid minimal range - pytest.param((0, 10), True, id="full_range"), # Full range - pytest.param((3, 7), False, id="valid_partial_range"), # Valid subset + pytest.param((0, 0), True, id="single_frame_should_fail"), + pytest.param((9, 9), True, id="last_frame_should_fail"), + pytest.param((0, 1), True, id="invalid_minimal_range_should_fail"), + pytest.param((0, 10), True, id="full_range_should_fail"), + pytest.param((3, 7), False, id="valid_partial_range_should_succeed"), ])
1284-1361: Add cleanup for created files.The test should clean up created files even if assertions fail.
+ pkg_path = None try: # Step 3: Call ExportClipPkg ExportClipPkg.do_action(context, params) + pkg_path = Path(params["filename"]) # Step 4: Assertions # Case 1: Assert .pkg.slp file exists - assert pkg_path.exists(), ".pkg.slp file was not created." + assert pkg_path.exists(), f"Expected package file not created at {pkg_path}" # Case 2: Load exported labels and check frame count exported_labels = Labels.load_file(str(pkg_path)) assert len(exported_labels.labeled_frames) == expected_frame_count + finally: + if pkg_path is not None and pkg_path.exists(): + pkg_path.unlink()
1362-1368: Improve error messages and add cleanup in edge case tests.Similar to the video edge cases, improve test IDs and add cleanup.
@pytest.mark.parametrize("subset, should_raise_error", [ - pytest.param((0, 0), True, id="single_frame"), - pytest.param((9, 9), True, id="later_frame"), - pytest.param((0, 1), True, id="invalid_minimal_range"), - pytest.param((0, 10), True, id="full_range"), - pytest.param((12, 13), True, id="outside_range"), + pytest.param((0, 0), True, id="single_frame_should_fail_pkg_export"), + pytest.param((9, 9), True, id="later_frame_should_fail_pkg_export"), + pytest.param((0, 1), True, id="invalid_minimal_range_should_fail_pkg_export"), + pytest.param((0, 10), True, id="full_range_should_fail_pkg_export"), + pytest.param((12, 13), True, id="outside_range_should_fail_pkg_export"), ])sleap/gui/commands.py (1)
3523-3523: Remove unused variable assignment.The
appvariable is assigned but never used.- app = QtWidgets.QApplication.instance() or QtWidgets.QApplication([])🧰 Tools
🪛 Ruff (0.8.2)
3523-3523: Local variable
appis assigned to but never usedRemove assignment to unused variable
app(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/commands.py(4 hunks)tests/gui/test_commands.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/gui/test_commands.py
12-12: sleap.skeleton.Node imported but unused
Remove unused import: sleap.skeleton.Node
(F401)
1074-1074: Local variable expected_frame_count is assigned to but never used
Remove assignment to unused variable expected_frame_count
(F841)
sleap/gui/commands.py
3523-3523: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
3547-3547: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3576-3576: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3659-3659: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3660-3660: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
sleap/gui/dialogs/export_clip.py (1)
33-81: Consider adding documentation for FPS range and validation.The implementation looks good, but consider:
- Adding a docstring explaining the FPS range (1-240) and why these limits were chosen
- Adding validation for the video_fps parameter to ensure it's within the valid range
class ExportClipAndLabelsDialog(FormBuilderModalDialog): + """Dialog for exporting clips with labels. + + Args: + video_fps: Default FPS value from the video metadata (default: 30). + Will be clamped to the valid range of 1-240 FPS. + """ def __init__(self, video_fps=30): from sleap.io.videowriter import VideoWriter # Initialize with a blank widget (no YAML needed) super().__init__(form_widget=QtWidgets.QWidget()) self.setWindowTitle("Export Clip Options") # FPS Field self.fps_input = QtWidgets.QSpinBox() self.fps_input.setRange(1, 240) # Common range for most video formats + video_fps = max(1, min(240, video_fps)) # Clamp to valid range self.fps_input.setValue(video_fps) # Set default FPS from videosleap/gui/commands.py (1)
3522-3531: Remove unused variable and improve progress bar initialization.The
appvariable is assigned but never used. Also, the progress bar initialization could be simplified.- show_progress = os.getenv("PYTEST_RUNNING") != "1" - if show_progress: - app = QtWidgets.QApplication.instance() or QtWidgets.QApplication([]) - progress = QtWidgets.QProgressDialog("Exporting video...", "Cancel", 0, len(range(*frame_range))) - progress.setWindowModality(QtCore.Qt.WindowModal) - progress.setValue(0) - else: - progress = None # Progress bar disabled during tests + show_progress = os.getenv("PYTEST_RUNNING") != "1" + progress = (QtWidgets.QProgressDialog("Exporting video...", "Cancel", 0, len(range(*frame_range))) + if show_progress else None) + if progress is not None: + progress.setWindowModality(QtCore.Qt.WindowModal)🧰 Tools
🪛 Ruff (0.8.2)
3525-3525: Local variable
appis assigned to but never usedRemove assignment to unused variable
app(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/commands.py(4 hunks)sleap/gui/dialogs/export_clip.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
sleap/gui/commands.py (1)
Learnt from: ericleonardis
PR: talmolab/sleap#2059
File: sleap/gui/commands.py:0-0
Timestamp: 2024-12-17T23:07:06.722Z
Learning: In `sleap/gui/commands.py`, the `save_labeled_video` function is no longer used for exporting clips; the `Labels` class is now used instead.
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3525-3525: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
3549-3549: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3578-3578: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3661-3661: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3662-3662: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
sleap/gui/dialogs/export_clip.py (1)
Line range hint 9-31: LGTM!
The ExportClipDialog class correctly handles the video encoding message based on ffmpeg availability.
sleap/gui/commands.py (1)
3484-3486: LGTM!
The class and method signatures are well-defined with clear documentation.
Also applies to: 3487-3493
sleap/gui/commands.py
Outdated
| # Ensure frame range is set; default to all frames if None | ||
| frame_range = context.state.get("frame_range", (0, video.frames)) | ||
| # Validate frame range | ||
| if frame_range[0] < 0 or frame_range[1] > video.frames: | ||
| raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]") | ||
| # Check if clip is selected, raise error if no clip selected | ||
| if frame_range == (0, video.frames) or frame_range == (0, 1) or frame_range[0] == frame_range[1]: | ||
| raise ValueError("No valid clip frame range selected! Please select a valid frame range using shift + click in the GUI.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting frame range validation.
The frame range validation logic is duplicated between ExportClipVideo and ExportClipPkg. Consider extracting it into a shared helper method.
@staticmethod
def _validate_frame_range(frame_range: tuple, video) -> None:
"""Validates the frame range for clip export.
Args:
frame_range: Tuple of (start, end) frame indices
video: Video object containing the frames
Raises:
ValueError: If frame range is invalid
"""
if frame_range[0] < 0 or frame_range[1] > video.frames:
raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]")
if frame_range == (0, video.frames) or frame_range == (0, 1) or frame_range[0] == frame_range[1]:
raise ValueError("No valid clip frame range selected! Please select a valid frame range using shift + click in the GUI.")| except Exception as e: | ||
| writer.close() | ||
| os.remove(params["filename"]) | ||
| raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with proper exception chaining.
Use exception chaining to preserve the original error context.
except Exception as e:
writer.close()
os.remove(params["filename"])
- raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}")
+ raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") from e📝 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.
| except Exception as e: | |
| writer.close() | |
| os.remove(params["filename"]) | |
| raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") | |
| except Exception as e: | |
| writer.close() | |
| os.remove(params["filename"]) | |
| raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
3549-3549: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| try: | ||
| pkg_filename = params["filename"] | ||
| pruned_labels.save(pkg_filename, with_images=True) | ||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in package save operation.
- Use proper exception chaining
- Include the original error message
- Clean up the file on failure
try:
pkg_filename = params["filename"]
pruned_labels.save(pkg_filename, with_images=True)
except Exception as e:
- raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}.")
+ if os.path.exists(pkg_filename):
+ os.remove(pkg_filename)
+ raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}: {str(e)}") from e📝 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.
| try: | |
| pkg_filename = params["filename"] | |
| pruned_labels.save(pkg_filename, with_images=True) | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}.") | |
| try: | |
| pkg_filename = params["filename"] | |
| pruned_labels.save(pkg_filename, with_images=True) | |
| except Exception as e: | |
| if os.path.exists(pkg_filename): | |
| os.remove(pkg_filename) | |
| raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
3661-3661: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3662-3662: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
The indexing into the clip range assumes that
- the
LabeledFrames were added to theLabels.labelsin the same order as theLabeledFrame.frame_idx - the
Labelsproject has aLabeledFramefor every frame index in the video - there is only a single video in the project
sleap/gui/app.py
Outdated
| add_menu_item( | ||
| labelMenu, | ||
| "Extract clip and labels", | ||
| "Extract Clip and Labels...", | ||
| self.commands.exportClipVideo, | ||
| ) | ||
|
|
||
| add_menu_item( | ||
| labelMenu, | ||
| "extract clip labels package", | ||
| "Extract Clip Labels Package...", | ||
| self.commands.exportClipPkg, | ||
| ) |
There was a problem hiding this comment.
Lets enable/disable these based on whether a clip is selected in MainWindow._update_gui_state (which is called periodically while the GUI is open). For example:
Line 1129 in 1eff33d
There was a problem hiding this comment.
Great suggestion! I have now changed these menu items to be enabled/disabled based on whether a clip is selected.
sleap/gui/commands.py
Outdated
|
|
||
| # Extract only the selected frames into a new Labels object | ||
| pruned_labels = labels.extract( | ||
| inds=range(*frame_range), |
There was a problem hiding this comment.
When inds is either a list or range, then we just call Labels.get for each item in the list or range (int in our case).
This means that we try to grab the item from the Labels.labels: List[LabeledFrame] at the index we provided - which is not the same as grabbing the item at the frame index.
There was a problem hiding this comment.
Ok, I think I have addressed this suggestion by creating a frame_to_index mapping and then feeding in valid frame indices on line 3666-3677 in command.py.
`
Map frame indices to the actual labeled frame objects
frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video}
valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index]
# Extract only the selected frames into a new Labels object
pruned_labels = labels.extract(
inds=[frame_to_index[frame] for frame in valid_frame_indices],
copy=True # Ensures a deep copy of the extracted labels
)
`
This should now be robust to multiple videos and sparsely labelled videos.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/gui/test_dialogs.py (1)
147-173: Remove debug print statement.The debug print statement on line 165 should be removed as it's not needed for the test.
- print("Extracted QLabel texts:", label_texts)sleap/gui/commands.py (2)
3484-3643: Improve error handling and remove unused variable.
- Add proper exception chaining to preserve error context
- Remove unused
appvariable- raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") + raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") from e - app = QtWidgets.QApplication.instance() or QtWidgets.QApplication([]) + QtWidgets.QApplication.instance() or QtWidgets.QApplication([])The implementation is otherwise solid with good progress feedback and frame range validation.
🧰 Tools
🪛 Ruff (0.8.2)
3542-3542: Local variable
appis assigned to but never usedRemove assignment to unused variable
app(F841)
3566-3566: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3595-3595: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3645-3710: Extract shared frame range validation logic.The frame range validation logic is duplicated between
ExportClipVideoandExportClipPkg. Consider extracting it to a shared helper method.@staticmethod def _validate_frame_range(frame_range: tuple, video) -> None: """Validates frame range for clip export. Args: frame_range: Tuple of (start, end) frame indices video: Video object containing frames Raises: ValueError: If frame range is invalid """ if frame_range[0] < 0 or frame_range[1] > video.frames: raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]") if frame_range == (0, video.frames) or frame_range == (0, 1) or frame_range[0] == frame_range[1]: raise ValueError("No valid clip frame range selected!")Also improve error handling:
try: pkg_filename = params["filename"] pruned_labels.save(pkg_filename, with_images=True) except Exception as e: - raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}.") + raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
3686-3686: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
3687-3687: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sleap/gui/commands.py(4 hunks)tests/gui/test_commands.py(4 hunks)tests/gui/test_dialogs.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
sleap/gui/commands.py (2)
Learnt from: ericleonardis
PR: talmolab/sleap#2059
File: sleap/gui/commands.py:0-0
Timestamp: 2024-12-17T23:07:06.722Z
Learning: In `sleap/gui/commands.py`, the `save_labeled_video` function is no longer used for exporting clips; the `Labels` class is now used instead.
Learnt from: ericleonardis
PR: talmolab/sleap#2059
File: sleap/gui/commands.py:0-0
Timestamp: 2024-12-17T22:56:56.102Z
Learning: In `sleap/gui/commands.py`, for the `ExportClipPkg.do_action` method, adding progress feedback for the package save operation is unnecessary, as the current pkg writer and slp savers do not include progress feedback.
🪛 Ruff (0.8.2)
tests/gui/test_dialogs.py
4-4: importlib imported but unused
Remove unused import: importlib
(F401)
5-5: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
12-12: sleap imported but unused
Remove unused import: sleap
(F401)
19-19: sleap.gui.dialogs.export_clip imported but unused
Remove unused import: sleap.gui.dialogs.export_clip
(F401)
20-20: sleap.gui.app.MainWindow imported but unused
Remove unused import: sleap.gui.app.MainWindow
(F401)
tests/gui/test_commands.py
12-12: sleap.skeleton.Node imported but unused
Remove unused import: sleap.skeleton.Node
(F401)
1262-1262: f-string without any placeholders
Remove extraneous f prefix
(F541)
1311-1311: f-string without any placeholders
Remove extraneous f prefix
(F541)
1367-1367: f-string without any placeholders
Remove extraneous f prefix
(F541)
sleap/gui/commands.py
3542-3542: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
3566-3566: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3595-3595: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3686-3686: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3687-3687: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
tests/gui/test_dialogs.py (3)
122-141: LGTM! Test verifies ffmpeg availability message.
The test properly verifies that the dialog displays the correct message when ffmpeg is available.
143-145: LGTM! Clean utility function implementation.
The utility function is well-documented and correctly implements HTML tag stripping.
175-240: LGTM! Comprehensive dialog testing.
The test suite thoroughly covers dialog initialization, form handling, and acceptance behavior.
tests/gui/test_commands.py (3)
1035-1113: LGTM! Thorough test coverage for clip export.
The test comprehensively verifies:
- Video clip creation
- SLP file generation
- Frame indices in exported labels
- Video filename in exported labels
1114-1190: LGTM! Good test coverage for frame range handling.
The test thoroughly verifies frame range selection and video list handling in exports.
1192-1263: LGTM! Comprehensive edge case testing.
The parameterized test effectively covers various frame range scenarios and verifies proper error handling.
🧰 Tools
🪛 Ruff (0.8.2)
1262-1262: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/app.py (1)
749-764: Consider adding progress feedback for large clips.For better user experience, consider:
- Adding progress feedback during clip extraction
- Implementing cancellation support for long-running operations
- Adding error handling for cases like insufficient disk space
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/app.py(3 hunks)
🔇 Additional comments (2)
sleap/gui/app.py (2)
749-764: LGTM: Menu items properly implemented.
The new menu items for clip extraction are well-integrated into the Labels menu, following the established patterns and conventions.
1137-1140: LGTM: Menu items state properly managed.
The menu items are correctly enabled/disabled based on frame range selection, ensuring a good user experience.
sleap/gui/commands.py
Outdated
| raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]") | ||
|
|
||
| # Check if clip is selected, raise error if no clip selected | ||
| if frame_range == (0, video.frames) or frame_range == (0, 1) or frame_range[0] == frame_range[1]: |
There was a problem hiding this comment.
Why is frame_range == (0, video.frames) not a valid choice?
There was a problem hiding this comment.
I guess I thought a "clip" meant it wasn't the whole video, but that's just semantics. I went ahead and made that a valid choice.
Unfortunately this has lead to another discovery which has lead me to open a new issue. When I do select the maximum frame range from (0, video.frames) the GUI the selection and frame count display does not match the video. I have opened a new issue at #2074 to describe the mismatch between the video frames and the Shift + Click frame selection tool on the Timeline viewer.
When I select the full clip range I get the ValueError: Frame range (0, 1505) is outside video bounds [0, 1500]. So this PR depends on Issue #2074 being fixed before we can move forward with this one.
| # Map frame indices to the actual labeled frame objects | ||
| frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video} | ||
| valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index] | ||
|
|
||
| if not valid_frame_indices: | ||
| raise ValueError("No valid labeled frames found in the selected frame range.") | ||
|
|
||
| # Extract only the selected frames into a new Labels object | ||
| pruned_labels = labels.extract( | ||
| inds=[frame_to_index[frame] for frame in valid_frame_indices], | ||
| copy=True # Ensures a deep copy of the extracted labels | ||
| ) |
There was a problem hiding this comment.
| # Map frame indices to the actual labeled frame objects | |
| frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video} | |
| valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index] | |
| if not valid_frame_indices: | |
| raise ValueError("No valid labeled frames found in the selected frame range.") | |
| # Extract only the selected frames into a new Labels object | |
| pruned_labels = labels.extract( | |
| inds=[frame_to_index[frame] for frame in valid_frame_indices], | |
| copy=True # Ensures a deep copy of the extracted labels | |
| ) | |
| inds = (video, range(frame_range)) | |
| # Extract only the selected frames into a new Labels object | |
| pruned_labels = labels.extract( | |
| inds=inds, | |
| copy=True # Ensures a deep copy of the extracted labels | |
| ) |
since we want to run into this case in Labels.extract -> Labels.__getitem__ -> Labels.get:
Lines 763 to 764 in 66d96ce
which will call this case of Labels.find -> Labels._cache.find_frames:
Lines 142 to 147 in 66d96ce
| clipboard.clear(mode=clipboard.Clipboard) | ||
| clipboard.setText(text, mode=clipboard.Clipboard) | ||
| # Ensure frame range is set; default to all frames if None | ||
| frame_range = context.state.get("frame_range", (0, video.frames)) |
There was a problem hiding this comment.
Maybe we say
| frame_range = context.state.get("frame_range", (0, video.frames)) | |
| frame_range = context.state.get("frame_range", None) |
and raise the no valid clip range error below if frame_range is None?
|
|
||
| # Create a new Video object for the output video | ||
| new_media_video = MediaVideo( | ||
| filename=params["filename"], grayscale=video.channels == 1, bgr=True | ||
| ) |
There was a problem hiding this comment.
| # Create a new Video object for the output video | |
| new_media_video = MediaVideo( | |
| filename=params["filename"], grayscale=video.channels == 1, bgr=True | |
| ) | |
| # Create a new Video object for the output video | |
| new_media_video = MediaVideo( | |
| filename=params["filename"], grayscale=video.grayscale, bgr=video.bgr | |
| ) |
| # Save the pruned labels | ||
| labels_filename = params["filename"].replace(".mp4", ".slp") |
There was a problem hiding this comment.
Use pathlib for path operations
| # Save the pruned labels | |
| labels_filename = params["filename"].replace(".mp4", ".slp") | |
| # Save the pruned labels | |
| labels_filename = str(Path(params["filename"]).with_suffix(".slp")) |
| # Map frame indices to the actual labeled frame objects | ||
| frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video} | ||
| valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index] | ||
|
|
||
| if not valid_frame_indices: | ||
| raise ValueError("No valid labeled frames found in the selected frame range.") | ||
|
|
||
| # Extract only the selected frames into a new Labels object | ||
| pruned_labels = labels.extract( | ||
| inds=[frame_to_index[frame] for frame in valid_frame_indices], | ||
| copy=True # Ensures a deep copy of the extracted labels | ||
| ) | ||
|
|
||
| # Remap frame indices in pruned_labels to start from 0 | ||
| for labeled_frame in pruned_labels.labeled_frames: | ||
| labeled_frame.frame_idx -= frame_range[0] |
There was a problem hiding this comment.
A lot of this do_action is the same as the ExportClipVideo class... do you think there is a way we can maybe break out the subparts of do_action into class methods and either combine the ExportClipVideo and ExportClipPkg classes into one class OR create create a base class and subclass to re-use code?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
sleap/gui/commands.py (5)
3509-3514: Improve frame range validation.
- The error message for
(0, video.frames)is confusing as it's a valid range.- Consider using more specific error types for different validation failures.
- if frame_range == (0, 1) or frame_range[0] == frame_range[1]: - raise ValueError("No valid clip frame range selected! Please select a valid frame range using shift + click in the GUI.") + if frame_range[0] >= frame_range[1]: + raise ValueError("Invalid frame range: end frame must be greater than start frame.") + if frame_range[1] - frame_range[0] < 2: + raise ValueError("Selected range is too short. Please select at least 2 frames.")
3542-3542: Remove unused variable assignment.The
appvariable is assigned but never used.- app = QtWidgets.QApplication.instance() or QtWidgets.QApplication([]) + QtWidgets.QApplication.instance() or QtWidgets.QApplication([])🧰 Tools
🪛 Ruff (0.8.2)
3542-3542: Local variable
appis assigned to but never usedRemove assignment to unused variable
app(F841)
3563-3566: Add proper exception chaining.Use exception chaining to preserve the original error context.
except Exception as e: writer.close() os.remove(params["filename"]) - raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") + raise RuntimeError(f"Failed to write frame {frame_idx}: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
3566-3566: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3590-3591: Use pathlib for path operations.Use pathlib for more robust path handling.
- labels_filename = params["filename"].replace(".mp4", ".slp") + labels_filename = str(Path(params["filename"]).with_suffix(".slp"))
3683-3687: Improve error handling in package save operation.Include the original error message and use proper exception chaining.
try: pkg_filename = params["filename"] pruned_labels.save(pkg_filename, with_images=True) except Exception as e: - raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}.") + if os.path.exists(pkg_filename): + os.remove(pkg_filename) + raise RuntimeError(f"Failed to save labels pkg to {pkg_filename}: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
3686-3686: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
3687-3687: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/commands.py(4 hunks)
🧰 Additional context used
📓 Learnings (1)
sleap/gui/commands.py (2)
Learnt from: ericleonardis
PR: talmolab/sleap#2059
File: sleap/gui/commands.py:0-0
Timestamp: 2024-12-17T23:07:06.722Z
Learning: In `sleap/gui/commands.py`, the `save_labeled_video` function is no longer used for exporting clips; the `Labels` class is now used instead.
Learnt from: ericleonardis
PR: talmolab/sleap#2059
File: sleap/gui/commands.py:0-0
Timestamp: 2024-12-17T22:56:56.102Z
Learning: In `sleap/gui/commands.py`, for the `ExportClipPkg.do_action` method, adding progress feedback for the package save operation is unnecessary, as the current pkg writer and slp savers do not include progress feedback.
🪛 Ruff (0.8.2)
sleap/gui/commands.py
3542-3542: Local variable app is assigned to but never used
Remove assignment to unused variable app
(F841)
3566-3566: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3595-3595: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
3686-3686: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
3687-3687: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
sleap/gui/commands.py (1)
3601-3642: LGTM! Dialog implementation is clean and follows Qt patterns.
The dialog implementation correctly handles:
- User input validation
- Default FPS from video metadata
- File selection
- Parameter population
| class ExportClipVideo(AppCommand): | ||
| @staticmethod |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a base class for clip export commands.
There's significant code duplication between ExportClipVideo and ExportClipPkg, particularly in frame range validation and labels extraction logic. Consider creating a base class to share common functionality.
class BaseClipExportCommand(AppCommand):
@staticmethod
def _validate_frame_range(frame_range: tuple, video) -> None:
"""Validates the frame range for clip export."""
if frame_range[0] < 0 or frame_range[1] > video.frames:
raise ValueError(f"Frame range {frame_range} is outside video bounds [0, {video.frames}]")
if frame_range == (0, 1) or frame_range[0] == frame_range[1]:
raise ValueError("No valid clip frame range selected! Please select a valid frame range using shift + click in the GUI.")
@staticmethod
def _extract_labeled_frames(video, labels, frame_range: tuple) -> Labels:
"""Extracts and remaps labeled frames for the given range."""
frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video}
valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index]
if not valid_frame_indices:
raise ValueError("No valid labeled frames found in the selected frame range.")
pruned_labels = labels.extract(
inds=[frame_to_index[frame] for frame in valid_frame_indices],
copy=True
)
# Remap frame indices
frame_offset = frame_range[0]
for labeled_frame in pruned_labels.labeled_frames:
labeled_frame.frame_idx -= frame_offset
return pruned_labelsAlso applies to: 3645-3646
| # Map frame indices to the actual labeled frame objects | ||
| frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video} | ||
| valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index] | ||
|
|
||
| if not valid_frame_indices: | ||
| raise ValueError("No valid labeled frames found in the selected frame range.") | ||
|
|
||
| # Extract only the selected frames into a new Labels object | ||
| pruned_labels = labels.extract( | ||
| inds=[frame_to_index[frame] for frame in valid_frame_indices], | ||
| copy=True # Ensures a deep copy of the extracted labels | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revise labels extraction implementation.
The current implementation might not handle frame indices correctly. Consider using the suggested approach from the codebase maintainer.
- # Map frame indices to the actual labeled frame objects
- frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video}
- valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index]
-
- # Extract only the selected frames into a new Labels object
- pruned_labels = labels.extract(
- inds=[frame_to_index[frame] for frame in valid_frame_indices],
- copy=True # Ensures a deep copy of the extracted labels
- )
+ # Extract frames using video and range
+ inds = (video, range(*frame_range))
+ pruned_labels = labels.extract(
+ inds=inds,
+ copy=True # Ensures a deep copy of the extracted labels
+ )📝 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.
| # Map frame indices to the actual labeled frame objects | |
| frame_to_index = {lf.frame_idx: idx for idx, lf in enumerate(labels.labeled_frames) if lf.video == video} | |
| valid_frame_indices = [frame for frame in range(*frame_range) if frame in frame_to_index] | |
| if not valid_frame_indices: | |
| raise ValueError("No valid labeled frames found in the selected frame range.") | |
| # Extract only the selected frames into a new Labels object | |
| pruned_labels = labels.extract( | |
| inds=[frame_to_index[frame] for frame in valid_frame_indices], | |
| copy=True # Ensures a deep copy of the extracted labels | |
| ) | |
| # Extract frames using video and range | |
| inds = (video, range(*frame_range)) | |
| pruned_labels = labels.extract( | |
| inds=inds, | |
| copy=True # Ensures a deep copy of the extracted labels | |
| ) |
Description
This is a new enhancement for an extract clip feature which was proposed in Discussion #1984.
Two new menu items been added to the Labels menu called "Extract Clip and Labels...." which returns a video of the selected clip and slp file and "Extract Clip Labels Package" extracts the clip and saves it as a labels pkg.slp file. I created a new ExportClipAndLabelsDialogue which asks the user for the FPS for the video they are rendering the clip for and saving the slp and pkg.slp files with a reset frame index.
I also wrote a test which checks to see if the mp4, slp, and pkg.slp files are made. I have also written a test which checks to see if the frame numbers and video numbers in the exported labels object are correct.
I have not yet tested this on a dataset of just images, but the slp and pkg.slp look good in the GUI.
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests