(3a -> 3) Debug missing PredictedInstances (not displaying in GUI)
#1757
Conversation
WalkthroughThis update enhances the handling of instances within frame groups in the SLEAP project. It introduces refined logic for adding and removing instances, ensuring proper management and linking of these instances in various structures, thus improving the software's robustness and functionality in tracking scenarios. Changes
Possibly related issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Additional Context UsedRuff (34)
Additional comments not posted (2)
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 (
|
roomrys
changed the title
PredictedInstances (not displaying in GUI)PredictedInstances (not displaying in GUI)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (7)
sleap/io/cameras.py (4)
Line range hint
79-81: Consider removing unnecessary f-string usage as there are no placeholders.- logger.warning(f"{session} not found in {self}.") + logger.warning(f"{session} not found in this Camcorder.") - logger.warning(f"{video} not found in {self}.") + logger.warning(f"{video} not found in this Camcorder.")
Line range hint
448-448: The nameSkeletonis undefined in this scope, which will cause a runtime error.Please ensure that
Skeletonis defined or imported correctly in this file.
Line range hint
829-829: Avoid using a bareexceptstatement as it can catch unexpected exceptions and make debugging difficult.- except: + except Exception as e: + logger.error(f"Error occurred: {e}")
Line range hint
1003-1003: The nameLabelsis undefined in this scope, which will cause a runtime error.Please ensure that
Labelsis defined or imported correctly in this file.sleap/io/dataset.py (3)
Line range hint
61-61: Remove unused import ofh5py.- import h5py as h5
Line range hint
69-69: Replace bareexceptwith specific exceptions to avoid catching unexpected errors.- except: + except ImportError:
Line range hint
55-55: Remove duplicate import ofCallable.- from typing import ( - Callable, + from typing import (
| # 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 | ||
| ) | ||
|
|
There was a problem hiding this comment.
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.
| # 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.") |
| """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) | ||
|
|
There was a problem hiding this comment.
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.
| """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) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/asc-initial-update-instances-across-views #1757 +/- ##
===================================================================================
- Coverage 73.84% 73.78% -0.06%
===================================================================================
Files 135 135
Lines 24890 24919 +29
===================================================================================
+ Hits 18379 18387 +8
- Misses 6511 6532 +21 ☔ View full report in Codecov by Sentry. |
roomrys
merged commit 507eefa
into
liezl/asc-initial-update-instances-across-views
Description
TriangulateSessioncallsframe_group.upsert_pointswhich should createPredictedInstances for anyCamcorders that do not already have an assignedInstance(orPredictedIntance). However, after triangulating, thePredictedInstances were not being displayed in the GUI - even though the containingInstanceGroupappeared to have been filled in withPredictedInstances for each "missing" view.This PR does 3 things:
PredictedInstances to theLabeledFramePredictedInstances withInstances (when creating anInstancefrom aPredictedInstance)InstancewithPredictedInstance(when removing anInstancethat ha an associatedPredictedInstance)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