(3a -> 3) Add method to match instances across views #1579
Conversation
WalkthroughThe recent changes across the codebase focused on enhancing the Changes
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (9)
Files not summarized due to errors (1)
Additional Context UsedRuff (64)
Learnings (1)
Additional comments not posted (12)
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/asc-initial-update-instances-across-views
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/asc-initial-update-instances-across-views #1579 +/- ##
===================================================================================
- Coverage 73.71% 73.29% -0.43%
===================================================================================
Files 135 135
Lines 24502 24810 +308
===================================================================================
+ Hits 18062 18184 +122
- Misses 6440 6626 +186 ☔ View full report in Codecov by Sentry. |
roomrys
changed the title
|
@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/gui/commands.py (12 hunks)
- tests/gui/test_commands.py (8 hunks)
Additional comments: 13
tests/gui/test_commands.py (7)
985-990: No issues found. The function signature and the initial setup look fine.
1002-1006: No issues found. The assertions are checking the expected properties of the instances.
1029-1033: No issues found. The assertions are checking the expected properties of the instances.
1070-1108: No issues found. The function calls and assertions are checking the expected properties of the instances.
1188-1202: No issues found. The function calls and assertions are checking the expected properties of the instances.
1214-1227: No issues found. The function calls and assertions are checking the expected properties of the instances.
1240-1258: No issues found. The function calls and assertions are checking the expected properties of the instances.
sleap/gui/commands.py (6)
39-39: The
castfunction is imported but not used in the provided hunks. Ensure it is used elsewhere in the code or remove the import to maintain clean code.3429-3429: The
askmethod now has a return type annotation ofbool. Ensure that all calls to this method throughout the codebase have been updated to handle the return type.3449-3449: The
verify_views_and_instancesmethod now has a return type annotation ofbool. Ensure that all calls to this method throughout the codebase have been updated to handle the return type.3498-3498: The
get_and_verify_enough_instancesmethod has been renamed toget_instances_across_views_multiple_framesand now accepts a list of frame indices instead of a single frame index. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.3516-3554: The
get_and_verify_enough_instancesmethod now returns a dictionary with frame identifier keys and values of another inner dictionary withCamcorderkeys andList[Instance]values. Ensure that all calls to this method throughout the codebase have been updated to handle the return type.3643-3712: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [3606-3709]
The
get_instances_across_viewsmethod has been renamed toget_instances_across_views_multiple_framesand now accepts a list of frame indices instead of a single frame index. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
…ps://github.com/talmolab/sleap into liezl/add-method-to-match-instances-across-views
* 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)
roomrys
merged commit 2f56a1c
into
liezl/asc-initial-update-instances-across-views
There was a problem hiding this comment.
Actionable comments posted: 9
Actionable comments outside the diff hunks (25)
tests/fixtures/datasets.py (1)
Line range hint
1-1: Remove unused import to clean up the code.- import ostests/gui/test_commands.py (8)
Line range hint
5-5: Remove unused importDictfromtyping.- 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: Replace equality comparison toTruewith a direct truth check.- assert okay == True + assert okay
Line range hint
368-368: Use direct truth check instead of equality comparison toTrue.- assert video.backend.grayscale == True + assert video.backend.grayscale
Line range hint
360-360: The local variablelast_lf_frameis assigned but never used. Consider removing it if it's not needed.- last_lf_frame = get_last_lf_in_video(labels, videos[0])
Line range hint
528-528: Remove unnecessary f-string as it contains no placeholders.- default_name += f".{adaptor.default_ext}" + default_name += "." + adaptor.default_extsleap/instance.py (3)
Line range hint
411-411: Useisinstance()for type checks.- if from_predicted is not None and type(from_predicted) != PredictedInstance: + if from_predicted is not None and not isinstance(from_predicted, PredictedInstance):Direct type comparisons using
!=can lead to issues, especially with inheritance. Useisinstance()for a more robust type check.
Line range hint
449-449: Useisinstance()for type checks.- if type(points) is dict: + if isinstance(points, dict):Replace type comparison with
isinstance()for consistency and to handle potential subclasses ofdict.
Line range hint
1652-1652: Undefined nameLabelsin the methodcomplex_merge_between.It appears that the name
Labelsis used but not defined within this file or imported. Ensure thatLabelsis correctly imported from its module, or check if the name is incorrect or misspelled.sleap/io/cameras.py (3)
Line range hint
79-79: Remove unnecessary f-string as it does not contain any placeholders.- f"{self.name}, " + self.name + ", "
Line range hint
80-80: Remove unnecessary f-string as it does not contain any placeholders.- f"{message[:-2]})" + message[:-2] + ")"
Line range hint
81-81: Remove unnecessary f-string as it does not contain any placeholders.- f"{self.__class__.__name__}(name={self.name}, size={self.size})" + self.__class__.__name__ + "(name=" + self.name + ", size=" + self.size + ")"sleap/io/dataset.py (8)
Line range hint
61-61: Unused import ofh5py.Please remove the unused import of
h5pyto maintain clean and efficient code.
Line range hint
69-69: Avoid using bareexcept.Replace the bare
exceptwith a specific exception type to avoid catching unexpected exceptions, which can make debugging more difficult.
Line range hint
934-934: Useisinstance()for type checking.Replace the type comparison using
type()withisinstance()for a more robust and idiomatic check.- if type(inst) == PredictedInstance: + if isinstance(inst, PredictedInstance):
Line range hint
2355-2355: Undefined nameglob.The function
globis used but not imported. Please add the necessary import statement:from glob import glob
Line range hint
2450-2450: Avoid equality comparisons toFalse.Use
if not ret:instead ofif ret == False:for a more Pythonic approach.- if ret == False: + if not ret:
Line range hint
2553-2553: Undefined namesleap.The module
sleapis referenced but not imported. Ensure that the necessary import statement is added or correct the reference if it's a mistake.
Line range hint
2646-2646: Unused variablee.The variable
eis assigned in theexceptblock but never used. Consider removing it if not needed.- except IndexError as e: + except IndexError:
Line range hint
2648-2648: F-string without placeholders.The f-string used does not contain any placeholders. Use a regular string instead if no formatting is needed.
- raise IndexError(f"There are no videos in this project. No points matrix to return.") + raise IndexError("There are no videos in this project. No points matrix to return.")sleap/gui/commands.py (2)
Line range hint
196-196: Undefined nameMainWindowmay cause a runtime error if not handled properly.Please ensure that
MainWindowis defined or imported correctly in this context.
Line range hint
838-838: The local variablefile_diris assigned but never used, which is unnecessary.Consider removing the assignment or using the variable appropriately.
| import logging | ||
| import tempfile | ||
| from pathlib import Path | ||
| from typing import Any, Dict, Iterator, List, Optional, Tuple, Union | ||
| from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set |
There was a problem hiding this comment.
Remove unused import cast from typing.
- from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set
+ from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, SetCommittable 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.
| from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set | |
| from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, Set |
| instance = self.instances[0] | ||
|
|
||
| # Use the example instance to create a dummy instance | ||
| skeleton: "Skeleton" = instance.skeleton |
There was a problem hiding this comment.
Undefined name Skeleton used. Ensure that Skeleton is defined or imported in this file.
| """ | ||
|
|
||
| # Get the `Skeleton` | ||
| skeleton: "Skeleton" = self.dummy_instance.skeleton |
There was a problem hiding this comment.
Undefined name Skeleton used. Ensure that Skeleton is defined or imported in this file.
| # isinstance(idx_or_key, Instance): | ||
| try: | ||
| return self.get_cam(idx_or_key) | ||
| except: |
There was a problem hiding this comment.
Avoid using a bare except statement. Specify the exception type to handle specific exceptions appropriately.
- except:
+ except Exception as e: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.
| except: | |
| except Exception as e: |
| @@ -35,8 +35,9 @@ class which inherits from `AppCommand` (or a more specialized class such as | |||
| import traceback | |||
| from enum import Enum | |||
| from glob import glob | |||
| from itertools import permutations, product | |||
There was a problem hiding this comment.
Remove unused imports to clean up the code.
- from itertools import permutations, productCommittable 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.
| from itertools import permutations, product |
| @@ -412,8 +914,9 @@ | |||
| # TODO(LM): Consider implementing Observer pattern for `camera_cluster` and `labels` | |||
| camera_cluster: CameraCluster = field(factory=CameraCluster) | |||
| metadata: dict = field(factory=dict) | |||
| labels: Optional["Labels"] = field(default=None) | |||
There was a problem hiding this comment.
Undefined name Labels used. Ensure that Labels is defined or imported in this file.
| from pathlib import Path, PurePath | ||
| from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union | ||
| from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union, cast |
There was a problem hiding this comment.
Remove unused import to clean up the code.
- from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union, cast
+ from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, UnionCommittable 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.
| from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union, cast | |
| from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union |
| 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) | ||
|
|
There was a problem hiding this comment.
Ensure consistent handling of point types in update_points.
The method differentiates between PredictedPointArray and PointArray when creating new point objects. This is a good practice as it maintains the distinction between predicted and manually labeled points. However, ensure that this differentiation is consistently applied throughout your codebase to avoid type mismatches.
| @@ -500,7 +500,6 @@ | |||
| ) | |||
| try: | |||
| parray[skeleton.node_to_index(node)] = point | |||
| # parray[skeleton.node_to_index(node.name)] = point | |||
| except: | |||
There was a problem hiding this comment.
Avoid using a bare except statement.
- except:
+ except Exception as e:
+ logger.error(f"Error setting point for node {node} in {skeleton} with point {point}: {str(e)}")Using a bare except can catch unexpected exceptions and make debugging difficult. Specify the exception type or handle the general Exception and log the error for better error management.
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.
| except: | |
| except Exception as e: | |
| logger.error(f"Error setting point for node {node} in {skeleton} with point {point}: {str(e)}") |
Description
This PR aims to add a method to match instances across views (without knowing
Tracks).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
delete.pyfile for improved frame removal handling.instance.pyfile to improve point object creation.dataset.pyfile for better video and session management.datasets.pyfile.test_commands.pyfile to reflect changes in session handling.test_cameras.pyfile to align with entity imports.test_dataset.pyfile for explicit parameter passing.