Skip to content
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

(3a -> 3) Debug missing PredictedInstances (not displaying in GUI) #1757

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions sleap/gui/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -2928,6 +2928,26 @@ def do_action(cls, context: CommandContext, params: dict):
if context.state["labeled_frame"] not in context.labels.labels:
context.labels.append(context.state["labeled_frame"])

# Also add the instance to the frame group if it exists
video = context.state["video"]
session = context.labels.get_session(video=video)
if session is None:
return

frame_idx = context.state["frame_idx"]
frame_group = session.frame_groups.get(frame_idx, None)
if frame_group is None:
return

instance_group = frame_group.get_instance_group(instance=from_predicted)
if instance_group is None:
return

camera = session.get_camera(video=video)
frame_group.add_instance(
instance=new_instance, camera=camera, instance_group=instance_group
)

Comment on lines +2931 to +2950
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the addition of new instances to InstanceGroup for clarity and error handling.

Consider refactoring this block to separate the concerns of checking for the existence of session, frame group, and instance group, and the actual addition of the instance. This can improve readability and maintainability. Additionally, consider adding error handling for cases where the session, frame group, or instance group does not exist.

-        if session is None:
-            return
-        frame_idx = context.state["frame_idx"]
-        frame_group = session.frame_groups.get(frame_idx, None)
-        if frame_group is None:
-            return
-        instance_group = frame_group.get_instance_group(instance=from_predicted)
-        if instance_group is None:
-            return
-        camera = session.get_camera(video=video)
-        frame_group.add_instance(
-            instance=new_instance, camera=camera, instance_group=instance_group
-        )
+        if session and frame_group and instance_group:
+            frame_idx = context.state["frame_idx"]
+            frame_group = session.frame_groups.get(frame_idx, None)
+            instance_group = frame_group.get_instance_group(instance=from_predicted)
+            camera = session.get_camera(video=video)
+            frame_group.add_instance(
+                instance=new_instance, camera=camera, instance_group=instance_group
+            )
+        else:
+            logger.error("Session, FrameGroup, or InstanceGroup not found.")

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.

Suggested change
# Also add the instance to the frame group if it exists
video = context.state["video"]
session = context.labels.get_session(video=video)
if session is None:
return
frame_idx = context.state["frame_idx"]
frame_group = session.frame_groups.get(frame_idx, None)
if frame_group is None:
return
instance_group = frame_group.get_instance_group(instance=from_predicted)
if instance_group is None:
return
camera = session.get_camera(video=video)
frame_group.add_instance(
instance=new_instance, camera=camera, instance_group=instance_group
)
# Also add the instance to the frame group if it exists
video = context.state["video"]
session = context.labels.get_session(video=video)
if session and frame_group and instance_group:
frame_idx = context.state["frame_idx"]
frame_group = session.frame_groups.get(frame_idx, None)
instance_group = frame_group.get_instance_group(instance=from_predicted)
camera = session.get_camera(video=video)
frame_group.add_instance(
instance=new_instance, camera=camera, instance_group=instance_group
)
else:
logger.error("Session, FrameGroup, or InstanceGroup not found.")

@staticmethod
def create_new_instance(
context: CommandContext,
Expand Down
15 changes: 9 additions & 6 deletions sleap/io/cameras.py
Original file line number Diff line number Diff line change
Expand Up @@ -1666,15 +1666,15 @@ def add_instance(
):
"""Add an (existing) `Instance` to the `FrameGroup`.

If no `InstanceGroup` is provided, then check the `Instance` is already in an
If no `InstanceGroup` is provided, then require the `Instance` is already in an
`InstanceGroup` contained in the `FrameGroup`. Otherwise, add the `Instance` to
the `InstanceGroup` and `FrameGroup`.
the both the `FrameGroup` and provided `InstanceGroup`.

Args:
instance: `Instance` to add to the `FrameGroup`.
camera: `Camcorder` to link the `Instance` to.
instance_group: `InstanceGroup` to add the `Instance` to. If None, then
check the `Instance` is already in an `InstanceGroup`.
require the `Instance` is already in `FrameGroup.instance_groups`.

Raises:
ValueError: If the `InstanceGroup` is not in the `FrameGroup`.
Expand Down Expand Up @@ -1721,9 +1721,9 @@ def remove_instance(self, instance: Instance):
instance_group = self.get_instance_group(instance=instance)

if instance_group is None:
logger.warning(
f"Instance {instance} not found in this FrameGroup.instance_groups: "
f"{self.instance_groups}."
logger.debug(
f"Instance to remove {instance} not found in this "
f"FrameGroup.instance_groups: {self.instance_groups}."
)
return

Expand Down Expand Up @@ -1974,6 +1974,9 @@ def _create_and_add_instance(
# Add the `Instance` to the `FrameGroup`
self._instances_by_cam[camera].add(instance)

# Add the `Instance` to the `RecordingSession`'s `Labels` (and `LabeledFrame`)
self.session.labels.add_instance(frame=labeled_frame, instance=instance)

def create_and_add_missing_instances(self, instance_group: InstanceGroup):
"""Add missing instances to `FrameGroup` from `InstanceGroup`s.

Expand Down
34 changes: 32 additions & 2 deletions sleap/io/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
import numpy as np
import datetime
from sklearn.model_selection import train_test_split
from sleap.io.cameras import RecordingSession
from sleap.io.cameras import FrameGroup, RecordingSession

try:
from typing import ForwardRef
Expand Down Expand Up @@ -1405,11 +1405,41 @@ def track_swap(
def remove_instance(
self, frame: LabeledFrame, instance: Instance, in_transaction: bool = False
):
"""Remove instance from frame, updating track occupancy."""
"""Remove instance from frame, updating track occupancy and `FrameGroup`."""

frame.instances.remove(instance)
if not in_transaction:
self._cache.remove_instance(frame, instance)

# Also remove instance from `InstanceGroup` if any
session = self.get_session(frame.video)
if session is None:
return # No session, so no `FrameGroup` to update
frame_group: FrameGroup = session.frame_groups.get(frame.frame_idx, None)
if frame_group is None:
return # No `FrameGroup` for this frame
video = frame.video
camera = session.get_camera(video=video)
if camera is None:
return # No camera, so no `InstanceGroup` to update

# If `Instance.from_predicted`, then replace with `PredictedInstance`
instance_group = frame_group.get_instance_group(instance=instance)
replace_instance = (instance.from_predicted is not None) and (
instance_group is not None
)

# Add the new instance to the `FrameGroup` and replace the old one
if replace_instance:
frame_group.add_instance(
instance=instance.from_predicted,
camera=camera,
instance_group=instance_group,
)
# Otherwise just remove the instance
else:
frame_group.remove_instance(instance=instance)

Comment on lines +1408 to +1442
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor remove_instance to simplify logic and improve readability.

-        if not in_transaction:
-            self._cache.remove_instance(frame, instance)
-
-        # Also remove instance from `InstanceGroup` if any
-        session = self.get_session(frame.video)
-        if session is None:
-            return  # No session, so no `FrameGroup` to update
-        frame_group: FrameGroup = session.frame_groups.get(frame.frame_idx, None)
-        if frame_group is None:
-            return  # No `FrameGroup` for this frame
-        video = frame.video
-        camera = session.get_camera(video=video)
-        if camera is None:
-            return  # No camera, so no `InstanceGroup` to update
-
-        # If `Instance.from_predicted`, then replace with `PredictedInstance`
-        instance_group = frame_group.get_instance_group(instance=instance)
-        replace_instance = (instance.from_predicted is not None) and (
-            instance_group is not None
-        )
-
-        # Add the new instance to the `FrameGroup` and replace the old one
-        if replace_instance:
-            frame_group.add_instance(
-                instance=instance.from_predicted,
-                camera=camera,
-                instance_group=instance_group,
-            )
-        # Otherwise just remove the instance
-        else:
-            frame_group.remove_instance(instance=instance)
+        if not in_transaction:
+            self._cache.remove_instance(frame, instance)
+
+        session = self.get_session(frame.video)
+        if session:
+            frame_group = session.frame_groups.get(frame.frame_idx, None)
+            if frame_group:
+                video = frame.video
+                camera = session.get_camera(video=video)
+                if camera:
+                    instance_group = frame_group.get_instance_group(instance=instance)
+                    if instance.from_predicted and instance_group:
+                        frame_group.add_instance(
+                            instance=instance.from_predicted,
+                            camera=camera,
+                            instance_group=instance_group,
+                        )
+                    else:
+                        frame_group.remove_instance(instance=instance)

This refactoring reduces the depth of conditional nesting and clarifies the conditions under which different actions are taken. It also combines checks to reduce redundancy and improve the flow of the method.


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.

Suggested change
"""Remove instance from frame, updating track occupancy and `FrameGroup`."""
frame.instances.remove(instance)
if not in_transaction:
self._cache.remove_instance(frame, instance)
# Also remove instance from `InstanceGroup` if any
session = self.get_session(frame.video)
if session is None:
return # No session, so no `FrameGroup` to update
frame_group: FrameGroup = session.frame_groups.get(frame.frame_idx, None)
if frame_group is None:
return # No `FrameGroup` for this frame
video = frame.video
camera = session.get_camera(video=video)
if camera is None:
return # No camera, so no `InstanceGroup` to update
# If `Instance.from_predicted`, then replace with `PredictedInstance`
instance_group = frame_group.get_instance_group(instance=instance)
replace_instance = (instance.from_predicted is not None) and (
instance_group is not None
)
# Add the new instance to the `FrameGroup` and replace the old one
if replace_instance:
frame_group.add_instance(
instance=instance.from_predicted,
camera=camera,
instance_group=instance_group,
)
# Otherwise just remove the instance
else:
frame_group.remove_instance(instance=instance)
"""Remove instance from frame, updating track occupancy and `FrameGroup`."""
frame.instances.remove(instance)
if not in_transaction:
self._cache.remove_instance(frame, instance)
session = self.get_session(frame.video)
if session:
frame_group = session.frame_groups.get(frame.frame_idx, None)
if frame_group:
video = frame.video
camera = session.get_camera(video=video)
if camera:
instance_group = frame_group.get_instance_group(instance=instance)
if instance.from_predicted and instance_group:
frame_group.add_instance(
instance=instance.from_predicted,
camera=camera,
instance_group=instance_group,
)
else:
frame_group.remove_instance(instance=instance)

def add_instance(self, frame: LabeledFrame, instance: Instance):
"""Add instance to frame, updating track occupancy."""
# Ensure that there isn't already an Instance with this track
Expand Down
Loading