Goto instance change key binding #2148
Conversation
WalkthroughThe changes introduce two new keyboard shortcuts in the configuration file Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Interface (Menu/Shortcut)
participant MW as MainWindow
participant CC as CommandContext
participant GN as GoNextInstanceChange
participant GP as GoPrevInstanceChange
participant V as Video/Data
U->>UI: Triggers "goto next instance change"
UI->>MW: Activates menu/shortcut command
MW->>CC: Calls nextInstanceChange()
CC->>GN: Executes GoNextInstanceChange.do_action(context, params)
GN->>V: Retrieves current frame and instance count
V-->>GN: Returns current data
GN->>V: Searches for next frame with a different instance count
V-->>GN: Returns target frame index
GN->>CC: Issues navigation instruction
CC-->>MW: Updates UI to target frame
MW-->>U: Displays the new instance change
U->>UI: Triggers "goto prev instance change"
UI->>MW: Activates menu/shortcut command
MW->>CC: Calls prevInstanceChange()
CC->>GP: Executes GoPrevInstanceChange.do_action(context, params)
GP->>V: Retrieves current frame and instance count
V-->>GP: Returns current data
GP->>V: Searches for previous frame with a different instance count
V-->>GP: Returns target frame index
GP->>CC: Issues navigation instruction
CC-->>MW: Updates UI to target frame
MW-->>U: Displays the new instance change
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/commands.py (1)
1608-1629: Consider handling edge cases and optimizing performanceThe implementation works well for the main use case, but could benefit from a few improvements:
Converting the iterator to a list with
list(later_frames_list)is unnecessary and could be memory-intensive for large datasets. Consider iterating directly throughlater_frames.There's no feedback when no frames with different instance counts are found. Consider adding a status message in this case.
For better error handling, check if
current_frameexists before accessing its instances.class GoNextInstanceChange(NavCommand): @classmethod def do_action(cls, context: CommandContext, params: dict): video = context.state["video"] cur_idx = context.state["frame_idx"] # Get current number of instances current_frames = context.labels.find(video, cur_idx, return_new=True) if not current_frames: + context.app.updateStatusMessage("No current frame found.") + return current_frame = current_frames[0] current_instance_count = len(current_frame.instances) # Get all labeled frames after current frame later_frames = context.labels.frames(video, from_frame_idx=cur_idx + 1) - later_frames_list = list(later_frames) # Find first frame where instance count changes found_change = False - for frame in later_frames_list: + for frame in later_frames: instance_count = len(frame.instances) if instance_count != current_instance_count: cls.go_to(context, frame.frame_idx) + found_change = True break + if not found_change: + context.app.updateStatusMessage("No frames with different instance counts found.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sleap/config/shortcuts.yaml(1 hunks)sleap/gui/app.py(1 hunks)sleap/gui/commands.py(2 hunks)sleap/gui/shortcuts.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
sleap/gui/commands.py (3)
sleap/io/dataset.py (5)
video(569-579)labels(553-555)find(933-961)instances(1156-1172)frames(963-979)sleap/instance.py (5)
video(908-913)find(1502-1521)instances(1524-1526)instances(1529-1550)frame_idx(916-921)sleap/gui/app.py (2)
labels(293-294)labels(297-298)
sleap/gui/app.py (1)
sleap/gui/commands.py (1)
nextInstanceChange(635-637)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macos-14)
- GitHub Check: Tests (windows-2022)
- GitHub Check: Tests (ubuntu-22.04)
🔇 Additional comments (4)
sleap/config/shortcuts.yaml (1)
21-21: LGTM: New shortcut for navigating to the next instance change.The shortcut
Ctrl+Shift+Rightfollows the existing pattern in the file - similar to other navigation shortcuts like "goto next labeled" and appropriately uses the directional key (Right) to indicate forward movement.sleap/gui/shortcuts.py (1)
44-44: LGTM: Added the shortcut name to the supported shortcuts list.The new "goto next instance change" entry is correctly added to the
_namestuple in theShortcutsclass, allowing it to be recognized and managed by the application's shortcut system.sleap/gui/app.py (1)
578-583: LGTM: Added menu item for navigating to the next instance change.The new menu item is correctly integrated into the "Go" menu alongside other navigation-related options. The implementation follows the established pattern for menu items, using the
add_menu_itemhelper function with appropriate parameters.sleap/gui/commands.py (1)
635-638: LGTM: Well-documented command methodThe new method is clear and follows the established pattern for command methods in this class.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2148 +/- ##
===========================================
+ Coverage 75.43% 76.17% +0.73%
===========================================
Files 134 134
Lines 24749 24837 +88
===========================================
+ Hits 18670 18920 +250
+ Misses 6079 5917 -162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/gui/test_commands.py (1)
1052-1079: Well-implemented test for the new navigation feature.The test thoroughly verifies the functionality of
GoNextInstanceChangeby:
- Setting up frames with different instance counts
- Executing the command
- Verifying it correctly navigates to the frame with a different instance count
Consider adding additional tests for edge cases:
- What happens when there are no frames with different instance counts?
- Behavior when there are multiple frames with different counts
def test_go_next_instance_change(centered_pair_predictions: Labels): """Test that goto next instance change command works correctly.""" # Set up test data labels = centered_pair_predictions video = labels.videos[0] # Create frames with different numbers of instances lf1 = labels[0] # First frame (already exists) initial_count = len(lf1.instances) # Create a new frame with different number of instances lf2 = LabeledFrame(frame_idx=lf1.frame_idx + 10, video=video) new_instance = Instance(labels.skeleton) # Create one new instance lf2.add_instance(new_instance) labels.add_frame(lf2) # Set up command context context = CommandContext.from_labels(labels) context.state["video"] = video context.state["frame_idx"] = lf1.frame_idx # Execute the command context.execute(GoNextInstanceChange) # Verify that we moved to the frame with different instance count assert context.state["frame_idx"] == lf2.frame_idx assert len(labels.find(video, lf2.frame_idx)[0].instances) != initial_count + + # Additional test: Navigate back to original frame + context.state["frame_idx"] = lf2.frame_idx + # Create a third frame with same instance count as the first + lf3 = LabeledFrame(frame_idx=lf2.frame_idx + 10, video=video) + for _ in range(initial_count): + lf3.add_instance(Instance(labels.skeleton)) + labels.add_frame(lf3) + + # Execute the command again + context.execute(GoNextInstanceChange) + + # Verify that we moved to the frame with different instance count + assert context.state["frame_idx"] == lf3.frame_idx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/gui/test_commands.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/gui/test_commands.py (5)
sleap/gui/commands.py (3)
GoNextInstanceChange(1608-1627)labels(208-210)CommandContext(177-637)tests/fixtures/datasets.py (1)
centered_pair_predictions(40-41)sleap/io/dataset.py (5)
Labels(389-2710)labels(553-555)video(569-579)instances(1156-1172)find(933-961)sleap/instance.py (7)
video(908-913)instances(1524-1526)instances(1529-1550)LabeledFrame(1429-1941)frame_idx(916-921)Instance(344-991)find(1502-1521)sleap/io/videowriter.py (3)
add_frame(26-27)add_frame(67-70)add_frame(121-124)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macos-14)
- GitHub Check: Tests (windows-2022)
- GitHub Check: Tests (ubuntu-22.04)
🔇 Additional comments (1)
tests/gui/test_commands.py (1)
25-25: Good job adding the required import for the test.The new import of
GoNextInstanceChangeis correctly added to the import list from sleap.gui.commands.
…almolab/sleap into elise/jump-to-track-num-change
sleap/gui/commands.py
Outdated
|
|
||
| # Get current number of instances | ||
| current_frame = context.labels.find(video, cur_idx, return_new=True)[0] | ||
| current_instance_count = len(current_frame.instances) |
There was a problem hiding this comment.
LabeledFrame.instances returns both the user instances and underlying predictions (i.e. when a user double clicks a prediction to correct it, the prediction will still be saved in the instances list, but the user only sees instance_to_show:
Lines 1677 to 1698 in 6531447
sleap/gui/commands.py
Outdated
|
|
||
| # Find first frame where instance count changes | ||
| for frame in later_frames_list: | ||
| instance_count = len(frame.instances) |
There was a problem hiding this comment.
Same here: instances are not necessarily the Instances the user is seeing in the GUI.
sleap/gui/commands.py
Outdated
| # Get all labeled frames after current frame | ||
| later_frames = context.labels.frames(video, from_frame_idx=cur_idx + 1) |
There was a problem hiding this comment.
If I am at a frame with n>0 instances, I would expect it to jump to a frame where there are 0 instances as well since that is a number of instances change. However, Labels.labeled_frames only stores labeled frames with instances>0.
| @@ -18,6 +18,7 @@ goto next user: Ctrl+U | |||
| goto next labeled: Alt+Right | |||
| goto prev suggestion: Shift+Space | |||
| goto prev labeled: Alt+Left | |||
| goto next instance change: Ctrl+Shift+Right | |||
There was a problem hiding this comment.
Would also be nice to go the other direction please and thank you!
Co-authored-by: Liezl Maree <38435167+roomrys@users.noreply.github.com>
…almolab/sleap into elise/jump-to-track-num-change
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sleap/gui/commands.py (2)
1608-1631: Refactor repeated calls for clarity and efficiency.Within the loop, the code repeatedly calls:
context.labels.find(video, frame, return_new=True)[0].instances_to_showStoring the labeled frame in a variable will improve readability and avoid recalculating the same object repeatedly:
for frame in range(cur_idx + 1, video.frames): - if len(context.labels.find(video, frame, return_new=True)[0].instances_to_show) - != current_instance_count: + next_frame = context.labels.find(video, frame, return_new=True)[0] + if len(next_frame.instances_to_show) != current_instance_count: cls.go_to(context, frame) break
1632-1654: Update the comment to reflect reverse iteration.The comment says “Get all labeled frames after current frame,” but this code block is iterating backward. Consider adjusting or removing the comment for accuracy.
tests/gui/test_commands.py (2)
1057-1070: Add error handling for no instance count changesThe test assumes there will always be a frame with a different instance count, but doesn't handle the case where none is found.
Consider adding a fallback or assertion if no frame with a different instance count is found:
# Create frames with different numbers of instances lf1 = context.labels.find(video, 0, return_new=True)[ 0 ] # First frame (already exists) initial_count = len(lf1.instances_to_show) + skipped_to_frame = None for frame_idx in range(1, video.num_frames): lf = context.labels.find(video, frame_idx, return_new=True)[0] if len(lf.instances_to_show) != initial_count: skipped_to_frame = lf.frame_idx break + + # Skip test if no frame with different instance count is found + if skipped_to_frame is None: + pytest.skip("No frame with different instance count found")🧰 Tools
🪛 Ruff (0.8.2)
1060-1060: Undefined name
context(F821)
1066-1066: Undefined name
context(F821)
1084-1085: Fix docstring in test_go_prev_instance_changeThe docstring incorrectly states "goto next instance change" instead of "goto prev instance change".
def test_go_prev_instance_change(centered_pair_predictions: Labels): - """Test that goto next instance change command works correctly.""" + """Test that goto previous instance change command works correctly."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sleap/config/shortcuts.yaml(1 hunks)sleap/gui/app.py(1 hunks)sleap/gui/commands.py(2 hunks)sleap/gui/shortcuts.py(1 hunks)tests/gui/test_commands.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sleap/gui/shortcuts.py
- sleap/config/shortcuts.yaml
🧰 Additional context used
🧬 Code Definitions (3)
sleap/gui/app.py (1)
sleap/gui/commands.py (1)
nextInstanceChange(635-637)
tests/gui/test_commands.py (2)
sleap/gui/commands.py (4)
GoNextInstanceChange(1608-1629)GoPrevInstanceChange(1632-1653)labels(208-210)CommandContext(177-637)tests/fixtures/datasets.py (1)
centered_pair_predictions(40-41)
sleap/gui/commands.py (2)
sleap/io/dataset.py (3)
video(569-579)find(933-961)frames(963-979)sleap/instance.py (3)
video(908-913)find(1502-1521)instances_to_show(1678-1698)
🪛 Ruff (0.8.2)
tests/gui/test_commands.py
1060-1060: Undefined name context
(F821)
1066-1066: Undefined name context
(F821)
1091-1091: Undefined name context
(F821)
1097-1097: Undefined name context
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macos-14)
- GitHub Check: Tests (windows-2022)
- GitHub Check: Tests (ubuntu-22.04)
🔇 Additional comments (3)
sleap/gui/app.py (1)
578-590: Well-integrated navigation feature for instance count changes.The new menu items seamlessly integrate with the existing navigation options in the "Go" menu. The addition enhances the user experience by providing a way to quickly navigate between frames where the instance count changes.
The implementation follows the established pattern of using
add_menu_itemwith appropriate command references, and the menu item labels are clear and descriptive.sleap/gui/commands.py (1)
635-638: Looks clean and straightforward.This method neatly delegates the logic to the
GoNextInstanceChangecommand.tests/gui/test_commands.py (1)
25-26: LGTM: Command imports added properlyThe imports for the new command classes are correctly added to support the new tests.
tests/gui/test_commands.py
Outdated
| def test_go_next_instance_change(centered_pair_predictions: Labels): | ||
| """Test that goto next instance change command works correctly.""" | ||
| # Set up test data | ||
| labels = centered_pair_predictions | ||
| video = labels.videos[0] | ||
|
|
||
| # Create frames with different numbers of instances | ||
| lf1 = context.labels.find(video, 0, return_new=True)[ | ||
| 0 | ||
| ] # First frame (already exists) | ||
| initial_count = len(lf1.instances_to_show) | ||
|
|
||
| for frame_idx in range(1, video.num_frames): | ||
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | ||
| if len(lf.instances_to_show) != initial_count: | ||
| skipped_to_frame = lf.frame_idx | ||
| break | ||
|
|
||
| # Set up command context | ||
| context = CommandContext.from_labels(labels) | ||
| context.state["video"] = video | ||
| context.state["frame_idx"] = lf1.frame_idx | ||
|
|
||
| # Execute the command | ||
| context.execute(GoNextInstanceChange) | ||
|
|
||
| # Verify that we moved to the frame with different instance count | ||
| assert context.state["frame_idx"] == skipped_to_frame | ||
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count | ||
|
|
There was a problem hiding this comment.
Fix undefined variable error
This test function references context before it's defined, which will cause a NameError when the test runs.
def test_go_next_instance_change(centered_pair_predictions: Labels):
"""Test that goto next instance change command works correctly."""
# Set up test data
labels = centered_pair_predictions
video = labels.videos[0]
+ # Set up command context
+ context = CommandContext.from_labels(labels)
+ context.state["video"] = video
+ context.state["frame_idx"] = 0
# Create frames with different numbers of instances
lf1 = context.labels.find(video, 0, return_new=True)[
0
] # First frame (already exists)
initial_count = len(lf1.instances_to_show)
for frame_idx in range(1, video.num_frames):
lf = context.labels.find(video, frame_idx, return_new=True)[0]
if len(lf.instances_to_show) != initial_count:
skipped_to_frame = lf.frame_idx
break
- # Set up command context
- context = CommandContext.from_labels(labels)
- context.state["video"] = video
- context.state["frame_idx"] = lf1.frame_idx
+ # Update frame index to start position
+ context.state["frame_idx"] = lf1.frame_idx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_go_next_instance_change(centered_pair_predictions: Labels): | |
| """Test that goto next instance change command works correctly.""" | |
| # Set up test data | |
| labels = centered_pair_predictions | |
| video = labels.videos[0] | |
| # Create frames with different numbers of instances | |
| lf1 = context.labels.find(video, 0, return_new=True)[ | |
| 0 | |
| ] # First frame (already exists) | |
| initial_count = len(lf1.instances_to_show) | |
| for frame_idx in range(1, video.num_frames): | |
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | |
| if len(lf.instances_to_show) != initial_count: | |
| skipped_to_frame = lf.frame_idx | |
| break | |
| # Set up command context | |
| context = CommandContext.from_labels(labels) | |
| context.state["video"] = video | |
| context.state["frame_idx"] = lf1.frame_idx | |
| # Execute the command | |
| context.execute(GoNextInstanceChange) | |
| # Verify that we moved to the frame with different instance count | |
| assert context.state["frame_idx"] == skipped_to_frame | |
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count | |
| def test_go_next_instance_change(centered_pair_predictions: Labels): | |
| """Test that goto next instance change command works correctly.""" | |
| # Set up test data | |
| labels = centered_pair_predictions | |
| video = labels.videos[0] | |
| # Set up command context | |
| context = CommandContext.from_labels(labels) | |
| context.state["video"] = video | |
| context.state["frame_idx"] = 0 | |
| # Create frames with different numbers of instances | |
| lf1 = context.labels.find(video, 0, return_new=True)[0] # First frame (already exists) | |
| initial_count = len(lf1.instances_to_show) | |
| for frame_idx in range(1, video.num_frames): | |
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | |
| if len(lf.instances_to_show) != initial_count: | |
| skipped_to_frame = lf.frame_idx | |
| break | |
| # Update frame index to start position | |
| context.state["frame_idx"] = lf1.frame_idx | |
| # Execute the command | |
| context.execute(GoNextInstanceChange) | |
| # Verify that we moved to the frame with different instance count | |
| assert context.state["frame_idx"] == skipped_to_frame | |
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count |
🧰 Tools
🪛 Ruff (0.8.2)
1060-1060: Undefined name context
(F821)
1066-1066: Undefined name context
(F821)
tests/gui/test_commands.py
Outdated
| def test_go_prev_instance_change(centered_pair_predictions: Labels): | ||
| """Test that goto next instance change command works correctly.""" | ||
| # Set up test data | ||
| labels = centered_pair_predictions | ||
| video = labels.videos[0] | ||
|
|
||
| # Create frames with different numbers of instances | ||
| lf1 = context.labels.find(video, video.num_frames - 1, return_new=True)[ | ||
| 0 | ||
| ] # First frame (already exists) | ||
| initial_count = len(lf1.instances_to_show) | ||
|
|
||
| for frame_idx in range(video.num_frames - 1, 0, -1): | ||
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | ||
| if len(lf.instances_to_show) != initial_count: | ||
| skipped_to_frame = lf.frame_idx | ||
| break | ||
|
|
||
| # Set up command context | ||
| context = CommandContext.from_labels(labels) | ||
| context.state["video"] = video | ||
| context.state["frame_idx"] = lf1.frame_idx | ||
|
|
||
| # Execute the command | ||
| context.execute(GoPrevInstanceChange) | ||
|
|
||
| # Verify that we moved to the frame with different instance count | ||
| assert context.state["frame_idx"] == skipped_to_frame | ||
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count |
There was a problem hiding this comment.
Fix undefined variable error
Similar to the previous test, this function references context before it's defined, which will cause a NameError when the test runs.
def test_go_prev_instance_change(centered_pair_predictions: Labels):
"""Test that goto next instance change command works correctly."""
# Set up test data
labels = centered_pair_predictions
video = labels.videos[0]
+ # Set up command context
+ context = CommandContext.from_labels(labels)
+ context.state["video"] = video
+ context.state["frame_idx"] = video.num_frames - 1
# Create frames with different numbers of instances
lf1 = context.labels.find(video, video.num_frames - 1, return_new=True)[
0
] # First frame (already exists)
initial_count = len(lf1.instances_to_show)
for frame_idx in range(video.num_frames - 1, 0, -1):
lf = context.labels.find(video, frame_idx, return_new=True)[0]
if len(lf.instances_to_show) != initial_count:
skipped_to_frame = lf.frame_idx
break
- # Set up command context
- context = CommandContext.from_labels(labels)
- context.state["video"] = video
- context.state["frame_idx"] = lf1.frame_idx
+ # Update frame index to start position
+ context.state["frame_idx"] = lf1.frame_idx📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_go_prev_instance_change(centered_pair_predictions: Labels): | |
| """Test that goto next instance change command works correctly.""" | |
| # Set up test data | |
| labels = centered_pair_predictions | |
| video = labels.videos[0] | |
| # Create frames with different numbers of instances | |
| lf1 = context.labels.find(video, video.num_frames - 1, return_new=True)[ | |
| 0 | |
| ] # First frame (already exists) | |
| initial_count = len(lf1.instances_to_show) | |
| for frame_idx in range(video.num_frames - 1, 0, -1): | |
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | |
| if len(lf.instances_to_show) != initial_count: | |
| skipped_to_frame = lf.frame_idx | |
| break | |
| # Set up command context | |
| context = CommandContext.from_labels(labels) | |
| context.state["video"] = video | |
| context.state["frame_idx"] = lf1.frame_idx | |
| # Execute the command | |
| context.execute(GoPrevInstanceChange) | |
| # Verify that we moved to the frame with different instance count | |
| assert context.state["frame_idx"] == skipped_to_frame | |
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count | |
| def test_go_prev_instance_change(centered_pair_predictions: Labels): | |
| """Test that goto next instance change command works correctly.""" | |
| # Set up test data | |
| labels = centered_pair_predictions | |
| video = labels.videos[0] | |
| # Set up command context | |
| context = CommandContext.from_labels(labels) | |
| context.state["video"] = video | |
| context.state["frame_idx"] = video.num_frames - 1 | |
| # Create frames with different numbers of instances | |
| lf1 = context.labels.find(video, video.num_frames - 1, return_new=True)[0] # First frame (already exists) | |
| initial_count = len(lf1.instances_to_show) | |
| for frame_idx in range(video.num_frames - 1, 0, -1): | |
| lf = context.labels.find(video, frame_idx, return_new=True)[0] | |
| if len(lf.instances_to_show) != initial_count: | |
| skipped_to_frame = lf.frame_idx | |
| break | |
| # Update frame index to start position | |
| context.state["frame_idx"] = lf1.frame_idx | |
| # Execute the command | |
| context.execute(GoPrevInstanceChange) | |
| # Verify that we moved to the frame with different instance count | |
| assert context.state["frame_idx"] == skipped_to_frame | |
| assert len(labels.find(video, skipped_to_frame)[0].instances) != initial_count |
🧰 Tools
🪛 Ruff (0.8.2)
1091-1091: Undefined name context
(F821)
1097-1097: Undefined name context
(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/commands.py (1)
1636-1658: Consider refactoring to eliminate code duplicationThe implementation for
GoPrevInstanceChangeis nearly identical toGoNextInstanceChangeexcept for the frame iteration direction. Consider refactoring to reduce duplication by creating a common helper method with a direction parameter.-class GoNextInstanceChange(NavCommand): +class InstanceChangeNavigator: + @staticmethod + def find_instance_change(context: CommandContext, direction: int = 1): + video = context.state["video"] + cur_idx = context.state["frame_idx"] + + # Get current number of instances + current_frame = context.labels.find(video, cur_idx, return_new=True)[0] + current_instance_count = len(current_frame.instances_to_show) + + # Determine frame range and iteration + start = cur_idx + direction + end = video.frames if direction > 0 else -1 + step = direction + + # Find frame with different instance count + for frame in range(start, end, step): + if ( + len( + context.labels.find(video, frame, return_new=True)[ + 0 + ].instances_to_show + ) + != current_instance_count + ): + return frame + return None + +class GoNextInstanceChange(NavCommand): @classmethod def do_action(cls, context: CommandContext, params: dict): - video = context.state["video"] - cur_idx = context.state["frame_idx"] - - # Get current number of instances - current_frame = context.labels.find(video, cur_idx, return_new=True)[0] - current_instance_count = len(current_frame.instances_to_show) - - # Get all labeled frames after current frame - for frame in range(cur_idx + 1, video.frames): - if ( - len( - context.labels.find(video, frame, return_new=True)[ - 0 - ].instances_to_show - ) - != current_instance_count - ): - cls.go_to(context, frame) - break + frame = InstanceChangeNavigator.find_instance_change(context, direction=1) + if frame is not None: + cls.go_to(context, frame) class GoPrevInstanceChange(NavCommand): @classmethod def do_action(cls, context: CommandContext, params: dict): - video = context.state["video"] - cur_idx = context.state["frame_idx"] - - # Get current number of instances - current_frame = context.labels.find(video, cur_idx, return_new=True)[0] - current_instance_count = len(current_frame.instances_to_show) - - # Get all labeled frames after current frame - for frame in range(cur_idx - 1, -1, -1): - if ( - len( - context.labels.find(video, frame, return_new=True)[ - 0 - ].instances_to_show - ) - != current_instance_count - ): - cls.go_to(context, frame) - break + frame = InstanceChangeNavigator.find_instance_change(context, direction=-1) + if frame is not None: + cls.go_to(context, frame)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/commands.py(2 hunks)tests/gui/test_commands.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/gui/test_commands.py (3)
sleap/gui/commands.py (4)
GoNextInstanceChange(1612-1633)GoPrevInstanceChange(1636-1657)labels(208-210)CommandContext(177-641)tests/fixtures/datasets.py (1)
centered_pair_predictions(40-41)sleap/instance.py (6)
video(908-913)find(1502-1521)instances_to_show(1678-1698)frame_idx(916-921)instances(1524-1526)instances(1529-1550)
sleap/gui/commands.py (2)
sleap/io/dataset.py (3)
video(569-579)find(933-961)frames(963-979)sleap/instance.py (3)
video(908-913)find(1502-1521)instances_to_show(1678-1698)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macos-14)
- GitHub Check: Tests (windows-2022)
- GitHub Check: Tests (ubuntu-22.04)
🔇 Additional comments (5)
sleap/gui/commands.py (2)
635-641: Good implementation of instance change navigation methodsThese methods provide a clean interface for navigating to frames where the number of instances changes, following the established pattern for navigation methods in the CommandContext class.
1612-1634: Well-structured implementation for navigating to next instance changeThe implementation correctly:
- Uses
instances_to_showto match what's visible in the GUI- Properly finds frames where instance count changes
- Handles edge cases by using
return_new=Truewhen finding labeled framesThis addresses the previous reviewer's concerns about ensuring frames with zero instances are included in the navigation.
tests/gui/test_commands.py (3)
25-26: Imports added correctly for new functionality.The necessary imports for
GoNextInstanceChangeandGoPrevInstanceChangeare properly added to support the new test functions.
1053-1080: Test implementation for next instance change navigation looks good.This test function correctly verifies the behavior of the
GoNextInstanceChangecommand by:
- Finding a frame with a different instance count
- Executing the command
- Verifying that the frame index changes to the expected frame
The test function has fixed the previous issue where
contextwas referenced before being defined.
1082-1110: Test implementation for previous instance change navigation looks good.This test function properly verifies the behavior of the
GoPrevInstanceChangecommand, starting from the last frame and moving backward to find a frame with a different instance count.Like the previous test, this also fixes the issue where
contextwas referenced before being defined.
Description
Adding a keybinding to jump to the next (or previous) frame where the number of displayed instances changes from the current frame
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit