-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add feature to maintain current frame index #1789
Add feature to maintain current frame index #1789
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce significant new functionality and updates across multiple aspects of the project. Key updates include the addition of session and instance group management in the GUI, handling of OpenCV package installations, and improvements to testing and documentation. These enhancements aim to improve the user experience, streamline package management, and ensure robust testing of new features. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant GUI
participant Commands
participant DataCache
participant Labels
User->>GUI: Add Session
GUI->>Commands: addSession()
Commands->>DataCache: add_recording_session()
DataCache->>Labels: add_session()
Labels->>DataCache: update_cache()
DataCache-->>GUI: Session Added
User->>GUI: Triangulate Session
GUI->>Commands: triangulateSession()
Commands->>DataCache: triangulate_instances()
DataCache-->>GUI: Triangulation Complete
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 17
Outside diff range and nitpick comments (41)
tests/conftest.py (1)
Line range hint
4-5
: Replace the bareexcept
with a specific exception to avoid catching unexpected errors.- except: + except ImportError:sleap/gui/shortcuts.py (2)
Line range hint
90-90
: Replace the bareexcept
with a more specific exception type to avoid catching unexpected exceptions.- except: + except Exception as e:
Line range hint
138-138
: Useisinstance()
for type checking instead of comparing types directly.- if type(idx) == int: + if isinstance(idx, int):tests/gui/test_app.py (2)
Line range hint
166-166
: The local variableinst_29_1
is assigned but never used, which could be an oversight or redundant code.
Line range hint
336-336
: Simplify boolean checks for clarity and Pythonic style.- assert win.commands.has_any_changes == False + assert not win.commands.has_any_changes - assert win.state["project_loaded"] == False + assert not win.state["project_loaded"] - assert win.state["project_loaded"] == True + assert win.state["project_loaded"]Also applies to: 337-337, 348-348
docs/installation.md (2)
Line range hint
3-3
: Correct the spelling of "Mac OS" to "macOS" and "Apple silicon" to "Apple Silicon" for consistency with Apple's branding.- Mac OS X, and Mac OS Apple Silicon + macOS, and macOS Apple SiliconAlso applies to: 244-244
Line range hint
31-31
: Address markdown linting issues to improve the readability and structure of the document.- ### Macs Pre-M1 (Pre-Installation) + ## Macs Pre-M1 (Pre-Installation) - https://mamba.readthedocs.io/en/latest/installation.html + [Mambaforge](https://mamba.readthedocs.io/en/latest/installation.html) - ```bash + ``` - ```bash + ``` - ```bash + ```Also applies to: 92-92, 301-301, 320-320, 339-339
sleap/io/format/labels_json.py (4)
Line range hint
379-379
: Replace type comparison withisinstance()
.- if type(data) is str: + if isinstance(data, str):
Line range hint
404-404
: Replace bareexcept
with specific exceptions to avoid catching unexpected errors.- except: + except ValueError:Also applies to: 408-408
Line range hint
154-154
: Remove unused local variableex
.- except Exception as ex: + except Exception:Also applies to: 224-224
Line range hint
428-428
: Remove unused variablenew_skel
.- new_skel = skeletons[0]
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 InstanceAlso applies to: 25-25, 29-29, 30-30, 31-31
Line range hint
118-118
: Replace the bareexcept
with specific exceptions to avoid catching unexpected errors.- except: + except KeyError: # or the specific exceptions you expecttests/io/test_dataset.py (6)
Line range hint
119-119
: Useis not
for object identity checks.- assert not multi_skel_vid_labels.skeletons[0] is loaded_labels.skeletons[0] + assert multi_skel_vid_labels.skeletons[0] is not loaded_labels.skeletons[0]
Line range hint
120-120
: Usenot in
for membership checks.- assert not multi_skel_vid_labels.nodes[3] in loaded_labels.nodes + assert multi_skel_vid_labels.nodes[3] not in loaded_labels.nodes
Line range hint
121-121
: Useis not
for object identity checks.- assert not multi_skel_vid_labels.videos[0] is loaded_labels.videos[0] + assert multi_skel_vid_labels.videos[0] is not loaded_labels.videos[0]
Line range hint
692-692
: Local variablenew_labels_json
is assigned but never used.Consider removing or using the variable
new_labels_json
if it was intended for further operations.
Line range hint
708-708
: Local variablenew_labels_json
is assigned but never used.Consider removing or using the variable
new_labels_json
if it was intended for further operations.
Line range hint
1495-1495
: Local variabletrack_insts
is assigned but never used.Consider removing or using the variable
track_insts
if it was intended for further operations.sleap/io/video.py (6)
Line range hint
107-107
: Useisinstance()
for type checks.- if type(self.filename) is str: + if isinstance(self.filename, str):
Line range hint
231-231
: Replace bareexcept
with specific exceptions.- except: + except Exception as e:Also applies to: 242-242, 1066-1066
Line range hint
536-536
: Useisinstance()
for type checks.- if type(path) is not str: + if not isinstance(path, str):
Line range hint
959-959
: Remove unnecessary f-string.- raise FileNotFoundError(f"Cannot find a video file: {path}") + raise FileNotFoundError("Cannot find a video file: " + path)
Line range hint
1487-1487
: Undefined name 'sleap' in the return type hint.- def to_pipeline(self, batch_size: Optional[int] = None, prefetch: bool = True, frame_indices: Optional[List[int]] = None) -> "sleap.pipelines.Pipeline": + def to_pipeline(self, batch_size: Optional[int] = None, prefetch: bool = True, frame_indices: Optional[List[int]] = None) -> "Pipeline":
Line range hint
1586-1586
: Useisinstance()
for type checks.- if type(path) is not str: + if not isinstance(path, str):tests/gui/test_commands.py (4)
Line range hint
227-227
: Refactor to use direct truthiness checks.- if okay == True + if okayReplace equality checks with
True
with direct truthiness checks to make the code more Pythonic and readable.Also applies to: 236-236, 244-244, 253-253, 265-265, 274-274, 281-281, 299-299
Line range hint
360-360
: Unused variablelast_lf_frame
.The variable
last_lf_frame
is assigned but never used. Consider removing it if it's not needed, or ensure it's used appropriately if intended.
Line range hint
368-368
: Refactor to use direct truthiness checks.- if video.backend.grayscale == True + if video.backend.grayscaleUse direct truthiness checks for boolean conditions.
Line range hint
528-528
: f-string without placeholders.The f-string used here does not contain any placeholders, which makes the use of an f-string unnecessary. Consider using a regular string unless you plan to include variables or expressions.
sleap/instance.py (3)
20-20
: Add a brief comment explaining the purpose of thelogging
import.
Line range hint
411-411
: Replace type comparison withisinstance()
.- if from_predicted is not None and type(from_predicted) != PredictedInstance: + if from_predicted is not None and not isinstance(from_predicted, PredictedInstance):
Line range hint
1637-1637
: The nameLabels
is undefined in this context. Ensure that it is imported or defined within the scope.+ from sleap.labels import Labels
sleap/gui/widgets/video.py (3)
Line range hint
27-75
: Move all module-level imports to the top of the file.- 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 + # Move all these imports to the top of the file
Line range hint
306-306
: Avoid using bareexcept
statements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
1470-1470
: The local variablein_bounds
is assigned but never used.- in_bounds = True
sleap/io/dataset.py (2)
Line range hint
2428-2428
: Fix the undefined nameglob
.- csv_files = glob(f"{filename}/*/*.csv") + from glob import glob + csv_files = glob(f"{filename}/*/*.csv")The
glob
module is used but not imported, which will cause a runtime error. Ensure that it is properly imported at the beginning of the file.
Line range hint
2523-2523
: Simplify the conditional check.- if ret == False: + if not ret:For checking boolean conditions, it's more Pythonic to use
if not ret:
instead of comparing directly toFalse
.sleap/gui/commands.py (4)
Line range hint
196-196
: Undefined nameMainWindow
used. Ensure thatMainWindow
is defined or imported correctly.
Line range hint
868-868
: Local variablefile_dir
is assigned but never used. Consider removing it if it's unnecessary.- file_dir = os.path.dirname(filename)
Line range hint
1763-1763
: Avoid using bareexcept
statements. Specify exception types to handle specific exceptions appropriately.- except: + except Exception as e:Also applies to: 1784-1784, 3356-3356
Line range hint
2557-2557
: f-string without any placeholders. Convert to regular string if dynamic formatting is not needed.- f"Require at least 1 instance group, but found " + "Require at least 1 instance group, but found "Also applies to: 2993-2993
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (12)
environment.yml
is excluded by!**/*.yml
environment_no_cuda.yml
is excluded by!**/*.yml
sleap/config/shortcuts.yaml
is excluded by!**/*.yaml
tests/data/cameras/minimal_session/calibration.toml
is excluded by!**/*.toml
tests/data/videos/min_session_back.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_backL.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_mid.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_midL.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_side.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_sideL.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_top.mp4
is excluded by!**/*.mp4
,!**/*.mp4
tests/data/videos/min_session_topL.mp4
is 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 (18 hunks)
- sleap/gui/commands.py (19 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 (3)
- sleap/gui/dialogs/delete.py (no review received)
- tests/fixtures/datasets.py (no review received)
- tests/gui/widgets/test_docks.py (no review received)
Files skipped from review due to trivial changes (2)
- dev_requirements.txt
- tests/fixtures/cameras.py
Additional context 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.
Ruff
tests/conftest.py
4-4:
pytestqt
imported 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 namessleap/gui/shortcuts.py
90-90: Do not use bare
except
138-138: Do not compare types, use
isinstance()
tests/gui/test_dataviews.py
1-1:
from sleap.gui.dataviews import *
used; unable to detect undefined names
5-5:
GenericTableView
may be undefined, or defined from star imports
6-6:
SkeletonNodesTableModel
may be undefined, or defined from star imports
12-12:
GenericTableView
may be undefined, or defined from star imports
13-13:
SkeletonEdgesTableModel
may be undefined, or defined from star imports
18-18:
GenericTableView
may be undefined, or defined from star imports
20-20:
VideosTableModel
may be undefined, or defined from star imports
30-30:
GenericTableView
may be undefined, or defined from star imports
33-33:
LabeledFrameTableModel
may be undefined, or defined from star imports
40-40:
GenericTableView
may be undefined, or defined from star imports
44-44:
LabeledFrameTableModel
may be undefined, or defined from star imports
76-76:
GenericTableView
may be undefined, or defined from star imports
80-80:
SessionsTableModel
may be undefined, or defined from star imports
101-101:
GenericTableModel
may be undefined, or defined from star imports
105-105:
GenericTableView
may be undefined, or defined from star imports
117-117:
CamerasTableModel
may be undefined, or defined from star imports
123-123:
GenericTableView
may be undefined, or defined from star importstests/fixtures/datasets.py
1-1:
os
imported but unusedsleap/util.py
96-96: Do not use bare
except
130-130: Import
field
from line 19 shadowed by loop variabletests/gui/test_app.py
8-8:
from sleap.gui.commands import *
used; unable to detect undefined names
18-18:
Labels
may be undefined, or defined from star imports
80-80:
UpdateTopic
may be undefined, or defined from star imports
166-166: Local variable
inst_29_1
is assigned to but never used
255-255:
UpdateTopic
may be undefined, or defined from star imports
260-260:
VideoFrameSuggestions
may be undefined, or defined from star imports
272-272:
UpdateTopic
may be undefined, or defined from star imports
288-288:
LabeledFrame
may be undefined, or defined from star imports
295-295:
UpdateTopic
may be undefined, or defined from star imports
302-302:
VideoFrameSuggestions
may 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:
OpenProject
may be undefined, or defined from star imports
348-348: Avoid equality comparisons to
True
; useif win.state["project_loaded"]:
for truth checks
355-355:
OpenProject
may be undefined, or defined from star imports
377-377:
OpenProject
may be undefined, or defined from star imports
390-390:
sys
may be undefined, or defined from star imports
392-392:
Labels
may be undefined, or defined from star importssleap/io/format/labels_json.py
154-154: Local variable
ex
is assigned to but never used
224-224: Local variable
ex
is 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_skel
is assigned to but never usedsleap/gui/dataviews.py
25-25:
typing.Dict
imported but unused
25-25:
typing.Type
imported but unused
29-29:
sleap.gui.color.ColorManager
imported but unused
30-30:
sleap.io.dataset.Labels
imported but unused
31-31:
sleap.instance.Instance
imported but unused
118-118: Do not use bare
except
sleap/gui/widgets/docks.py
15-15: Redefinition of unused
QLabel
from line 12
16-16: Redefinition of unused
QComboBox
from line 8
18-18: Redefinition of unused
QGroupBox
from line 10tests/io/test_dataset.py
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_json
is assigned to but never used
708-708: Local variable
new_labels_json
is assigned to but never used
1495-1495: Local variable
track_insts
is assigned to but never usedsleap/io/video.py
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()
tests/gui/test_commands.py
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_frame
is 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
sleap/instance.py
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
Labels
sleap/gui/app.py
261-261: Local variable
filename
is assigned to but never usedsleap/gui/widgets/video.py
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_bounds
is assigned to but never usedsleap/io/cameras.py
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
605-605: Undefined name
Skeleton
836-836: Do not use bare
except
1010-1010: Undefined name
Labels
sleap/io/dataset.py
56-56: Redefinition of unused
Callable
from line 46
62-62:
h5py
imported but unused
70-70: Do not use bare
except
71-71:
typing._ForwardRef
imported but unused
393-393: Do not assign a
lambda
expression, use adef
395-398: Do not assign a
lambda
expression, use adef
400-403: Do not assign a
lambda
expression, 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
e
is assigned to but never used
2721-2721: f-string without any placeholders
sleap/gui/commands.py
56-56:
sleap.io.cameras.Camcorder
imported but unused
196-196: Undefined name
MainWindow
868-868: Local variable
file_dir
is assigned to but never used
1763-1763: Do not use bare
except
1784-1784: Do not use bare
except
2557-2557: f-string without any placeholders
2993-2993: f-string without any placeholders
3356-3356: Do not use bare
except
Shellcheck
.conda/build.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
LanguageTool
docs/installation.md
[grammar] ~3-~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...
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...l should give you two extra commands to run listed under Next Steps. ````{n...
[misspelling] ~183-~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 ...
[grammar] ~244-~244: Did you mean the proper noun “Apple Silicon”?
Context: ...This works on **any OS except Apple silicon** and on **Google Colab**.
{note...
[uncategorized] ~268-~268: “you” seems less likely than “you’re” (you are).
Context: ... ## Testing that things are working If you installed usingmamba
, first activate...
[uncategorized] ~310-~310: “you” seems less likely than “you’re” (you are).
Context: ....0.19041-SP0 ### GPU support Assuming you installed using either of themamba
-b...
[style] ~416-~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...
[style] ~422-~422: Consider using “unable” to avoid wordiness.
Context: ...`bash sleap-diagnostic ``` If you were not able to get SLEAP installed, activate the ma...
Markdownlint
docs/installation.md
31-31: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
92-92: null
Bare URL used
301-301: Expected: fenced; Actual: indented
Code block style
320-320: Expected: fenced; Actual: indented
Code block style
339-339: Expected: fenced; Actual: indented
Code block style
Additional comments not posted (43)
tests/conftest.py (2)
14-14
: LGTM! The addition of camera fixtures is aligned with the PR's focus on enhancing testing capabilities.
Line range hint
4-4
: Verify the usage ofpytestqt
to ensure it's not an unused import.Verification successful
The
pytestqt
import is used in a logging statement withintests/conftest.py
. This indicates that the import is not entirely unused but is conditionally used for logging purposes.
tests/conftest.py
:logging.warning("Could not import pytestqt, skipping GUI tests.")
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `pytestqt` across the test suite. # Test: Search for `pytestqt` usage. Expect: At least one occurrence outside of imports. rg --type python --glob 'tests/**/*.py' 'pytestqt'Length of output: 81
Script:
#!/bin/bash # Description: Verify the usage of `pytestqt` across the test suite. # Test: Search for `pytestqt` usage. Expect: At least one occurrence outside of imports. rg --glob 'tests/**/*.py' 'pytestqt'Length of output: 162
.conda_mac/build.sh (1)
12-14
: The script effectively handles the uninstallation and reinstallation of OpenCV packages to resolve version conflicts. Consider adding a comment explaining whyopencv-contrib-python<4.7.0
is specifically chosen..conda/bld.bat (1)
12-14
: The script effectively handles the uninstallation and reinstallation of OpenCV packages on Windows to resolve version conflicts. Consider adding a comment explaining whyopencv-contrib-python<4.7.0
is specifically chosen.requirements.txt (2)
9-9
: The addition ofsleap-anipose
aligns with the PR's focus on enhancing session management and triangulation functionalities.
17-28
: The detailed instructions for handling OpenCV packages across different platforms are well-documented and ensure compatibility. Consider adding a rationale for choosingopencv-contrib-python<4.7.0
specifically..conda/build.sh (1)
12-14
: The script effectively handles the uninstallation and reinstallation of OpenCV packages to resolve version conflicts. Consider adding a comment explaining whyopencv-contrib-python<4.7.0
is specifically chosen..github/workflows/ci.yml (1)
95-100
: The addition of a step to handle OpenCV packages in the CI workflow ensures consistency with the development environment. Consider adding a comment explaining whyopencv-contrib-python<4.7.0
is specifically chosen.sleap/gui/shortcuts.py (1)
44-45
: LGTM! The addition of "goto next view" and "goto prev view" shortcuts aligns with the existing pattern and should work seamlessly.tests/gui/test_dataviews.py (2)
73-98
: LGTM! The new tests for the sessions table are comprehensive and effectively check the functionality of theSessionsTableModel
.
Line range hint
112-171
: LGTM! The new tests for the cameras table are thorough and validate the functionality of theCamerasTableModel
.sleap/util.py (2)
36-66
: The_DeepIterableConverter
class is well implemented with appropriate type checking and conversion logic. The use ofattr.s
decorator enhances readability and ensures that the class is hashable and has slots.
68-82
: The functiondeep_iterable_converter
is correctly implemented to handle deep conversion of iterables. It uses the_DeepIterableConverter
class effectively.sleap/gui/dataviews.py (2)
389-398
: TheSessionsTableModel
class correctly implements the required methods for a table model. Ensure that thehash
function provides a unique and consistent ID for sessions across application restarts.
668-684
: TheCamerasTableModel
class is well-implemented. It correctly fetches and displays camera and video data from aRecordingSession
. Good use of model-view separation.sleap/gui/widgets/docks.py (1)
576-733
: TheSessionsDock
class is well-structured and modular, effectively managing sessions, cameras, and unlinked videos. Ensure that all methods are implemented as described in the TODO comments.tests/io/test_cameras.py (9)
19-60
: Comprehensive tests forCamcorder
data structure.
62-131
: Comprehensive tests forCameraCluster
data structure.
133-277
: Comprehensive tests forRecordingSession
data structure.
279-295
: Tests forget_videos_from_selected_cameras
method inRecordingSession
.
297-311
: Tests forRecordingSession.remove_video
method.
313-394
: Tests forRecordingSession
integration withFrameGroup
andInstanceGroup
.
396-452
: Method to create anInstanceGroup
for testing.
455-556
: Comprehensive tests forInstanceGroup
data structure.
645-848
: Comprehensive tests forFrameGroup
data structure.tests/gui/test_commands.py (6)
930-957
: Addition oftest_AddSession
function.This test function properly checks the functionality of adding sessions, both when no session is selected and when a session is already selected. The assertions are correctly placed to verify the expected outcomes.
959-993
: Addition oftest_TriangulateSession_has_enough_instances
function.This test function effectively checks the
TriangulateSession.has_enough_instances
method. It tests various scenarios including the removal of instances to ensure the method behaves as expected under different conditions.
996-1104
: Addition oftest_TriangulateSession_do_action
function.This test function comprehensively tests the
TriangulateSession.do_action
method with detailed scenarios, including triangulation with all instances and selective instance triangulation. The use of numpy assertions to check the changes in instance group data is a robust approach.
1108-1284
: Addition oftest_SetSelectedInstanceGroup
function.This test function checks the functionality of setting a new instance group. It includes thorough testing with various preconditions and asserts the correct updates in the instance groups and their associations.
1287-1390
: Addition oftest_AddInstanceGroup
function.This test function evaluates the addition of new instance groups under various conditions. It ensures that the function behaves correctly when no session, frame index, or instance is selected, and checks the integrity of the instance groups after additions.
1392-1453
: Addition oftest_DeleteInstanceGroup
function.This test function effectively tests the deletion of instance groups. It handles different scenarios to ensure that the deletion process updates the system's state correctly, including the removal of all instance groups.
sleap/instance.py (2)
34-34
: Logger initialized for the module. Ensure that it is used appropriately throughout the module to log necessary information, especially in error handling and critical operations.
720-758
: The methodupdate_points
is well-implemented with clear documentation and logical checks. However, consider adding error handling for potential exceptions during the point updates.sleap/gui/app.py (9)
74-74
: Added import ofSessionsDock
fromsleap.gui.widgets.docks
.This import is necessary for the new session management features.
146-146
: Addedself.state["session"]
in the__init__
method.This state management addition is crucial for handling session-specific data throughout the application lifecycle.
329-342
: Modifiedswitch_frame
method to maintain the same frame index when switching videos if possible.This change enhances user experience by attempting to keep the user on the same frame index when switching videos, which is useful in multi-video analysis scenarios.
486-492
: AddedaddSession
method in thefileMenu
section.This addition allows users to add recording sessions directly from the file menu, improving accessibility and workflow efficiency.
582-593
: AddednextView
andprevView
methods in thegoMenu
section.These methods enhance navigation capabilities within the GUI, allowing users to move between different views more efficiently.
1050-1050
: Addedsessions_dock
in the_create_dock_windows
method.This change integrates the session management dock into the GUI, aligning with the new functionalities introduced in this update.
1113-1113
: Addedhas_selected_session
state check.This state check is necessary for enabling or disabling GUI components based on whether a session is selected, which is part of the new session management feature.
1207-1210
: Updated session handling in theon_data_update
method.This update ensures that the session-related GUI components are refreshed appropriately when session data changes.
1274-1280
: Enhanced session menu updating logic in the_update_sessions_menu
method.This enhancement improves the dynamic updating of session-related menus based on the current frame index and available session data.
sleap/gui/commands.py (1)
45-45
: Remove unused import ofsleap.io.cameras.Camcorder
.- from sleap.io.cameras import Camcorder, FrameGroup, InstanceGroup, RecordingSession + from sleap.io.cameras import FrameGroup, InstanceGroup, RecordingSessionLikely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-gui-elements-for-sessions #1789 +/- ##
=======================================================================
- Coverage 74.11% 74.10% -0.01%
=======================================================================
Files 135 135
Lines 25306 25309 +3
=======================================================================
Hits 18755 18755
- Misses 6551 6554 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes to be made to get this working. Also, this seems testable. So let's also add some tests.
sleap/gui/app.py
Outdated
|
||
|
||
if __name__ == "__main__": | ||
ds = os.environ["dsmview"] | ||
main([ds]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if __name__ == "__main__": | |
ds = os.environ["dsmview"] | |
main([ds]) |
…com/talmolab/sleap into ramiz/add-feature-maintain-same-branch
…ub.com/talmolab/sleap into ramiz/add-feature-maintain-same-branch
c76a030
into
liezl/add-gui-elements-for-sessions
Description
Includes modifications to the switch_frame function and the GoAdjacentView class in the sleap/gui/app.py file. The changes ensure that when switching videos, the application attempts to maintain the current frame index if the new video has enough frames
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️