(3e -> 3a) Add InstanceGroup class
#1618
Conversation
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
WalkthroughThe changes encompass a comprehensive overhaul of the camera-related functionalities in SLEAP. This includes refactoring imports, updating logic for handling instances and points, introducing new data structures, and refining the test suite to align with the enhanced codebase. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files not summarized due to errors (1)
Additional comments not posted (15)
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 base branch from
develop
to
liezl/add-method-for-multi-instance-products
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/io/cameras.py (2 hunks)
- tests/io/test_cameras.py (2 hunks)
Additional comments: 8
sleap/io/cameras.py (5)
5-5: The import of
castfrom thetypingmodule is consistent with the summary.5-5: The removal of
triangulatefrom thesleap_aniposeimport is consistent with the summary.5-5: The addition of new imports from
sleap.typesis consistent with the summary.5-5: The import of
Labelsfromsleap.io.datasetis consistent with the summary.759-899: The addition of the
InstanceGroupclass is consistent with the summary of the pull request, which indicates that this is a new feature.tests/io/test_cameras.py (3)
6-8: The import changes reflect the addition of the
InstanceGroupclass and the removal of unused imports, which aligns with the summary.283-320: The new test case for
InstanceGroupis well-structured and covers the creation of anInstanceGroupfrom a dictionary, as well as the retrieval of instances by camera and vice versa. This aligns with the introduction of the newInstanceGroupclass as described in the summary.278-320: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-320]
No further issues or improvements are identified in the provided hunks. The changes are consistent with the summary and the pull request description.
sleap/io/cameras.py
Outdated
| frame_idx = cast( | ||
| int, frame_idx | ||
| ) # Could be None if no real instances in dictionary |
There was a problem hiding this comment.
The use of cast to convert frame_idx to an integer may not be safe if frame_idx can be None. Consider adding a check to ensure frame_idx is not None before casting.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/add-method-to-match-instances-across-views #1618 +/- ##
====================================================================================
+ Coverage 73.81% 73.85% +0.03%
====================================================================================
Files 135 135
Lines 24636 24807 +171
====================================================================================
+ Hits 18186 18322 +136
- Misses 6450 6485 +35 ☔ View full report in Codecov by Sentry. |
…//github.com/talmolab/sleap into liezl/add-instance-group-class
…//github.com/talmolab/sleap into liezl/add-instance-group-class
|
…https://github.com/talmolab/sleap into liezl/add-method-to-test-instance-grouping
…github.com/talmolab/sleap into liezl/add-method-for-multi-instance-products
…//github.com/talmolab/sleap into liezl/add-instance-group-class
sleap/gui/commands.py
Outdated
| # Possible race condition | ||
| if selected_cam not in instances: | ||
| raise RuntimeError( | ||
| f"Selected camera {selected_cam} not in instances at frame {frame_idx}." | ||
| ) |
There was a problem hiding this comment.
While this will help prevent a KeyError if the instance has not yet been added from the selected_cam - this does not handle the case when an instance is already available for the selected_cam, but we double click another PredictedInstance in the same view.
roomrys
changed the base branch from
liezl/add-method-for-multi-instance-products
to
liezl/add-method-to-match-instances-across-views
…tps://github.com/talmolab/sleap into liezl/add-instance-group-class
roomrys
changed the title
InstanceGroup classInstanceGroup class
* Add FrameGroup class * Replace _get_instances_by_camera with maintained _instances_by_cam * Convert enforce_frame_idx_unique to regular method * Write (failing) FrameGroup test * Get FrameGroup.session validator working * Get rid of test_instance_group bug * Fix create_instance_group bug * Fix failing tests * Add update_points, get_instance_group, and numpy methods * Add `get_instances` method * Rename `update_points` to `upsert_points` and numpy multiple `InstanceGroup`s * Create and add missing instance when updating reprojections * Add method to add `Instance`s and `InstanceGroup`s * Create new InstanceGroup if instance_group is None in add_instance_group * (3g -> 3f) Use frame group for triangulation (#1693)
There was a problem hiding this comment.
Actionable comments posted: 49
Actionable comments outside the diff hunks (15)
tests/fixtures/datasets.py (1)
Line range hint
1-1: Remove unused import to clean up the code.- import ostests/gui/test_commands.py (13)
Line range hint
5-5: Remove unused importDict.- from typing import Dict, List + from typing import List
Line range hint
7-7: Remove unused importnumpy.- import numpy as np
Line range hint
21-21: Remove unused importTriangulateSession.- from sleap.gui.commands import ( - AddSession, - CommandContext, - ExportAnalysisFile, - ExportDatasetWithImages, - ImportDeepLabCutFolder, - OpenSkeleton, - RemoveVideo, - ReplaceVideo, - SaveProjectAs, - TriangulateSession, - get_new_version_filename, - ) + from sleap.gui.commands import ( + AddSession, + CommandContext, + ExportAnalysisFile, + ExportDatasetWithImages, + ImportDeepLabCutFolder, + OpenSkeleton, + RemoveVideo, + ReplaceVideo, + SaveProjectAs, + get_new_version_filename, + )
Line range hint
25-25: Remove unused importCamcorder.- from sleap.io.cameras import Camcorder
Line range hint
227-227: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
236-236: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
244-244: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
253-253: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
265-265: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
274-274: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
281-281: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
299-299: Useif okay:instead ofif okay is True:for truth checks.- assert okay is True + assert okay
Line range hint
528-528: Remove unnecessary f-string as it contains no placeholders.- fly32_json = get_package_file(f"skeletons/fly32.json") + fly32_json = get_package_file("skeletons/fly32.json")sleap/io/cameras.py (1)
Line range hint
80-82: Consider removing the f-string formatting as there are no placeholders used in these strings.- raise AttributeError( - f"No camera has been specified. " - f"This is likely because the `Camcorder.from_dict` method was not used to initialize this object. " - f"Please use `Camcorder.from_dict` to recreate the object." - ) + raise AttributeError( + "No camera has been specified. " + "This is likely because the `Camcorder.from_dict` method was not used to initialize this object. " + "Please use `Camcorder.from_dict` to recreate the object." + )
| return | ||
|
|
||
| # Get params | ||
| # Get `FrameGroup` for the current frame index |
There was a problem hiding this comment.
Ensure video and session parameters are properly handled when not provided.
- video = params.get("video", None) or context.state["video"]
- session = params.get("session", None) or context.labels.get_session(video)
+ video = params.get("video") if "video" in params else context.state["video"]
+ session = params.get("session") if "session" in params else context.labels.get_session(video)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.
| # Get `FrameGroup` for the current frame index | |
| # Get `FrameGroup` for the current frame index | |
| video = params.get("video") if "video" in params else context.state["video"] | |
| session = params.get("session") if "session" in params else context.labels.get_session(video) |
|
|
||
| # Skip if new point is nan or old point is complete | ||
| if np.isnan(point_new).any() or (exclude_complete and points_old.complete): | ||
| continue | ||
| points_dict[node_name] = Point(x=point_new[0], y=point_new[1]) | ||
|
|
||
| # Grab the x, y from the new point and visible, complete from the old point | ||
| x, y = point_new | ||
| visible = points_old.visible | ||
| complete = points_old.complete | ||
|
|
||
| # Create a new point and add to the dict | ||
| if type(self._points) == PredictedPointArray: | ||
| # TODO(LM): The point score is meant to rate the confidence of the | ||
| # prediction, but this method updates from triangulation. | ||
| score = points_old.score | ||
| point_obj = PredictedPoint( | ||
| x=x, y=y, visible=visible, complete=complete, score=score | ||
| ) | ||
| else: | ||
| point_obj = Point(x=x, y=y, visible=visible, complete=complete) | ||
|
|
||
| # Update the points dict | ||
| points_dict[node_name] = point_obj | ||
|
|
||
| # Update the points |
There was a problem hiding this comment.
Consider refining the handling of PredictedPoint objects in the update_points method. The current implementation may not correctly handle cases where the score attribute needs to be updated based on new data.
- score = points_old.score
+ score = calculate_new_score(points_old, point_new) # This is a placeholder for the actual score calculation logic.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.
| # Skip if new point is nan or old point is complete | |
| if np.isnan(point_new).any() or (exclude_complete and points_old.complete): | |
| continue | |
| points_dict[node_name] = Point(x=point_new[0], y=point_new[1]) | |
| # Grab the x, y from the new point and visible, complete from the old point | |
| x, y = point_new | |
| visible = points_old.visible | |
| complete = points_old.complete | |
| # Create a new point and add to the dict | |
| if type(self._points) == PredictedPointArray: | |
| # TODO(LM): The point score is meant to rate the confidence of the | |
| # prediction, but this method updates from triangulation. | |
| score = points_old.score | |
| point_obj = PredictedPoint( | |
| x=x, y=y, visible=visible, complete=complete, score=score | |
| ) | |
| else: | |
| point_obj = Point(x=x, y=y, visible=visible, complete=complete) | |
| # Update the points dict | |
| points_dict[node_name] = point_obj | |
| # Update the points | |
| # Skip if new point is nan or old point is complete | |
| if np.isnan(point_new).any() or (exclude_complete and points_old.complete): | |
| continue | |
| # Grab the x, y from the new point and visible, complete from the old point | |
| x, y = point_new | |
| visible = points_old.visible | |
| complete = points_old.complete | |
| # Create a new point and add to the dict | |
| if type(self._points) == PredictedPointArray: | |
| # TODO(LM): The point score is meant to rate the confidence of the | |
| # prediction, but this method updates from triangulation. | |
| score = calculate_new_score(points_old, point_new) # This is a placeholder for the actual score calculation logic. | |
| point_obj = PredictedPoint( | |
| x=x, y=y, visible=visible, complete=complete, score=score | |
| ) | |
| else: | |
| point_obj = Point(x=x, y=y, visible=visible, complete=complete) | |
| # Update the points dict | |
| points_dict[node_name] = point_obj | |
| # Update the points |
| # Update instances | ||
| TriangulateSession.update_instances( | ||
| instances_and_coords=instances_and_reprojected_coords | ||
| frame_idx: int = params["frame_idx"] |
There was a problem hiding this comment.
Validate frame_idx parameter to ensure it is provided.
- frame_idx: int = params["frame_idx"]
+ frame_idx: int = params.get("frame_idx")
+ if frame_idx is None:
+ raise ValueError("frame_idx parameter is required for triangulation.")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.
| frame_idx: int = params["frame_idx"] | |
| frame_idx: int = params.get("frame_idx") | |
| if frame_idx is None: | |
| raise ValueError("frame_idx parameter is required for triangulation.") |
| if instance_groups is None or len(instance_groups) == 0: | ||
| return # Not enough instances for triangulation |
There was a problem hiding this comment.
Add error handling for the case where no instance groups are available for triangulation.
- return # Not enough instances for triangulation
+ raise ValueError("No instance groups available for triangulation.")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.
| if instance_groups is None or len(instance_groups) == 0: | |
| return # Not enough instances for triangulation | |
| if instance_groups is None or len(instance_groups) == 0: | |
| raise ValueError("No instance groups available for triangulation.") |
| _instance_by_camcorder: Dict[Camcorder, "Instance"] = field(factory=dict) | ||
| _camcorder_by_instance: Dict["Instance", Camcorder] = field(factory=dict) | ||
| _dummy_instance: Optional["Instance"] = field(default=None) |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
| # Create a dummy instance of all nan values | ||
| example_instance: "Instance" = next(iter(all_instances_by_camera[0])) | ||
| skeleton: "Skeleton" = example_instance.skeleton | ||
| dummy_instance: "Instance" = example_instance.from_numpy( |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
| ) | ||
|
|
||
| def _fill_in_missing_instances( | ||
| unlocked_instances_in_view: List["Instance"], |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
|
|
||
| # For each view, get permutations of unlocked instances | ||
| unlocked_instance_permutations: Dict[ | ||
| Camcorder, Iterator[Tuple["Instance"]] |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
| for cam, instances_in_view in instances_by_camera.items(): | ||
| # Gather all instances for this cam from locked `InstanceGroup`s | ||
| locked_instances_in_view: Set[ | ||
| "Instance" |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
| ] = self._locked_instances_by_cam.get(cam, set()) | ||
|
|
||
| # Remove locked instances from instances in view | ||
| unlocked_instances_in_view: List["Instance"] = list( |
There was a problem hiding this comment.
The name Instance is used but not defined in this scope.
Please ensure that the Instance class is correctly imported or defined within this file or module to avoid runtime errors.
roomrys
merged commit c3a8173
into
liezl/add-method-to-match-instances-across-views
* Update methods to allow triangulating multiple instances at once * Return instances and coords as a dictionary with cams * Update get_instance_across_views to handle multiple frames * [wip] Update calculate reprojected points to support multiple frames * Finish support for multi-frame reprojection * Remove code to put in next PR * (3b -> 3a) Add method to get single instance permutations (#1586) * Add method to get single instance permutations * Append a dummy instance for missing instances * Correct 'permutations' to 'products' * (3c -> 3b) Add method to test instance grouping (#1599) * (3d -> 3c) Add method for multi instance products (#1605) * (3e -> 3a) Add `InstanceGroup` class (#1618) * Add method to get single instance permutations * Add method and (failing) test to get instance grouping * Append a dummy instance for missing instances * Update tests to accept a dummy instance * Add initial InstanceGroup class * Few extra tests for `InstanceGroup` * Remember instance grouping after testing hypotheses * Use reconsumable iterator for reprojected coords * Only triangulate user instances, add fixture, update tests * Normalize instance reprojection errors * Add `locked`, `_dummy_instance`, `numpy`, and `update_points` * Allow `PredictedPoint`s to be updated as well * Add tests for new attributes and methods * Add methods to create, add, replace, and remove instances * Use PredictedInstance for new/dummy instances * (3f -> 3e) Add `FrameGroup` class (#1665) * (3g -> 3f) Use frame group for triangulation (#1693) * Only use user-`Instance`s for triangulation * Remove unused code * Use `LabeledFrame` class instead of dummy labeled frame * Limit which methods can update `Labels.labeled_frames` * Update cache when Labels. remove_session_video * Remove RecordingSession.instance_groups * [wip] Maintain cached FrameGroup dictionaries * Add unique name (per FrameGroup) requirement for InstanceGroup * Lint * Fix remove_video bug * Add RecordingSession.new_frame_group method * Add TODO comments for later * Fix RecordingSesssion.remove_video bug * Remove FrameGroup._frame_idx_registry class attribute
Description
This PR adds and implements the
InstanceGroupdatastructure and should be used to updateInstanceGroup.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
Summary by CodeRabbit
Refactor
Tests
InstanceGroupandFrameGroupdata structures.