(4a -> 4) Add instance group dock #1775
Conversation
WalkthroughThe recent updates enhance the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant App
participant InstanceGroupDock
participant ColorManager
participant DataViews
User->>App: Open Application
App->>InstanceGroupDock: Initialize with videos_dock
App->>App: Call _update_instance_group_model
App->>DataViews: Fetch instance groups for current frame
DataViews-->>App: Return instance groups
App->>InstanceGroupDock: Update model with instance groups
App->>ColorManager: Get color for instance group
ColorManager-->>App: Return (r, g, b) color
App->>User: Display updated instance groups with colors
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked 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: 16
Out of diff range and nitpick comments (51)
tests/conftest.py (2)
Line range hint
4-4: Remove unused importpytestqt.- import pytestqt
Line range hint
5-5: Avoid using bareexcept. Specify the exception type.- except: + except ImportError:.conda/build.sh (1)
Line range hint
1-1: Add a shebang to specify the shell.+ #!/bin/bashsleap/gui/shortcuts.py (2)
Line range hint
90-90: Avoid using bareexcept. Specify the exception type.- except: + except Exception:
Line range hint
138-138: Useisinstance()instead of comparing types.- if type(idx) == int: + if isinstance(idx, int):sleap/util.py (1)
Line range hint
96-96: Avoid using bareexceptinjson_loadsfunction.- except: + except (ValueError, TypeError) as e:tests/gui/test_app.py (17)
Line range hint
80-80: EnsureUpdateTopicis defined or imported.The name
UpdateTopicmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import UpdateTopic
Line range hint
166-166: Remove unused variableinst_29_1.The local variable
inst_29_1is assigned but never used. Remove it to clean up the code.- inst_29_1 = app.state["labeled_frame"].instances[1]
Line range hint
255-255: EnsureUpdateTopicis defined or imported.The name
UpdateTopicmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import UpdateTopic
Line range hint
260-260: EnsureVideoFrameSuggestionsis defined or imported.The name
VideoFrameSuggestionsmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import VideoFrameSuggestions
Line range hint
272-272: EnsureUpdateTopicis defined or imported.The name
UpdateTopicmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import UpdateTopic
Line range hint
288-288: EnsureLabeledFrameis defined or imported.The name
LabeledFramemay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import LabeledFrame
Line range hint
295-295: EnsureUpdateTopicis defined or imported.The name
UpdateTopicmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import UpdateTopic
Line range hint
302-302: EnsureVideoFrameSuggestionsis defined or imported.The name
VideoFrameSuggestionsmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import VideoFrameSuggestions
Line range hint
315-315: Usenot infor membership test.Use
not infor membership tests to improve readability.- if not (sugg.video in video_source): + if sugg.video not in video_source:
Line range hint
336-336: Avoid equality comparisons toFalse.Use
if not win.commands.has_any_changes:for false checks to improve readability.- assert win.commands.has_any_changes == False + assert not win.commands.has_any_changes
Line range hint
337-337: Avoid equality comparisons toFalse.Use
if not win.state["project_loaded"]:for false checks to improve readability.- assert win.state["project_loaded"] == False + assert not win.state["project_loaded"]
Line range hint
344-344: EnsureOpenProjectis defined or imported.The name
OpenProjectmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import OpenProject
Line range hint
348-348: Avoid equality comparisons toTrue.Use
if win.state["project_loaded"]:for truth checks to improve readability.- assert win.state["project_loaded"] == True + assert win.state["project_loaded"]
Line range hint
355-355: EnsureOpenProjectis defined or imported.The name
OpenProjectmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import OpenProject
Line range hint
377-377: EnsureOpenProjectis defined or imported.The name
OpenProjectmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import OpenProject
Line range hint
390-390: Ensuresysis defined or imported.The name
sysmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + import sys
Line range hint
392-392: EnsureLabelsis defined or imported.The name
Labelsmay be undefined due to the use of star imports. Ensure it is defined or imported explicitly.- from sleap.gui.commands import * + from sleap.gui.commands import Labelssleap/gui/dataviews.py (2)
Line range hint
25-31: Remove unused imports.- from typing import Any, Callable, Dict, List, Optional, Type + from typing import Any, Callable, List, Optional - from sleap.gui.color import ColorManager - from sleap.io.dataset import Labels - from sleap.instance import Instance
Line range hint
118-118: Avoid using bareexceptstatements.except Exception as e: print(f"Error: {e}")sleap/io/video.py (7)
Line range hint
107-107: Useisinstancefor type checking.- elif type(self.filename) is str: + elif isinstance(self.filename, str):
Line range hint
231-231: Avoid using bareexcept. Specify the exception type.- except: + except Exception:
Line range hint
242-242: Avoid using bareexcept. Specify the exception type.- except: + except Exception:
Line range hint
536-536: Useisinstancefor type checking.- elif type(self.filename) is str: + elif isinstance(self.filename, str):
Line range hint
959-959: Replace the f-string with a regular string.- f"Could not detect backend for specified filename: {filename}" + "Could not detect backend for specified filename: " + filename
Line range hint
1066-1066: Avoid using bareexcept. Specify the exception type.- except: + except Exception:
Line range hint
1487-1487: The namesleapis not defined. Ensure it is defined or imported.- sleap.pipelines.Pipeline + from sleap import pipelinessleap/gui/widgets/video.py (1)
Line range hint
306-306: Avoid using bareexceptstatements.- except Exception: + except Exception as e: frame = None logging.error(f"Error loading frame: {e}")sleap/io/dataset.py (11)
Line range hint
56-56: Redefinition of unusedCallablefrom line 46.- Callable,
Line range hint
62-62:h5pyimported but unused.- h5py as h5
Line range hint
70-70: Avoid using bareexceptclauses.- except: + except ImportError:
Line range hint
71-71:typing._ForwardRefimported but unused.- from typing import _ForwardRef as ForwardRef
Line range hint
393-393: Avoid assigning alambdaexpression, use adeffunction instead.- Skeleton, lambda x: str(self.skeletons.index(x)) + Skeleton, def(x): return str(self.skeletons.index(x))
Line range hint
395-398: Avoid assigning alambdaexpression, use adeffunction instead.- Video, lambda x: str(self.videos.index(x)) - Node, lambda x: str(self.nodes.index(x)) - Track, lambda x: str(self.tracks.index(x)) + Video, def(x): return str(self.videos.index(x)) + Node, def(x): return str(self.nodes.index(x)) + Track, def(x): return str(self.tracks.index(x))
Line range hint
962-962: Avoid comparing types, useisinstance()instead.- if type(n) != int: + if not isinstance(n, int):
Line range hint
1514-1514: Avoid comparing types, useisinstance()instead.- if type(tr) == Track and inst.track is tr: + if isinstance(tr, Track) and inst.track is tr:
Line range hint
2716-2716: Avoid comparing types, useisinstance()instead.- if type(video) == int: + if isinstance(video, int):
Line range hint
2719-2719: Local variableeis assigned but never used.- except IndexError as e: + except IndexError:
Line range hint
2721-2721: f-string without any placeholders.- f"There are no videos in this project. No points matrix to return." + "There are no videos in this project. No points matrix to return."sleap/gui/commands.py (7)
Line range hint
1749-1749: Avoid using bareexceptclauses. Specify the exception type.- except: + except Exception:
Line range hint
1770-1770: Avoid using bareexceptclauses. Specify the exception type.- except: + except StopIteration:
Line range hint
2543-2543: Remove the unnecessary f-string.- f"{''.join(tb_str)}" + ''.join(tb_str)
Line range hint
2875-2875: Remove the unnecessary f-string.- f"{''.join(tb_str)}" + ''.join(tb_str)
Line range hint
196-196: Fix the undefined nameMainWindow.- app: "MainWindow" + app: "MainWindow" # Ensure MainWindow is imported or defined
Line range hint
856-856: Remove the unused variablefile_dir.- file_dir = os.path.dirname(filename)
Line range hint
3238-3238: Avoid using bareexceptclauses. Specify the exception type.- except: + except Exception:
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (12)
environment.ymlis excluded by!**/*.ymlenvironment_no_cuda.ymlis excluded by!**/*.ymlsleap/config/shortcuts.yamlis excluded by!**/*.yamltests/data/cameras/minimal_session/calibration.tomlis excluded by!**/*.tomltests/data/videos/min_session_back.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_backL.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_mid.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_midL.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_side.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_sideL.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_top.mp4is excluded by!**/*.mp4,!**/*.mp4tests/data/videos/min_session_topL.mp4is excluded by!**/*.mp4,!**/*.mp4
Files selected for processing (30)
- .conda/bld.bat (1 hunks)
- .conda/build.sh (1 hunks)
- .conda_mac/build.sh (1 hunks)
- .github/workflows/ci.yml (2 hunks)
- dev_requirements.txt (1 hunks)
- docs/installation.md (1 hunks)
- requirements.txt (1 hunks)
- sleap/gui/app.py (13 hunks)
- sleap/gui/commands.py (13 hunks)
- sleap/gui/dataviews.py (4 hunks)
- sleap/gui/dialogs/delete.py (1 hunks)
- sleap/gui/shortcuts.py (1 hunks)
- sleap/gui/widgets/docks.py (5 hunks)
- sleap/gui/widgets/video.py (3 hunks)
- sleap/instance.py (11 hunks)
- sleap/io/cameras.py (1 hunks)
- sleap/io/dataset.py (25 hunks)
- sleap/io/format/hdf5.py (4 hunks)
- sleap/io/format/labels_json.py (3 hunks)
- sleap/io/video.py (2 hunks)
- sleap/util.py (2 hunks)
- tests/conftest.py (1 hunks)
- tests/fixtures/cameras.py (1 hunks)
- tests/fixtures/datasets.py (1 hunks)
- tests/gui/test_app.py (1 hunks)
- tests/gui/test_commands.py (4 hunks)
- tests/gui/test_dataviews.py (3 hunks)
- tests/gui/widgets/test_docks.py (3 hunks)
- tests/io/test_cameras.py (1 hunks)
- tests/io/test_dataset.py (2 hunks)
Files not summarized due to errors (1)
- sleap/io/cameras.py: Error: Message exceeds token limit
Files not reviewed due to errors (8)
- docs/installation.md (no review received)
- sleap/io/format/labels_json.py (no review received)
- sleap/io/format/hdf5.py (no review received)
- sleap/gui/widgets/docks.py (no review received)
- tests/io/test_cameras.py (no review received)
- tests/io/test_dataset.py (no review received)
- sleap/gui/app.py (no review received)
- sleap/instance.py (no review received)
Files skipped from review due to trivial changes (2)
- dev_requirements.txt
- tests/fixtures/cameras.py
Additional Context Used
LanguageTool (5)
docs/installation.md (5)
Near line 3: The operating system from Apple is written “macOS”
Context: ...ackage on Windows, Linux, Mac OS X, and Mac OS Apple Silicon. SLEAP requires many com...
Near line 183: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... -n sleap ``` If you do not have a NVIDIA GPU, then you should use the no ...
Near line 244: Did you mean the proper noun “Apple Silicon”?
Context: ...This works on **any OS except Apple silicon** and on **Google Colab**.{note...
Near line 416: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...nning the diagnostics to see what SLEAP is able to detect on your system: ```bash sleap-d...
Near line 422: Consider using “unable” to avoid wordiness.
Context: ...`bash sleap-diagnostic ``` If you were not able to get SLEAP installed, activate the ma...
ShellCheck (1)
.conda/build.sh (1)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
Ruff (151)
sleap/gui/app.py (2)
89-89:
sleap.io.cameras.InstanceGroupimported but unused
264-264: Local variable
filenameis assigned to but never usedsleap/gui/commands.py (8)
56-56:
sleap.io.cameras.Camcorderimported but unused
196-196: Undefined name
MainWindow
856-856: Local variable
file_diris assigned to but never used
1749-1749: Do not use bare
except
1770-1770: Do not use bare
except
2543-2543: f-string without any placeholders
2875-2875: f-string without any placeholders
3238-3238: Do not use bare
exceptsleap/gui/dataviews.py (6)
25-25:
typing.Dictimported but unused
25-25:
typing.Typeimported but unused
29-29:
sleap.gui.color.ColorManagerimported but unused
30-30:
sleap.io.dataset.Labelsimported but unused
31-31:
sleap.instance.Instanceimported but unused
118-118: Do not use bare
exceptsleap/gui/shortcuts.py (2)
90-90: Do not use bare
except
138-138: Do not compare types, use
isinstance()sleap/gui/widgets/docks.py (3)
15-15: Redefinition of unused
QLabelfrom line 12
16-16: Redefinition of unused
QComboBoxfrom line 8
18-18: Redefinition of unused
QGroupBoxfrom line 10sleap/gui/widgets/video.py (21)
27-27: Module level import not at top of file
28-28: Module level import not at top of file
29-29: Module level import not at top of file
30-30: Module level import not at top of file
32-32: Module level import not at top of file
33-33: Module level import not at top of file
34-34: Module level import not at top of file
35-35: Module level import not at top of file
36-51: Module level import not at top of file
52-65: Module level import not at top of file
67-67: Module level import not at top of file
68-68: Module level import not at top of file
69-69: Module level import not at top of file
70-70: Module level import not at top of file
71-71: Module level import not at top of file
72-72: Module level import not at top of file
73-73: Module level import not at top of file
74-74: Module level import not at top of file
75-75: Module level import not at top of file
306-306: Do not use bare
except
1470-1470: Local variable
in_boundsis assigned to but never usedsleap/instance.py (6)
89-89: Undefined name
self
89-89: Undefined name
self
411-411: Do not compare types, use
isinstance()
449-449: Do not compare types, use
isinstance()
503-503: Do not use bare
except
1637-1637: Undefined name
Labelssleap/io/cameras.py (7)
79-79: f-string without any placeholders
80-80: f-string without any placeholders
81-81: f-string without any placeholders
448-448: Undefined name
Skeleton
601-601: Undefined name
Skeleton
829-829: Do not use bare
except
1003-1003: Undefined name
Labelssleap/io/dataset.py (16)
56-56: Redefinition of unused
Callablefrom line 46
62-62:
h5pyimported but unused
70-70: Do not use bare
except
71-71:
typing._ForwardRefimported but unused
393-393: Do not assign a
lambdaexpression, use adef
395-398: Do not assign a
lambdaexpression, use adef
400-403: Do not assign a
lambdaexpression, use adef
962-962: Do not compare types, use
isinstance()
1514-1514: Do not compare types, use
isinstance()
2428-2428: Undefined name
glob
2523-2523: Avoid equality comparisons to
False; useif not ret:for false checks
2611-2611: Avoid equality comparisons to
False; useif not ret:for false checks
2626-2626: Undefined name
sleap
2716-2716: Do not compare types, use
isinstance()
2719-2719: Local variable
eis assigned to but never used
2721-2721: f-string without any placeholders
sleap/io/format/labels_json.py (6)
154-154: Local variable
exis assigned to but never used
224-224: Local variable
exis assigned to but never used
379-379: Do not compare types, use
isinstance()
404-404: Do not use bare
except
408-408: Do not use bare
except
428-428: Local variable
new_skelis assigned to but never usedsleap/io/video.py (8)
107-107: Do not compare types, use
isinstance()
231-231: Do not use bare
except
242-242: Do not use bare
except
536-536: Do not compare types, use
isinstance()
959-959: f-string without any placeholders
1066-1066: Do not use bare
except
1487-1487: Undefined name
sleap
1586-1586: Do not compare types, use
isinstance()sleap/util.py (2)
96-96: Do not use bare
except
130-130: Import
fieldfrom line 19 shadowed by loop variabletests/conftest.py (8)
4-4:
pytestqtimported but unused
5-5: Do not use bare
except
9-9:
from tests.fixtures.skeletons import *used; unable to detect undefined names
10-10:
from tests.fixtures.instances import *used; unable to detect undefined names
11-11:
from tests.fixtures.datasets import *used; unable to detect undefined names
12-12:
from tests.fixtures.videos import *used; unable to detect undefined names
13-13:
from tests.fixtures.models import *used; unable to detect undefined names
14-14:
from tests.fixtures.cameras import *used; unable to detect undefined namestests/fixtures/datasets.py (1)
1-1:
osimported but unusedtests/gui/test_app.py (19)
8-8:
from sleap.gui.commands import *used; unable to detect undefined names
18-18:
Labelsmay be undefined, or defined from star imports
80-80:
UpdateTopicmay be undefined, or defined from star imports
166-166: Local variable
inst_29_1is assigned to but never used
255-255:
UpdateTopicmay be undefined, or defined from star imports
260-260:
VideoFrameSuggestionsmay be undefined, or defined from star imports
272-272:
UpdateTopicmay be undefined, or defined from star imports
288-288:
LabeledFramemay be undefined, or defined from star imports
295-295:
UpdateTopicmay be undefined, or defined from star imports
302-302:
VideoFrameSuggestionsmay be undefined, or defined from star imports
315-315: Test for membership should be
not in
336-336: Avoid equality comparisons to
False; useif not win.commands.has_any_changes:for false checks
337-337: Avoid equality comparisons to
False; useif not win.state["project_loaded"]:for false checks
344-344:
OpenProjectmay be undefined, or defined from star imports
348-348: Avoid equality comparisons to
True; useif win.state["project_loaded"]:for truth checks
355-355:
OpenProjectmay be undefined, or defined from star imports
377-377:
OpenProjectmay be undefined, or defined from star imports
390-390:
sysmay be undefined, or defined from star imports
392-392:
Labelsmay be undefined, or defined from star importstests/gui/test_commands.py (13)
75-75: Ambiguous variable name:
l
82-82: Ambiguous variable name:
l
227-227: Avoid equality comparisons to
True; useif okay:for truth checks
236-236: Avoid equality comparisons to
True; useif okay:for truth checks
244-244: Avoid equality comparisons to
True; useif okay:for truth checks
253-253: Avoid equality comparisons to
True; useif okay:for truth checks
265-265: Avoid equality comparisons to
True; useif okay:for truth checks
274-274: Avoid equality comparisons to
True; useif okay:for truth checks
281-281: Avoid equality comparisons to
True; useif okay:for truth checks
299-299: Avoid equality comparisons to
True; useif okay:for truth checks
360-360: Local variable
last_lf_frameis assigned to but never used
368-368: Avoid equality comparisons to
True; useif video.backend.grayscale:for truth checks
528-528: f-string without any placeholders
tests/gui/test_dataviews.py (17)
1-1:
from sleap.gui.dataviews import *used; unable to detect undefined names
5-5:
GenericTableViewmay be undefined, or defined from star imports
6-6:
SkeletonNodesTableModelmay be undefined, or defined from star imports
12-12:
GenericTableViewmay be undefined, or defined from star imports
13-13:
SkeletonEdgesTableModelmay be undefined, or defined from star imports
18-18:
GenericTableViewmay be undefined, or defined from star imports
20-20:
VideosTableModelmay be undefined, or defined from star imports
30-30:
GenericTableViewmay be undefined, or defined from star imports
33-33:
LabeledFrameTableModelmay be undefined, or defined from star imports
40-40:
GenericTableViewmay be undefined, or defined from star imports
44-44:
LabeledFrameTableModelmay be undefined, or defined from star imports
76-76:
GenericTableViewmay be undefined, or defined from star imports
80-80:
SessionsTableModelmay be undefined, or defined from star imports
101-101:
GenericTableModelmay be undefined, or defined from star imports
105-105:
GenericTableViewmay be undefined, or defined from star imports
117-117:
CamerasTableModelmay be undefined, or defined from star imports
123-123:
GenericTableViewmay be undefined, or defined from star importstests/io/test_dataset.py (6)
119-119: Test for object identity should be
is not
120-120: Test for membership should be
not in
121-121: Test for object identity should be
is not
692-692: Local variable
new_labels_jsonis assigned to but never used
708-708: Local variable
new_labels_jsonis assigned to but never used
1495-1495: Local variable
track_instsis assigned to but never used
Learnings (1)
sleap/io/dataset.py (1)
User: roomrys PR: talmolab/sleap#1654 File: sleap/io/dataset.py:1717-1725 Timestamp: 2024-03-15T23:59:34.765Z Learning: When removing a recording session using the `remove_recording_session` method in the `Labels` class, it's necessary to also update the `LabelsDataCache._session_by_video` dictionary to reflect the removal. This ensures that the mapping between videos and their corresponding recording sessions remains accurate.
Additional comments not posted (76)
tests/conftest.py (1)
14-14: Import statement fortests.fixtures.cameraslooks good..conda_mac/build.sh (1)
14-14: Installation command foropencv-contrib-pythonlooks good..conda/bld.bat (2)
13-13: Uninstallation command for OpenCV packages looks good.
14-14: Installation command foropencv-contrib-pythonlooks good.requirements.txt (2)
9-9: Addition ofsleap-aniposelooks good.
17-28: The hack for uninstalling OpenCV packages and installingopencv-contrib-pythonlooks good..github/workflows/ci.yml (1)
94-101: LGTM! The changes correctly reinstall the specified version of OpenCV.sleap/gui/dialogs/delete.py (5)
Line range hint
14-14: LGTM! The__init__method is well-structured and follows good practices.
Line range hint
34-34: LGTM! The_make_form_widgetmethod is well-structured and follows good practices.
Line range hint
61-61: LGTM! The_make_button_widgetmethod is well-structured and follows good practices.
Line range hint
108-108: LGTM! Theget_frames_instancesmethod is well-structured and follows good practices.
219-219: LGTM! The_deletemethod is well-structured and follows good practices.tests/fixtures/datasets.py (3)
275-281: LGTM! Themultiview_min_session_labelsfixture is well-structured and follows good practices.
283-289: LGTM! Themultiview_min_session_user_labelsfixture is well-structured and follows good practices.
291-296: LGTM! Themultiview_min_session_frame_groupsfixture is well-structured and follows good practices.tests/gui/widgets/test_docks.py (4)
115-123: LGTM! Thetest_sessions_docktest is well-structured and follows good practices.
125-171: LGTM! Thetest_sessions_dock_cameras_tabletest is well-structured and follows good practices.
174-193: LGTM! Thetest_sessions_dock_session_tabletest is well-structured and follows good practices.
195-254: LGTM! Thetest_sessions_dock_unlinked_videos_tabletest is well-structured and follows good practices.sleap/util.py (2)
14-14: New imports look good and are necessary for the added functionality.Also applies to: 19-20
69-83: Thedeep_iterable_converterfunction is well-implemented and correctly uses the_DeepIterableConverterclass.tests/gui/test_commands.py (5)
930-957: LGTM! The test for adding a session is well-implemented.
959-993: LGTM! The test for checking if there are enough instances for triangulation is well-implemented.
996-1028: LGTM! The initial setup and the first scenario for triangulating all instances are well-implemented.
1029-1059: LGTM! The second scenario for triangulating selected instances is well-implemented.
1060-1104: LGTM! The third scenario for triangulating instances using theGuiStateis well-implemented.sleap/io/video.py (1)
15-15: The import statement modification looks good.sleap/gui/widgets/video.py (27)
Line range hint
72-72: LGTM! The initialization ofQtVideoPlayeris well-structured.
Line range hint
153-153: LGTM! Thecleanupmethod is straightforward and correctly quits the loader thread.
Line range hint
156-156: LGTM! ThedragEnterEventmethod is straightforward and correctly handles drag enter events.
Line range hint
159-159: LGTM! ThedropEventmethod is straightforward and correctly handles drop events.
Line range hint
162-162: LGTM! The_load_and_show_requested_imagemethod is straightforward and correctly loads and shows the requested image.
Line range hint
165-165: LGTM! The_register_shortcutsmethod is well-structured and correctly registers shortcuts for frame navigation.
Line range hint
186-186: LGTM! ThesetSeekbarSelectionmethod is straightforward and correctly sets the selection on the seekbar.
Line range hint
189-189: LGTM! Theshow_contextual_menumethod is well-structured and correctly shows a contextual menu.
Line range hint
209-209: LGTM! Theload_videomethod is well-structured and correctly loads a video into the viewer.
Line range hint
226-226: LGTM! Theresetmethod is straightforward and correctly resets the viewer by removing all video data.
Line range hint
238-238: LGTM! Theinstancesmethod is straightforward and correctly returns a list of allQtInstanceobjects in view.
Line range hint
243-243: LGTM! Theselectable_instancesmethod is straightforward and correctly returns a list of selectableQtInstanceobjects in view.
Line range hint
248-248: LGTM! Thepredicted_instancesmethod is straightforward and correctly returns a list of predictedQtInstanceobjects in view.
Line range hint
253-253: LGTM! Thescenemethod is straightforward and correctly returns theQGraphicsScenefor the viewer.
Line range hint
258-258: LGTM! TheaddInstancemethod is well-structured and correctly adds a skeleton instance to the video.
Line range hint
306-306: LGTM! The initialization ofGraphicsViewis well-structured.
Line range hint
306-306: LGTM! ThedragEnterEventmethod is straightforward and correctly handles drag enter events.
Line range hint
306-306: LGTM! ThedropEventmethod is straightforward and correctly handles drop events.
Line range hint
306-306: LGTM! ThehasImagemethod is straightforward and correctly returns whether or not the scene contains an image pixmap.
Line range hint
306-306: LGTM! Theclearmethod is straightforward and correctly clears the displayed frame from the scene.
Line range hint
306-306: LGTM! The_add_pixmapmethod is straightforward and correctly adds a pixmap to the scene and transforms it to midpoint coordinates.
Line range hint
306-306: LGTM! ThesetImagemethod is well-structured and correctly sets the scene's current image pixmap to the input QImage or QPixmap.
Line range hint
306-306: LGTM! TheupdateViewermethod is straightforward and correctly applies the current zoom.
Line range hint
306-306: LGTM! Theinstancesmethod is straightforward and correctly returns a list of instances.
Line range hint
306-306: LGTM! Thepredicted_instancesmethod is straightforward and correctly returns a list of predicted instances.
Line range hint
306-306: LGTM! Theselectable_instancesmethod is straightforward and correctly returns a list of selectable instances.
Line range hint
306-306: LGTM! Theall_instancesmethod is straightforward and correctly returns a list of allQtInstanceobjects in the scene.sleap/io/cameras.py (6)
158-350: LGTM!
1210-1510: LGTM!
1511-1512: LGTM!
1514-1810: LGTM!
1811-2175: LGTM!
2176-2291: LGTM!sleap/gui/commands.py (16)
85-85: LGTM! The addition ofsessionstoUpdateTopicis appropriate.
414-416: LGTM! ThenextViewmethod is a useful addition for navigating views.
418-420: LGTM! TheprevViewmethod is a useful addition for navigating views.
444-446: LGTM! TheaddSessionmethod is a useful addition for managing recording sessions.
448-450: LGTM! TheremoveSelectedSessionmethod is a useful addition for managing recording sessions.
452-454: LGTM! ThelinkVideoToSessionmethod is a useful addition for managing recording sessions.
633-645: LGTM! ThetriangulateSessionmethod is a useful addition for managing recording sessions.
663-665: LGTM! Theunlink_video_from_cameramethod is a useful addition for managing recording sessions.
1983-1995: LGTM! TheRemoveSessionclass is a useful addition for managing recording sessions.
1998-2015: LGTM! TheAddSessionclass is a useful addition for managing recording sessions.
2033-2047: LGTM! TheLinkVideoToSessionclass is a useful addition for managing recording sessions.
3483-3564: LGTM! TheTriangulateSessionclass is a useful addition for managing recording sessions.
3679-3693: LGTM! TheUnlinkVideoclass is a useful addition for managing recording sessions.
709-709: LGTM! The addition of setting the session state inLoadLabelsObjectis appropriate.
3008-3027: LGTM! The additions to handle frame groups and instance groups inAddInstanceare appropriate.
1711-1727: LGTM! TheGoAdjacentViewclass is a useful addition for navigating views.
.conda/build.sh
Outdated
| @@ -9,6 +9,9 @@ export PIP_IGNORE_INSTALLED=False | |||
| # https://docs.conda.io/projects/conda-build/en/stable/user-guide/wheel-files.html) | |||
| pip install --no-cache-dir -r ./requirements.txt | |||
|
|
|||
| # HACK(LM): (untested) Uninstall all opencv packages and install opencv-contrib-python | |||
| conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}' | |||
There was a problem hiding this comment.
Simplify the command to uninstall OpenCV packages.
- conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}'
+ pip uninstall -y $(pip list | grep opencv | awk '{print $1}')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.
| conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}' | |
| pip uninstall -y $(pip list | grep opencv | awk '{print $1}') |
roomrys
changed the base branch from
develop
to
liezl/add-gui-elements-for-sessions
roomrys
changed the title
…com/talmolab/sleap into vaibhav/add-isntance-group-dock
There was a problem hiding this comment.
It would be nice to have:
- ability to double click row and have it select the associated instance and outline it with a bounding box (as is done in the InstancesDock)
- change the Name cell to the color of the InstanceGroup (if Color By Instance Group is checked)
- A test in test_docks
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
sleap/gui/app.py (1)
Line range hint
262-262: Unused local variablefilename.The variable
filenameis declared but not used, which could lead to confusion and clutter in the codebase.- filename = event.mimeData().data(mime_format).data().decode()Consider removing this line if
filenameis indeed unnecessary, or use it appropriately if intended for future use.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/app.py (3 hunks)
Additional Context Used
Ruff (1)
sleap/gui/app.py (1)
262-262: Local variable
filenameis assigned to but never used
Additional comments not posted (3)
sleap/gui/app.py (3)
78-78: Addition ofInstanceGroupDockto the import list.This change aligns with the PR's objective to manage instance groups within the GUI.
1046-1046: Introduction ofInstanceGroupDockin the GUI setup.This addition is necessary for the new feature of managing instance groups and integrates well with the existing dock system.
1273-1273: Update methods for instance group models.These methods are crucial for updating the GUI elements related to instance groups when changes occur. The logic appears correct and efficient.
Also applies to: 1281-1290
…com/talmolab/sleap into vaibhav/add-isntance-group-dock
… when frame changes
roomrys
merged commit e5b8c67
into
talmolab:liezl/add-gui-elements-for-sessions
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
sleap/gui/color.py (2)
Line range hint
140-140: Replace bareexceptwith specific exceptions to improve error handling.- except: + except ValueError:Using specific exceptions helps in understanding the exact failure conditions and ensures that unexpected issues are not silently ignored.
Also applies to: 149-149
Line range hint
263-263: Undefined namesSkeletonandLabeledFrameneed to be imported or defined.+from sleap.skeleton import Skeleton +from sleap.instance import LabeledFrameEnsure that all classes used in the file are properly imported to avoid runtime errors.
Also applies to: 265-265
sleap/gui/dataviews.py (2)
Line range hint
25-25: Remove unused imports to clean up the code.- from typing import Dict, Type - from sleap.gui.color import ColorManager - from sleap.io.dataset import Labels - from sleap.instance import InstanceRemoving unused imports helps in maintaining cleaner code and reduces confusion about dependencies.
Also applies to: 29-29, 30-30, 31-31
Line range hint
118-118: Replace bareexceptwith specific exceptions to improve error handling.- except: + except KeyError: # Assuming the key error is the most likely exceptionUsing specific exceptions helps in understanding the exact failure conditions and ensures that unexpected issues are not silently ignored.
sleap/gui/app.py (1)
Line range hint
262-262: The local variablefilenameis assigned but never used in the methoddragEnterEvent. This could potentially be a remnant of a previous implementation. Consider removing it if it's not needed.- filename = event.mimeData().data(mime_format).data().decode()
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sleap/gui/app.py (4 hunks)
- sleap/gui/color.py (2 hunks)
- sleap/gui/dataviews.py (2 hunks)
Additional Context Used
Ruff (11)
sleap/gui/app.py (1)
262-262: Local variable
filenameis assigned to but never usedsleap/gui/color.py (4)
140-140: Do not use bare
except
149-149: Do not use bare
except
263-263: Undefined name
Skeleton
265-265: Undefined name
LabeledFramesleap/gui/dataviews.py (6)
25-25:
typing.Dictimported but unused
25-25:
typing.Typeimported but unused
29-29:
sleap.gui.color.ColorManagerimported but unused
30-30:
sleap.io.dataset.Labelsimported but unused
31-31:
sleap.instance.Instanceimported but unused
118-118: Do not use bare
except
Additional comments not posted (3)
sleap/gui/app.py (3)
78-78: Added import forInstanceGroupDockaligns with the PR's objective to introduce a new dock for instance groups.
1282-1296: The method_update_instance_group_modelis well-implemented to update the instance group model based on the current frame. However, consider adding error handling for cases whereframe_groupmight beNoneto avoid potential runtime errors.if frame_group is not None: self.instance_groups_dock.table.model().items = ( frame_group.instance_groups ) return + else: + logger.error("No frame group found for frame index: {}", frame_idx)Likely invalid or redundant comment.
1046-1046: The addition ofInstanceGroupDockin the method_create_dock_windowsis consistent with the PR's description. It's important to ensure that this new dock integrates well with the existing GUI structure and does not introduce any layout issues.Verification successful
The integration of
InstanceGroupDockappears to be consistent with the existing GUI structure. TheInstanceGroupDockclass is implemented similarly to other dock widgets, such asVideosDock,SkeletonDock, andSuggestionsDock. It follows the same pattern of creating models, tables, and laying out the components within the dock.The
_create_dock_windowsmethod insleap/gui/app.pyaddsInstanceGroupDockalongside other dock widgets, ensuring it is integrated into the GUI layout.
InstanceGroupDockis defined insleap/gui/widgets/docks.pyand follows the same structure as other dock widgets.- The
_create_dock_windowsmethod insleap/gui/app.pyincludesInstanceGroupDockin the GUI layout without any apparent issues.Based on the inspection of the code, there are no immediate concerns regarding the integration of
InstanceGroupDockwith the existing GUI structure.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of InstanceGroupDock with existing GUI structure. # Test: Search for GUI layout related issues in the application logs. cat logs/application.log | grep -i 'layout'Length of output: 96
Script:
#!/bin/bash # Search for the definition of InstanceGroupDock ast-grep --lang python --pattern $'class InstanceGroupDock($_, $_):\n $$$' # Search for the usage of InstanceGroupDock in the codebase rg 'InstanceGroupDock' # Search for the definition of the _create_dock_windows method ast-grep --lang python --pattern $'def _create_dock_windows($_):\n $$$'Length of output: 1306
Script:
#!/bin/bash # Inspect the implementation of InstanceGroupDock ast-grep --lang python --pattern $'class InstanceGroupDock(DockWidget):\n $$$' sleap/gui/widgets/docks.py # Inspect the implementation of other dock windows in _create_dock_windows ast-grep --lang python --pattern $'class $_(DockWidget):\n $$$' sleap/gui/widgets/docks.pyLength of output: 42688
| def get_instance_group_color(self, instance_group, frame_group): | ||
| """Returns the color to use for a given instance group. | ||
|
|
||
| Args: | ||
| instance_group: `InstanceGroup` object | ||
| frame_group: `FrameGroup` object | ||
| Returns: | ||
| (r, g, b)-tuple | ||
| """ | ||
| if frame_group is None or instance_group is None: | ||
| return (0, 0, 0) | ||
|
|
||
| if instance_group in frame_group.instance_groups: | ||
| instance_group_idx = frame_group.instance_groups.index(instance_group) | ||
| return self.get_color_by_idx(instance_group_idx) |
There was a problem hiding this comment.
Add handling for potential index errors in get_instance_group_color.
- instance_group_idx = frame_group.instance_groups.index(instance_group)
+ try:
+ instance_group_idx = frame_group.instance_groups.index(instance_group)
+ except ValueError:
+ return (0, 0, 0) # Return a default color if instance group is not foundThis change ensures that if the instance group is not found within the frame group, a default color is returned instead of raising an exception.
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.
| def get_instance_group_color(self, instance_group, frame_group): | |
| """Returns the color to use for a given instance group. | |
| Args: | |
| instance_group: `InstanceGroup` object | |
| frame_group: `FrameGroup` object | |
| Returns: | |
| (r, g, b)-tuple | |
| """ | |
| if frame_group is None or instance_group is None: | |
| return (0, 0, 0) | |
| if instance_group in frame_group.instance_groups: | |
| instance_group_idx = frame_group.instance_groups.index(instance_group) | |
| return self.get_color_by_idx(instance_group_idx) | |
| def get_instance_group_color(self, instance_group, frame_group): | |
| """Returns the color to use for a given instance group. | |
| Args: | |
| instance_group: `InstanceGroup` object | |
| frame_group: `FrameGroup` object | |
| Returns: | |
| (r, g, b)-tuple | |
| """ | |
| if frame_group is None or instance_group is None: | |
| return (0, 0, 0) | |
| if instance_group in frame_group.instance_groups: | |
| try: | |
| instance_group_idx = frame_group.instance_groups.index(instance_group) | |
| except ValueError: | |
| return (0, 0, 0) # Return a default color if instance group is not found | |
| return self.get_color_by_idx(instance_group_idx) |
|
|
||
|
|
||
| class InstanceGroupTableModel(GenericTableModel): | ||
| """Table model for displaying all instance groups in a given frame. | ||
|
|
||
| Args: | ||
| item: 'InstanceGroup' which has information about the instance group | ||
| """ | ||
|
|
||
| properties = ("name", "frame index", "cameras", "instances") | ||
|
|
||
| def item_to_data(self, obj, item: InstanceGroup): | ||
|
|
||
| return { | ||
| "name": item.name, | ||
| "frame index": item.frame_idx, | ||
| "cameras": len(item.camera_cluster.cameras), | ||
| "instances": len(item.instances), | ||
| } | ||
|
|
||
| def get_item_color(self, instance_group: InstanceGroup, key: str): | ||
| color_manager = self.context.app.color_manager | ||
| if color_manager.distinctly_color == "instance_groups" and key == "name": | ||
|
|
||
| # Get the RecordingSession | ||
| state = self.context.state | ||
| session = state["session"] | ||
| if session is None: | ||
| return | ||
|
|
||
| # Get the FrameGroup | ||
| frame_idx = state["frame_idx"] | ||
| frame_group = session.frame_groups.get(frame_idx, None) | ||
| if frame_group is None: | ||
| return | ||
|
|
||
| # Get the InstanceGroup and color | ||
| color = color_manager.get_instance_group_color(instance_group, frame_group) | ||
| return QtGui.QColor(*color) |
There was a problem hiding this comment.
Ensure consistent handling of None values in get_item_color.
- if session is None:
+ if session is None or frame_group is None:
+ return QtGui.QColor(0, 0, 0) # Return a default color if session or frame group is not availableThis change ensures that the method returns a default color when necessary data is not available, preventing potential runtime errors.
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.
| class InstanceGroupTableModel(GenericTableModel): | |
| """Table model for displaying all instance groups in a given frame. | |
| Args: | |
| item: 'InstanceGroup' which has information about the instance group | |
| """ | |
| properties = ("name", "frame index", "cameras", "instances") | |
| def item_to_data(self, obj, item: InstanceGroup): | |
| return { | |
| "name": item.name, | |
| "frame index": item.frame_idx, | |
| "cameras": len(item.camera_cluster.cameras), | |
| "instances": len(item.instances), | |
| } | |
| def get_item_color(self, instance_group: InstanceGroup, key: str): | |
| color_manager = self.context.app.color_manager | |
| if color_manager.distinctly_color == "instance_groups" and key == "name": | |
| # Get the RecordingSession | |
| state = self.context.state | |
| session = state["session"] | |
| if session is None: | |
| return | |
| # Get the FrameGroup | |
| frame_idx = state["frame_idx"] | |
| frame_group = session.frame_groups.get(frame_idx, None) | |
| if frame_group is None: | |
| return | |
| # Get the InstanceGroup and color | |
| color = color_manager.get_instance_group_color(instance_group, frame_group) | |
| return QtGui.QColor(*color) | |
| if session is None or frame_group is None: | |
| return QtGui.QColor(0, 0, 0) # Return a default color if session or frame group is not available | |
| # Get the FrameGroup | |
| frame_idx = state["frame_idx"] | |
| frame_group = session.frame_groups.get(frame_idx, None) | |
| if frame_group is None: | |
| return | |
| # Get the InstanceGroup and color | |
| color = color_manager.get_instance_group_color(instance_group, frame_group) | |
| return QtGui.QColor(*color) |
Description
Created an instance group dock for multiview. Stores data about the different instance groups in a frame which includes instance group name, number of instances, and number of cameras in each group.
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️