Add option to group cameras from sessions #2125
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## liezl/add-gui-elements-for-sessions #2125 +/- ##
=======================================================================
- Coverage 74.18% 73.84% -0.35%
=======================================================================
Files 135 135
Lines 25450 25688 +238
=======================================================================
+ Hits 18881 18969 +88
- Misses 6569 6719 +150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…com/talmolab/sleap into justin/group-cameras-from-sessions
…dd a camera to a camera group
SetCameraGroupName, AddCameraToGroup, and RemoveCameraFromGroup for creating, deleting, and managing camera groups
roomrys
requested changes
Mar 4, 2025
There was a problem hiding this comment.
A few things:
- If a method does not exist, instead of calling the method that doesn't exist, raise a
NotImplementedErrorand add aTODO(JS). - Also, can we see some screenshots of the GUI elements in the PR.
- I would also like to see tests that hits each one of the methods you've created.
sleap/gui/dataviews.py
Outdated
Comment on lines
718
to
719
| # Mark project as changed | ||
| self.context.changestack_push("rename camera group") |
There was a problem hiding this comment.
While we want to call a self.context method here, this is not the one (we haven't created the method we want to call yet).
Suggested change
| # Mark project as changed | |
| self.context.changestack_push("rename camera group") | |
| # TODO(JS): Add `CommandContext` method that updates the camera group name | |
| raise NotImplementedError |
sleap/gui/dataviews.py
Outdated
Comment on lines
693
to
701
| # Register for updates | ||
| if context and hasattr(context, 'state'): | ||
| context.state.connect("camera_groups", self.update_items) | ||
|
|
||
| def update_items(self, camera_groups): | ||
| """Update the model when camera groups change.""" | ||
| self.items = camera_groups | ||
| self.beginResetModel() | ||
| self.endResetModel() |
There was a problem hiding this comment.
Oh man, yeah I do kinda like this better than what we currently do:
Lines 1216 to 1220 in 6531447
. Let's leave it for now (and possibly open a new PR to refactor all the other table updates).
sleap/gui/widgets/docks.py
Outdated
Comment on lines
642
to
648
| if not camera: | ||
| QMessageBox.information( | ||
| self.main_window, | ||
| "No Camera Selected", | ||
| "Please select a camera to add to a group." | ||
| ) | ||
| return |
There was a problem hiding this comment.
Instead of raising an error message, maybe we can just disable the button so no one can click on it if there is no camera selected. See this example for the "Link Video" button:
Lines 1191 to 1195 in d898a84
.
Suggested change
| if not camera: | |
| QMessageBox.information( | |
| self.main_window, | |
| "No Camera Selected", | |
| "Please select a camera to add to a group." | |
| ) | |
| return |
sleap/gui/dataviews.py
Outdated
Comment on lines
691
to
692
| def __init__(self, items=None, context=None): | ||
| super().__init__(items=items, context=context) |
There was a problem hiding this comment.
Expose the property kwarg:
Suggested change
| def __init__(self, items=None, context=None): | |
| super().__init__(items=items, context=context) | |
| def __init__(self, items=None, properties=None, context=None): | |
| super().__init__(items=items, properties=properties, context=context) |
sleap/gui/widgets/docks.py
Outdated
| self.main_window, "New Camera Group", "Enter name for camera group:" | ||
| ) | ||
| if ok and name: | ||
| self.main_window.commands.addCameraGroup(name) |
There was a problem hiding this comment.
Suggested change
| self.main_window.commands.addCameraGroup(name) | |
| # TODO(JS): self.main_window.commands.addCameraGroup(name) | |
| raise NotImplementedError |
sleap/gui/widgets/docks.py
Outdated
| return | ||
|
|
||
| # Get available groups | ||
| if not self.main_window.state.get("camera_groups") or len(self.main_window.state["camera_groups"]) == 0: |
There was a problem hiding this comment.
GuiState has a __getitem__ method, so we at least won't run into an error if the key doesn't exist.
Lines 52 to 54 in 6531447
How about we use get, but with a default of an empty list:
Suggested change
| if not self.main_window.state.get("camera_groups") or len(self.main_window.state["camera_groups"]) == 0: | |
| if len(self.main_window.state.get("camera_groups", []) == 0: |
There was a problem hiding this comment.
ex:
In [20]: from sleap.gui.state import GuiState
In [21]: state = GuiState()
In [22]: state[3]
In [23]: state.get(3, [])
Out[23]: []
sleap/gui/widgets/docks.py
Outdated
Comment on lines
881
to
919
| def _add_camera_to_group(self): | ||
| """Add the selected camera to a group.""" | ||
| camera = self.main_window.state.get("camera") | ||
| if not camera: | ||
| QMessageBox.information( | ||
| self.main_window, | ||
| "No Camera Selected", | ||
| "Please select a camera to add to a group." | ||
| ) | ||
| return | ||
|
|
||
| # Get available groups | ||
| if not self.main_window.state.get("camera_groups") or len(self.main_window.state["camera_groups"]) == 0: | ||
| reply = QMessageBox.question( | ||
| self.main_window, | ||
| "No Groups", | ||
| "No camera groups exist. Would you like to create one?", | ||
| QMessageBox.Yes | QMessageBox.No, | ||
| QMessageBox.Yes | ||
| ) | ||
| if reply == QMessageBox.Yes: | ||
| self._create_camera_group() | ||
| return | ||
|
|
||
| # Show group selection dialog | ||
| group_names = [group.name for group in self.main_window.state["camera_groups"]] | ||
| selected_name, ok = QInputDialog.getItem( | ||
| self.main_window, | ||
| "Select Group", | ||
| "Choose a group to add the camera to:", | ||
| group_names, | ||
| 0, | ||
| False | ||
| ) | ||
|
|
||
| if ok and selected_name: | ||
| selected_idx = group_names.index(selected_name) | ||
| selected_group = self.main_window.state["camera_groups"][selected_idx] | ||
| self.main_window.commands.addCameraToGroup(camera, selected_group) |
There was a problem hiding this comment.
This entire method is a duplicate
Suggested change
| def _add_camera_to_group(self): | |
| """Add the selected camera to a group.""" | |
| camera = self.main_window.state.get("camera") | |
| if not camera: | |
| QMessageBox.information( | |
| self.main_window, | |
| "No Camera Selected", | |
| "Please select a camera to add to a group." | |
| ) | |
| return | |
| # Get available groups | |
| if not self.main_window.state.get("camera_groups") or len(self.main_window.state["camera_groups"]) == 0: | |
| reply = QMessageBox.question( | |
| self.main_window, | |
| "No Groups", | |
| "No camera groups exist. Would you like to create one?", | |
| QMessageBox.Yes | QMessageBox.No, | |
| QMessageBox.Yes | |
| ) | |
| if reply == QMessageBox.Yes: | |
| self._create_camera_group() | |
| return | |
| # Show group selection dialog | |
| group_names = [group.name for group in self.main_window.state["camera_groups"]] | |
| selected_name, ok = QInputDialog.getItem( | |
| self.main_window, | |
| "Select Group", | |
| "Choose a group to add the camera to:", | |
| group_names, | |
| 0, | |
| False | |
| ) | |
| if ok and selected_name: | |
| selected_idx = group_names.index(selected_name) | |
| selected_group = self.main_window.state["camera_groups"][selected_idx] | |
| self.main_window.commands.addCameraToGroup(camera, selected_group) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This new feature allows users to group different camera angles into distinct groups, where upon exporting a training set, the user cab export all frames from a specific group.
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️