(4a -> 4) Add menu to assign an Instance to an InstanceGroup
#1747
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update involves a significant overhaul focused on managing OpenCV versions and enhancing the functionality of the SLEAP application. It includes a hack to ensure compatibility with a specific OpenCV version across different platforms and scripts. Additionally, new features and improvements in the GUI, error handling, and data processing for camera and dataset management have been added, alongside updated testing frameworks to support these changes. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Actionable comments outside the diff hunks (37)
tests/conftest.py (1)
Line range hint
4-4: The importpytestqtis unused in this file.Consider removing the import if it's not needed to avoid unnecessary dependencies.
sleap/util.py (2)
Line range hint
96-96: Avoid using bareexceptstatements as they can catch unexpected exceptions and make debugging difficult. Specify the exception type.- except: + except Exception as e: + # Handle specific exception e
Line range hint
130-130: The variable namefieldin the loop shadows the import fromattrs. Consider renaming the variable to avoid confusion and potential bugs.- for field in attr.fields(cls): + for attr_field in attr.fields(cls):tests/gui/test_commands.py (4)
Line range hint
228-228: Avoid equality comparisons toTrue; useif okay:for truth checks.- if okay == True: + if okay:
Line range hint
361-361: The local variablelast_lf_frameis assigned but never used, which could indicate redundant or incomplete logic.
Line range hint
369-369: For boolean checks, directly use the condition without comparing toTrue.- if video.backend.grayscale == True: + if video.backend.grayscale:
Line range hint
529-529: This f-string does not contain any placeholders, which suggests it might be incorrectly formatted or unnecessary.tests/io/test_dataset.py (3)
Line range hint
692-692: The variablenew_labels_jsonis defined but not used.Consider removing the unused variable or using it if it was intended for further operations.
Line range hint
708-708: The variablenew_labels_jsonis defined but not used.Consider removing the unused variable or using it if it was intended for further operations.
Line range hint
1495-1495: The variabletrack_instsis defined but not used.Consider removing the unused variable or using it if it was intended for further operations.
sleap/io/video.py (6)
Line range hint
231-231: Avoid using a bareexceptstatement; specify the exception type to improve error handling.- except: + except Exception as e:
Line range hint
242-242: Avoid using a bareexceptstatement; specify the exception type to improve error handling.- except: + except Exception as e:
Line range hint
107-107: Useisinstance()for type checking instead of comparing types directly.- if type(self.filename) is str: + if isinstance(self.filename, str):
Line range hint
536-536: Useisinstance()for type checking instead of comparing types directly.- if type(self.filename) is str: + if isinstance(self.filename, str):
Line range hint
959-959: Remove unnecessary f-string as it does not contain any placeholders.- f"Cannot find a video file: {path}" + "Cannot find a video file: " + path
Line range hint
1487-1487: Undefined namesleapin the methodto_pipeline. Ensure that the necessary module is imported or available in the scope.+ import sleapsleap/instance.py (3)
Line range hint
89-89: Undefined nameselfused in method definition.- def numpy() -> np.ndarray: + def numpy(self) -> np.ndarray:The method
numpyis missing theselfparameter which is necessary for instance methods in Python classes. This should be added to avoid runtime errors.
Line range hint
411-411: Useisinstance()for type checks.- if from_predicted is not None and type(from_predicted) != PredictedInstance: + if from_predicted is not None and not isinstance(from_predicted, PredictedInstance):Replace type comparison with
isinstance()for better practice and to support polymorphism.
Line range hint
1631-1631: Undefined nameLabels.The name
Labelsis used but not defined in this scope. Ensure that it is imported or defined before use, or correct the name if it's a typo.sleap/gui/app.py (1)
Line range hint
264-264: The local variablefilenameis assigned but never used in the methoddragEnterEvent.- filename = event.mimeData().data(mime_format).data().decode()sleap/gui/widgets/video.py (3)
Line range hint
27-75: Move all module-level imports to the top of the file.+ from collections import deque + import atexit + import math + import time + from typing import Callable, List, Optional, Union + import numpy as np + import qimage2ndarray + from qtpy import QtCore, QtWidgets + from qtpy.QtCore import QLineF, QMarginsF, QPointF, QRectF, Qt + from qtpy.QtGui import ( + QBrush, + QColor, + QCursor, + QFont, + QImage, + QKeyEvent, + QKeySequence, + QMouseEvent, + QPainter, + QPainterPath, + QPen, + QPixmap, + QPolygonF, + QTransform, + ) + from qtpy.QtWidgets import ( + QApplication, + QGraphicsEllipseItem, + QGraphicsItem, + QGraphicsObject, + QGraphicsPolygonItem, + QGraphicsRectItem, + QGraphicsScene, + QGraphicsTextItem, + QGraphicsView, + QShortcut, + QVBoxLayout, + QWidget, + ) + import sleap + from sleap.gui.color import ColorManager + from sleap.gui.shortcuts import Shortcuts + from sleap.gui.state import GuiState + from sleap.gui.widgets.slider import VideoSlider + from sleap.instance import Instance, Point, PredictedInstance + from sleap.io.video import Video + from sleap.prefs import prefs + from sleap.skeleton import Node
Line range hint
306-306: Replace the bareexceptwith a more specific exception type to avoid catching unexpected exceptions.- except: + except Exception as e: + # Handle specific exceptions or log error + print(f"Error while loading frame: {e}")
Line range hint
1470-1470: The local variablein_boundsis assigned but never used. Consider removing it if it's unnecessary.- in_bounds = Truesleap/io/dataset.py (10)
Line range hint
61-61: Unused importh5py.Consider removing the unused import to clean up the code and improve readability.
Line range hint
69-69: Avoid using bareexcept.Using a bare
except:can catch unexpected exceptions and hide bugs. Specify the exception type to improve error handling.
Line range hint
952-952: Useisinstance()for type checks.Replace type comparison using
type()withisinstance()for a more robust and idiomatic check.- if type(inst) == PredictedInstance: + if isinstance(inst, PredictedInstance):
Line range hint
1465-1465: Useisinstance()for type checks.Replace type comparison using
type()withisinstance()for a more robust and idiomatic check.- if type(inst) == Instance: + if isinstance(inst, Instance):
Line range hint
2365-2365: Undefined nameglob.The
globmodule is used but not imported. Ensure to importglobfrom the standard library to fix the undefined name error.
Line range hint
2460-2460: Avoid equality comparisons toFalse.Use
if not ret:instead ofif ret == False:for a more Pythonic approach.- if ret == False: + if not ret:
Line range hint
2548-2548: Avoid equality comparisons toFalse.Use
if not ret:instead ofif ret == False:for a more Pythonic approach.- if ret == False: + if not ret:
Line range hint
2563-2563: Undefined namesleap.The
sleapmodule is referenced but not defined or imported in this scope. Ensure that it is correctly imported or defined to avoid runtime errors.
Line range hint
2653-2653: Useisinstance()for type checks.Replace type comparison using
type()withisinstance()for a more robust and idiomatic check.- if type(video) == int: + if isinstance(video, int):
Line range hint
2658-2658: F-string without placeholders.The f-string is used without any placeholders. This can be changed to a regular string if no formatting is needed.
- raise IndexError(f"There are no videos in this project. No points matrix to return.") + raise IndexError("There are no videos in this project. No points matrix to return.")sleap/gui/commands.py (4)
Line range hint
196-196: Undefined nameMainWindowused.- MainWindow + self.app.__class__() # Assuming this is intended to reference the main window class.
Line range hint
860-860: Local variablefile_diris assigned to but never used.Consider removing the unused variable to clean up the code.
Line range hint
2545-2545: f-string without any placeholders.- f"This video type {type(video.backend)} for video at index {idx} does not support grayscale yet." + "This video type does not support grayscale yet."
Line range hint
2877-2877: f-string without any placeholders.- f"Unable to load {filename}." + "Unable to load the file."
roomrys
changed the base branch from
develop
to
liezl/add-gui-elements-for-sessions
roomrys
changed the title
Instance to an InstanceGroup
roomrys
changed the title
Instance to an InstanceGroupInstance to an InstanceGroup
There was a problem hiding this comment.
Also, please move the _update_sessions_menu function to right above the _update_track_menu function. Thanks!
sleap/gui/app.py
Outdated
| add_menu_check_item( | ||
| sessionsMenu, "sync session", "Synchronize Session Settings" | ||
| ).setToolTip( | ||
| "If enabled, changes to the session settings will be synchronized across all instances." | ||
| ) | ||
| add_menu_item( | ||
| sessionsMenu, | ||
| "clear session", | ||
| "Clear Current Session", | ||
| self.commands.clearCurrentSession, | ||
| ) |
There was a problem hiding this comment.
Not sure what this is for?
| add_menu_check_item( | |
| sessionsMenu, "sync session", "Synchronize Session Settings" | |
| ).setToolTip( | |
| "If enabled, changes to the session settings will be synchronized across all instances." | |
| ) | |
| add_menu_item( | |
| sessionsMenu, | |
| "clear session", | |
| "Clear Current Session", | |
| self.commands.clearCurrentSession, | |
| ) |
There was a problem hiding this comment.
Please remove - unable to open GUI:
(sa0) λ sleap-label %ds-mview%
Saving config: C:\Users\TalmoLab/.sleap/1.3.3/preferences.yaml
Traceback (most recent call last):
File "C:\Users\TalmoLab\mambaforge\envs\sa0\Scripts\sleap-label-script.py", line 33, in <module>
sys.exit(load_entry_point('sleap', 'console_scripts', 'sleap-label')())
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 1800, in main
no_usage_data=args.no_usage_data,
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 175, in __init__
self._initialize_gui()
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 304, in _initialize_gui
self._create_menus()
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 817, in _create_menus
sessionsMenu, "sync session", "Synchronize Session Settings"
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 382, in add_menu_check_item
connect_check(key)
File "d:\social-leap-estimates-animal-poses\source\sleap\sleap\gui\app.py", line 376, in connect_check
self._menu_actions[key].setChecked(self.state[key])
TypeError: 'PySide2.QtWidgets.QAction.setChecked' called with wrong argument types:
PySide2.QtWidgets.QAction.setChecked(NoneType)
Supported signatures:
PySide2.QtWidgets.QAction.setChecked(bool)
…Group and FrameGroup classes necessary for testing
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
…-set-instance-group
…/talmolab/sleap into ramiz/add-gui-set-instance-group
…/talmolab/sleap into ramiz/add-gui-set-instance-group
There was a problem hiding this comment.
Thanks for the changes and for taking charge on adding the delete instance group menu! Heads up that I went ahead and updated your remote branch with the remote of the base branch (liezl/add-gui-elements-for-sessions), so make sure to git pull. To manually test, you can open the GUI from the command line with sleap-label and then File > Open > tests/data/cameras/minimal_session/min_session_user_labeled.slp which should not contain any FrameGroups or InstanceGroups... yet!
sleap/gui/app.py
Outdated
| def setup_menus(self): | ||
| self.inst_groups_delete_menu = QMenu("Delete Instance Group", self.sessionsMenu) | ||
| self.sessionsMenu.addMenu(self.inst_groups_delete_menu) | ||
|
|
||
| self.inst_groups_delete_menu.aboutToShow.connect(self.update_delete_instance_group_menu) |
There was a problem hiding this comment.
The function should be removed and its contents moved into the existing MainWindow._create_menus method. Also, do you mind following existing code style (i.e. use sessionsMenu.addMenu instead of QMenu).
sleap/gui/app.py
Outdated
| def update_delete_instance_group_menu(self): | ||
| self.inst_groups_delete_menu.clear() | ||
| session = self.state["session"] | ||
| frame_idx = self.state["frame_idx"] | ||
| frame_group = session.frame_groups[frame_idx] | ||
|
|
||
| for index, instance_group in enumerate(frame_group.instance_groups): | ||
| if index < 9: | ||
| shortcut = QKeySequence(Qt.SHIFT + Qt.Key_0 + index + 1) | ||
| else: | ||
| shortcut = "" | ||
| action = QAction(instance_group.name, self.inst_groups_delete_menu, shortcut=shortcut) | ||
| action.triggered.connect(lambda x, ig=instance_group: self.delete_instance_group(ig)) | ||
| self.inst_groups_delete_menu.addAction(action) | ||
|
|
||
| new_action = QAction("New Instance Group", self.inst_groups_delete_menu, shortcut=QKeySequence(Qt.SHIFT + Qt.Key_0)) | ||
| new_action.triggered.connect(self.commands.addInstanceGroup) | ||
| self.inst_groups_delete_menu.addAction(new_action) |
There was a problem hiding this comment.
Instead of looping through frame_group.instance_groups twice (once to MainWindow.inst_group_menu and now again to update MainWindow.inst_groups_delete_menu), let's merge/combine update_delete_instance_group_menu into the existing _update_sessions_menu. Also, please follow existing code convention (i.e. use self.inst_group_delete_menu.addAction instead of action = QAction(...)). Imitate the existing code.
sleap/gui/app.py
Outdated
| def delete_instance_group(self, instance_group): | ||
| if QMessageBox.question(self, "Delete Instance Group", | ||
| f"Are you sure you want to delete the instance group '{instance_group.name}'?", | ||
| QMessageBox.Yes | QMessageBox.No, QMessageBox.No) == QMessageBox.Yes: | ||
| self.commands.execute(DeleteInstanceGroup, instance_group=instance_group) |
There was a problem hiding this comment.
Not a fan of spamming pop-up boxes - let's ask for forgiveness not permission 😈 Also, you've skipped over using the CommandContext as it was intended to be used: add a method deleteInstanceGroup to the CommandContext and call self.commands.deleteInstanceGroup instead of adding this MainWindow.delete_instance_group.
sleap/gui/app.py
Outdated
| else: | ||
| shortcut = "" | ||
| action = QAction(instance_group.name, self.inst_groups_delete_menu, shortcut=shortcut) | ||
| action.triggered.connect(lambda x, ig=instance_group: self.delete_instance_group(ig)) |
There was a problem hiding this comment.
You've skipped over using the CommandContext as it was intended to be used: add a method deleteInstanceGroup to the CommandContext and call self.commands.deleteInstanceGroup instead of calling this MainWindow.delete_instance_group
sleap/gui/commands.py
Outdated
|
|
||
| if instance_group in frame_group.instance_groups: | ||
| frame_group.instance_groups.remove(instance_group) | ||
| context.app.refresh_ui() |
There was a problem hiding this comment.
I don't believe there is a MainWindow.refresh_ui method to call.
| context.app.refresh_ui() |
sleap/gui/app.py
Outdated
| @@ -59,7 +59,7 @@ | |||
|
|
|||
| import sleap | |||
| from sleap.gui.color import ColorManager | |||
| from sleap.gui.commands import CommandContext, UpdateTopic | |||
| from sleap.gui.commands import CommandContext, UpdateTopic, DeleteInstanceGroup | |||
There was a problem hiding this comment.
We should be calling the CommandContext.deleteInstanceGroup method (which you will need to create).
| from sleap.gui.commands import CommandContext, UpdateTopic, DeleteInstanceGroup | |
| from sleap.gui.commands import CommandContext, UpdateTopic |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-gui-elements-for-sessions #1747 +/- ##
=======================================================================
+ Coverage 74.11% 74.12% +0.01%
=======================================================================
Files 135 135
Lines 25103 25182 +79
=======================================================================
+ Hits 18605 18667 +62
- Misses 6498 6515 +17 ☔ View full report in Codecov by Sentry. |
roomrys
merged commit 84dc0eb
into
liezl/add-gui-elements-for-sessions
Add menu to assign an
Instanceto anInstanceGroupsessionsMenunamed "Sessions" (similar to thetracksMenu)self.inst_groups_menuattribute to theMainWindowthat adds a menu to thesessionsMenucalled "Set Instance Group" (similar toself.track_menu)self.inst_groups_menuwhenhas_selected_instance == True(similar toself.track_menu)MainWindow._update_sessions_menumethod that takes inselfandframe_idx, then retrieves thesession: Optional[RecordingSession] = self.state["session"]and theframe_group: FrameGroup = session.frame_groups[frame_idx]. First clear theself.inst_groups_menu. Then, enumerate through allinstance_groupsinframe_group.instance_groups: List[InstanceGroup]. For the first 9InstanceGroups use thekey_command = Qt.SHIFT + Qt.Key_0 + inst_group_ind + 1(otherwise use a defaultkey_command = ""). For allinstance_groups, add aself.inst_groups_menuaction with nameinstance_group.name, callbacklambda x=instance_group: self.commands.setInstanceGroup(x), and shortcut ofkey_command. Outside the enumeration, add a"New Instance Group"action to theself.inst_groups_menuwith name"New Instance Group", callbackself.commands.addInstanceGroup, and hotkeyQt.SHIFT + Qt.Key_0. Very similar toMainWindow._update_track_menu.CommandContext.setInstanceGroup(self, instance_group: Optional[InstanceGroup])method that executes theSetSelectedInstanceGroupclass with inputinstance_group=instance_group(seeCommandContext.setInstanceTrack).CommandContext.addInstanceGroup(self)method that executes theAddInstanceGroupclass (seeCommandContext.addTrack).sleap/gui/commands.pyadd aSetSelectedInstanceGroupclass that subclassesEditCommand. Add aSetSelectedInstanceGroup.do_action(context, params)staticmethod that first retrieves theselected_instance = context.state[selected_instance],frame_idx: int = context.state["frame_idx"],video: Video = context.state["video"],session: RecordingSession = context.state["session"],frame_group: FrameGroup = session.frame_groups.get(frame_idx, None), andcamera = session.get_camera(video=video). You will need to useframe_group = session.new_frame_group(frame_idx=frame_idx)ifframe_group is None. Then, useframe_group.add_instance(instance=selected_instance, camera=camera, instance_group=instance_group)to add theselected_instanceto theinstance_group(andframe_group). Similar to theSetSelectedInstanceTrackclass.sleap/gui/commands.pyadd aAddInstanceGroupclass that subclassesEditCommand. Add aAddInstanceGroup.do_action(context, params)staticmethod that first retrieves theframe_idx: int = context.state["frame_idx"],session: RecordingSession = context.state["session"], andframe_group: FrameGroup = session.frame_groups[frame_idx]. Then, useframe_group.add_instance_group(instance_group=None)to create and add a new emptyInstanceGroupto theframe_group. Similar to theAddTrackclass.Using the workflow above (but with less detailed write-up), please also:
Figure 2: GUI elements for assigning
Instances toInstanceGroups will be similar to how we assignInstances toTracks.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests