-
Notifications
You must be signed in to change notification settings - Fork 96
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
(3->2) Add method to update Instance
s across views in RecordingSession
#1279
base: liezl/ars-add-sessions-to-cache
Are you sure you want to change the base?
(3->2) Add method to update Instance
s across views in RecordingSession
#1279
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## liezl/ars-add-sessions-to-cache #1279 +/- ##
===================================================================
+ Coverage 73.60% 73.99% +0.38%
===================================================================
Files 135 135
Lines 24366 24923 +557
===================================================================
+ Hits 17935 18441 +506
- Misses 6431 6482 +51 ☔ View full report in Codecov by Sentry. |
…-update-instances-across-views
Instance
s across views in RecordingSession
Instance
s across views in RecordingSession
…-update-instances-across-views
…talmolab/sleap into liezl/asc-initial-update-instances-across-views
WalkthroughThe recent updates focus on enhancing the SLEAP software's triangulation capabilities and data handling, particularly with multiview analysis. New methods and classes have been introduced to manage and process camera data, instances, and sessions more effectively. The changes also involve improvements in deletion operations, error handling, and test coverage, ensuring more robust functionality and user experience. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files not summarized due to errors (1)
Additional Context UsedRuff (7)
Additional comments not posted (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Files selected for processing (3)
- sleap/gui/app.py (1 hunks)
- sleap/instance.py (2 hunks)
- sleap/io/cameras.py (2 hunks)
Additional comments (Suppressed): 5
sleap/gui/app.py (3)
1228-1230: The
video
andinstance
are retrieved from thestate
dictionary. Ensure that these keys always exist in thestate
dictionary before this method is called, or add error handling for cases where they might not be present.1236-1242: The
update_views
method is called on thesession
object withframe_idx
andtrack
as arguments. Make sure that all calls to this function throughout the codebase have been updated to match the new signature.1244-1247: The
updatePoints
method is called withcomplete=False
anduser_change=False
. Ensure that these parameters are correctly handled in theupdatePoints
method and that their implications are well understood.sleap/instance.py (1)
- 1397-1405: The sorting of instances in the
find
function is a good idea to ensure thatPredictedInstances
are processed last. However, it's important to note that this changes the order of instances returned by the function. If any code relies on the original order of instances, it could potentially break. Please verify that this change doesn't negatively impact any existing functionality.sleap/io/cameras.py (1)
- 15-22: The import of
triangulate
andreproject
functions from thesleap_anipose
module is new. Ensure that this module is included in the project dependencies and is properly installed in the environment where this code will run.
sleap/gui/app.py
Outdated
# TODO(LM): Move this to a dock for replotting on demand | ||
# Replot instance nodes (but keep same QtInstance) | ||
for inst in self.player.view.instances: | ||
inst.updatePoints(complete=False, user_change=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instances are updated using the updatePoints
method. This could potentially be a performance bottleneck if there are many instances. Consider optimizing this part of the code if performance becomes an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you propose we optimize this part of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roomrys, one way to optimize this part of the code could be to only update the instances that have actually changed. Currently, it seems like all instances are updated regardless of whether they've changed or not.
Here's a rough idea of how you might implement this:
# Keep track of the last frame index and track we updated
last_frame_idx = None
last_track = None
def _after_plot_update(self, frame_idx):
# ...
if session is not None and instance is not None:
track = instance.track
# Only update views if the frame index or track has changed
if frame_idx != self.last_frame_idx or track != self.last_track:
session.update_views(frame_idx, track)
self.last_frame_idx = frame_idx
self.last_track = track
# Replot instance nodes (but keep same QtInstance)
for inst in self.player.view.instances:
inst.updatePoints(complete=False, user_change=False)
This way, we avoid unnecessary updates when the frame index and track haven't changed. However, please note that this is just a suggestion and may need to be adapted based on your specific use case and codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- sleap/io/cameras.py (2 hunks)
Additional comments (Suppressed): 1
sleap/io/cameras.py (1)
- 1-22: The import order has been changed and new imports have been added. Ensure that these changes do not affect the functionality of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- sleap/io/cameras.py (2 hunks)
Additional comments (Suppressed): 6
sleap/io/cameras.py (6)
550-551: The
remove_video
method has been updated to remove the video from the labels if it exists. This is a good practice as it ensures that the labels are kept in sync with the videos in the session.553-576: The new method
get_videos_from_selected_cameras
retrieves allVideo
s from selectedCamcorder
s. If noCamcorder
s are specified, it returns all videos in the session. This method provides flexibility in selecting videos based on specific cameras.578-626: The new method
get_instances_accross_views
retrieves allInstances
across all views at a given frame index. It also allows for filtering byCamcorder
s andTrack
. Ifrequire_multiple_views
is set to True, it raises a warning if only one view is found. This method provides a way to gather instances across multiple views, which can be useful for operations like triangulation.628-652: The new method
calculate_reprojected_points
triangulates and reprojects instance coordinates. It first gathers instances into an array, then uses thetriangulate
andreproject
functions fromsleap_anipose
to calculate 3D points and reproject them back to 2D. The result is a list of reprojected instance coordinates. This method encapsulates the process of triangulating and reprojecting points, making it easier to perform these operations on a list of instances.654-708: The new method
update_views
updates the views of theRecordingSession
by triangulating 3D points from multiple views and updating the instances with these new points. It first checks if there are enoughCamcorder
s available. If not, it logs a warning and returns. Then it gets all instances across views at the specified frame index. If there are not enough instances, it logs a warning and returns. Finally, it calculates reprojected points and updates the points in each instance. This method provides a way to update the views of a recording session based on triangulated 3D points.709-711: The
__attrs_post_init__
method has been moved down in the code. It still adds the session to the camera cluster after initialization, ensuring that the camera cluster is kept up-to-date with its sessions.
…Fixture (#1281) * Fix (de)serialization of `RecordingSession` * Remove tracks from multiview dataset * Replace `RecordingSession` test data * Add `RecordingSession` to test set and adapt fixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Files ignored due to filter (9)
- tests/data/cameras/minimal_session/calibration.toml
- tests/data/videos/min_session_back.mp4
- tests/data/videos/min_session_backL.mp4
- tests/data/videos/min_session_mid.mp4
- tests/data/videos/min_session_midL.mp4
- tests/data/videos/min_session_side.mp4
- tests/data/videos/min_session_sideL.mp4
- tests/data/videos/min_session_top.mp4
- tests/data/videos/min_session_topL.mp4
Files selected for processing (6)
- sleap/io/cameras.py (5 hunks)
- sleap/io/dataset.py (1 hunks)
- sleap/io/format/hdf5.py (3 hunks)
- tests/fixtures/datasets.py (1 hunks)
- tests/io/test_cameras.py (3 hunks)
- tests/io/test_dataset.py (2 hunks)
Files skipped from review due to trivial changes (2)
- sleap/io/dataset.py
- tests/fixtures/datasets.py
Additional comments (Suppressed): 14
sleap/io/format/hdf5.py (3)
86-91: The key "sessions" has been added to the list of keys that are checked in the HDF5 file. Ensure that this new key is being properly handled elsewhere in the codebase and that its addition does not break any existing functionality.
325-330: Similar to the previous comment, the key "sessions" has been added to the list of keys for saving in the HDF5 dataset. Again, verify that this new key is being properly handled and does not introduce any issues.
345-346: There's a typo correction from "PredcitedInstances" to "PredictedInstances". This change should be verified across the codebase to ensure consistency.
tests/io/test_cameras.py (3)
4-10: The import statement for
Labels
fromsleap.io.dataset
has been added. Ensure that this new import is used in the subsequent code changes.63-66: The expected length of
camera_cluster
has changed from 4 to 8. Verify if this change aligns with the modifications made in the camera cluster setup or data.204-209: The keys in the
camcorder_to_video_idx_map
dictionary have changed from integers to strings. This could potentially affect how the map is accessed elsewhere in the code. Please verify that all usages of this map are updated to use string keys.tests/io/test_dataset.py (1)
- 972-981: The function signature for
test_save_labels_with_sessions
has been updated to include a new parametermultiview_min_session_labels
. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.sleap/io/cameras.py (7)
550-552: The
remove_video
method has been updated to remove the video from the labels if it exists. This is a good practice as it ensures that the labels are always in sync with the videos in the session.553-576: The new method
get_videos_from_selected_cameras
retrieves all videos associated with a given list of camcorders. If no camcorders are specified, it returns all videos in the session. This method provides flexibility in selecting videos based on their associated camcorders.578-626: The new method
get_instances_accross_views
retrieves all instances across all views at a given frame index. It allows for optional filtering by camcorders and tracks. Ifrequire_multiple_views
is set to True, it raises a warning if only one view is found. This method is useful for gathering instances across multiple views for further processing, such as triangulation and reprojection.628-652: The new method
calculate_reprojected_points
triangulates and reprojects instance coordinates. It takes a list of instances, gathers their coordinates into an array, performs triangulation and reprojection, and then splits the reprojected coordinates back into a list. This method is crucial for updating the views with new 3D points.654-708: The new method
update_views
updates the views of theRecordingSession
. It first checks if there are enough camcorders available or specified. If not, it logs a warning and returns. Then it gets all instances across all views at the specified frame index. If there are not enough instances, it logs a warning and returns. Finally, it calculates the reprojected points and updates the coordinates of each instance. This method encapsulates the process of updating views, making it easier to perform this operation.828-834: The keys and values in
camcorder_to_video_idx_map
are now converted to strings before being used. This change may affect other parts of the code that use this map. Ensure that all uses of this map have been updated to handle string keys and values.- camcorder_to_video_idx_map[cam_idx] = video_idx + camcorder_to_video_idx_map[str(cam_idx)] = str(video_idx)
- 864-871: The keys and values in
camcorder_to_video_idx_map
are now converted back to integers before being used. This change is consistent with the previous change where these keys and values were converted to strings. However, ensure that all uses of this map have been updated to handle integer keys and values.- camcorder = session.camera_cluster.cameras[cam_idx] - video = videos_list[video_idx] + camcorder = session.camera_cluster.cameras[int(cam_idx)] + video = videos_list[int(video_idx)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- sleap/io/cameras.py (5 hunks)
- tests/io/test_cameras.py (6 hunks)
Additional comments (Suppressed): 13
tests/io/test_cameras.py (6)
12-12: The import statement for
Instance
,LabeledFrame
, andLabels
fromsleap.io.dataset
is not present in the old hunk. Verify if these classes are used in the new hunks and if they are necessary.68-68: The number of cameras in the camera cluster has changed from 4 to 8. Ensure that this change is intentional and correctly reflected in the test data.
133-133: A new parameter
multiview_min_session_labels: Labels
has been added to the function signature. Make sure all calls to this function have been updated accordingly.211-213: The keys in the
camcorder_to_video_idx_map
dictionary have changed from integers to strings. This could potentially cause issues if the rest of the code expects integer keys. Please verify this change.227-236: New tests have been added for the
get_videos_from_selected_cameras
method. These tests seem to cover different scenarios and look well-written.262-303: New tests have been added for the
get_instances_across_views
method. These tests cover various scenarios including excluding certain camera views, requiring multiple views, and handling cases where there are not enough instances. The tests appear to be comprehensive and well-written.sleap/io/cameras.py (7)
1-22: Imports have been rearranged and
from sleap_anipose import triangulate, reproject
has been added. Ensure that the new import does not introduce any circular dependencies or conflicts with existing code.550-552: The
remove_video()
method now removes the video from the labels if it exists. This is a change in behavior that could potentially impact other parts of the system that rely on this method. Verify that this change does not break any existing functionality.553-576: New method
get_videos_from_selected_cameras()
retrieves videos from selectedCamcorder
s. The logic seems sound and the method is well-documented.649-673: New method
calculate_reprojected_points()
triangulates and reprojects instance coordinates. The method is well-documented and the logic seems sound.781-787: The string representation of the
RecordingSession
class now includes the number of videos. This is a minor change that should not impact functionality.849-855: The keys and values in
camcorder_to_video_idx_map
are now stored as strings instead of integers. This could potentially impact other parts of the system that use this map. Verify that this change does not break any existing functionality.885-892: The keys and values in
camcorder_to_video_idx_map
are now converted back to integers when retrievingCamcorder
andVideo
objects. This is consistent with the change made in lines 849-855, but again, verify that this change does not break any existing functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- sleap/io/cameras.py (7 hunks)
- tests/io/test_cameras.py (6 hunks)
Additional comments (Suppressed): 15
tests/io/test_cameras.py (5)
66-68: The number of cameras in the
camera_cluster
has been updated from 4 to 8. Ensure that this change is reflected throughout the codebase and does not break any existing functionality.129-133: A new parameter
multiview_min_session_labels: Labels
has been added to the test function forRecordingSession
. Make sure that all calls to this function have been updated accordingly.210-213: The keys in the
camcorder_to_video_idx_map
dictionary have been changed from integers to strings. This could potentially cause type errors if the rest of the codebase expects these keys to be integers. Please verify.227-236: New tests have been added for the
get_videos_from_selected_cameras
method. These tests check whether the method correctly returns videos from selected cameras, and whether it returns all videos when no cameras are selected.262-312: New tests have been added for the
get_instances_across_views
method. These tests check whether the method correctly retrieves instances across all views at a given frame index, whether it correctly handles cases where certain camera views are excluded, and whether it raises an error when there are not enough instances or views.sleap/io/cameras.py (10)
15-22: The import statements have been rearranged. Ensure that this does not affect the functionality of the code.
550-552: The
remove_video
method has been updated to remove the video from the labels if it exists. This is a good practice as it ensures data consistency.553-576: The new method
get_videos_from_selected_cameras
retrieves videos from selected cameras. It's a good addition for modularity and reusability.647-672: The new method
calculate_reprojected_points
triangulates and reprojects instance coordinates. The use of numpy operations should ensure good performance.673-728: The new method
update_views
updates the views of theRecordingSession
. It uses the previously defined methodsget_instances_across_views
andcalculate_reprojected_points
, demonstrating good use of modularity.735-740: No significant changes were made in these lines.
778-784: The
__repr__
method has been updated to include the number of videos in its output. This is a useful addition for debugging and logging purposes.836-841: No significant changes were made in these lines.
845-851: The keys and values of
camcorder_to_video_idx_map
are now being stored as strings instead of integers. Ensure that this change is reflected wherever this map is used.881-888: The keys and values of
camcorder_to_video_idx_map
are now being retrieved as integers instead of strings. This is consistent with the change made in lines 845-851.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- sleap/io/cameras.py (7 hunks)
Additional comments (Suppressed): 7
sleap/io/cameras.py (7)
1-22: The import order has been changed. Ensure that this does not affect the functionality of the code.
550-755: New methods
get_videos_from_selected_cameras()
,get_all_views_at_frame()
,get_instances_across_views()
,calculate_reprojected_points()
, andupdate_views()
have been added to theRecordingSession
class. These methods enable triangulation and reprojection of instance coordinates, retrieval of instances across all views at a given frame index, and updating of views in the recording session. Theremove_video()
method has also been updated to remove the video from labels if it exists.Ensure that these new methods are properly tested and that they work as expected with the rest of the system. Also, consider breaking down complex methods like
get_instances_across_views()
andupdate_views()
into smaller helper methods to improve readability and maintainability.
760-765: No significant changes detected.
803-809: The
__repr__
method now includes the number of videos in the session. This is a useful addition for debugging and logging purposes.861-866: No significant changes detected.
870-876: The keys and values in
camcorder_to_video_idx_map
are now stored as strings instead of integers. Ensure that this change is reflected wherever this map is used.906-913: The keys and values in
camcorder_to_video_idx_map
are now retrieved as strings and converted back to integers. Ensure that this change is reflected wherever this map is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- sleap/gui/app.py (1 hunks)
- sleap/io/cameras.py (7 hunks)
- tests/io/test_cameras.py (6 hunks)
Additional comments (Suppressed): 20
tests/io/test_cameras.py (4)
12-12: The import statement for
Instance
,LabeledFrame
,Labels
, andLabelsDataCache
fromsleap.io.dataset
is not present in the old hunk. Ensure that these classes are used in the new hunks and that they are imported correctly.68-68: The number of cameras in the
camera_cluster
has been changed from 4 to 8. Ensure that this change is intentional and that it does not affect other parts of the code that rely on the number of cameras.210-212: The keys in the
camcorder_to_video_idx_map
dictionary have been changed from integers to strings. Ensure that this change is intentional and that it does not affect other parts of the code that use this dictionary.251-448: New test functions have been added to test the new methods in the
RecordingSession
class. Ensure that these tests cover all possible edge cases and that they pass successfully.sleap/gui/app.py (3)
1228-1233: The
video
andinstance
are retrieved from thestate
dictionary. Ensure that these keys are always present in thestate
dictionary to avoid KeyError. If there's a chance they might not be present, consider usingself.state.get("video")
andself.state.get("instance")
instead.1236-1244: The
update_views
method is called on thesession
object. Ensure that this method is implemented correctly and efficiently, as it could potentially be a performance bottleneck if it involves heavy computations or I/O operations. Also, verify that thesession
object is always non-null before calling methods on it.1248-1249: The
updatePoints
method is called on each instance inself.player.view.instances
. As mentioned in the previous comments, this could potentially be a performance bottleneck if there are many instances. Consider optimizing this part of the code if performance becomes an issue. The previous suggestion to only update instances that have actually changed could be a viable solution.sleap/io/cameras.py (13)
547-549: No changes were made in these lines.
550-574: The new method
get_videos_from_selected_cameras
retrieves allVideo
objects from selectedCamcorder
objects. If noCamcorder
objects are specified, it returns all videos in the session. This method improves the modularity of the code by providing a reusable function for retrieving videos.575-612: The new method
get_all_views_at_frame
retrieves all views at a given frame index. It uses theget_videos_from_selected_cameras
method to get the videos and then retrieves theLabeledFrame
objects for each video. This method improves the modularity of the code by providing a reusable function for retrieving views.614-665: The new method
get_instances_across_views
retrieves allInstance
objects across all views at a given frame index. It uses theget_all_views_at_frame
method to get the views and then retrieves theInstance
objects for each view. This method improves the modularity of the code by providing a reusable function for retrieving instances.667-684: The new method
calculate_excluded_views
calculates the views that are excluded from a dictionary ofCamcorder
toInstance
. This method improves the modularity of the code by providing a reusable function for calculating excluded views.686-732: The new method
calculate_reprojected_points
triangulates and reprojects instance coordinates. It uses thecalculate_excluded_views
method to calculate the excluded views and then triangulates and reprojects the instance coordinates. This method improves the modularity of the code by providing a reusable function for triangulating and reprojecting instance coordinates.733-753: The new method
update_instances
triangulates, reprojects, and updates the coordinates ofInstance
objects. It uses thecalculate_reprojected_points
method to triangulate and reproject the instance coordinates and then updates the instance coordinates. This method improves the modularity of the code by providing a reusable function for updating instance coordinates.754-803: The new method
update_views
updates the views of theRecordingSession
. It uses theget_instances_across_views
andupdate_instances
methods to get the instances across views and update the instance coordinates, respectively. This method improves the modularity of the code by providing a reusable function for updating views.810-815: No changes were made in these lines.
853-859: The
__repr__
method has been updated to include the number of videos in theRecordingSession
in its output. This change improves the information provided by the__repr__
method.911-916: No changes were made in these lines.
920-926: The keys and values in
camcorder_to_video_idx_map
are now stored as strings instead of integers. This change may affect the way this map is used elsewhere in the code. Ensure that all uses of this map have been updated to handle string keys and values.956-963: The keys and values in
camcorder_to_video_idx_map
are now converted to integers before being used to index intosession.camera_cluster.cameras
andvideos_list
, respectively. This change is necessary due to the change in the type of the keys and values incamcorder_to_video_idx_map
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (5)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (3 hunks)
- sleap/io/cameras.py (7 hunks)
- tests/gui/test_commands.py (3 hunks)
- tests/io/test_cameras.py (6 hunks)
Additional comments (Suppressed): 18
sleap/gui/commands.py (3)
36-59: The import statements have been reorganized and some unused imports have been removed. The import of
sleap_anipose
for triangulation and reprojection functions is new. Ensure that this package is included in the project dependencies.614-637: A new method
triangulateSession
has been added. This method seems to be used for triangulatingInstance
s for selected views in aRecordingSession
. Ensure that this method is being called correctly in the codebase.3292-3726: A new class
TriangulateSession
has been added. This class seems to be used for triangulating and reprojecting instances in a session at a frame index. It includes several methods for verifying views and instances, getting instances across views, triangulating and reprojecting instance coordinates, and updating instance coordinates.The
open_website
function has been moved to the end of the file.Ensure that the
TriangulateSession
class is being used correctly in the codebase and that theopen_website
function is still accessible after being moved.tests/gui/test_commands.py (3)
- 5-5: The
Dict
import is missing in the old hunk. Ensure that the addition ofDict
does not break any existing functionality.- from typing import List + from typing import Dict, List
- 21-21: The
TriangulateSession
import is new. Ensure that the addition ofTriangulateSession
does not break any existing functionality.+ TriangulateSession,
- 960-1298: The new tests added for
TriangulateSession
are comprehensive and cover various scenarios. However, ensure that the tests are passing and that they cover all edge cases.tests/io/test_cameras.py (6)
12-12: The import of
Instance
,LabeledFrame
,Labels
, andLabelsDataCache
fromsleap.io.dataset
is new. Ensure that these are used appropriately in the tests.68-68: The number of cameras in the
camera_cluster
has been doubled from 4 to 8. Verify that this change is intentional and correctly reflected in the tests.131-137: No changes have been made in these lines. The code remains the same as in the old hunk.
191-193: The comparison function
compare_cameras
has been updated to compare the length ofsession_1.camera_cluster
withsession_2.camera_cluster
instead ofsession.camera_cluster
. This change makes the function more generic and reusable.210-212: The
camcorder_to_video_idx_map
now uses string indices instead of integer indices. Ensure that this change is reflected in all places where this map is used.251-282: New tests have been added for the methods
get_videos_from_selected_cameras
andremove_video
of theRecordingSession
class. These tests seem to cover the basic functionality of the methods. However, consider adding more edge cases and error scenarios to make the tests more robust.sleap/gui/app.py (2)
- 1224-1238: The
_after_plot_update
method has been updated to include a call toself.commands.triangulateSession
for replotting connected views in multi-camera projects. However, thecams_to_include
parameter is currently hardcoded toNone
. Consider making this configurable via the GUI or some other mechanism to allow users to specify which cameras to include in the triangulation.- cams_to_include = None # TODO: make this configurable via GUI + cams_to_include = self.get_selected_cameras() # hypothetical method to get selected cameras
- 1238-1238: The signature of the
_after_plot_change
method has been changed to include aselected_inst
parameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.sleap/io/cameras.py (4)
625-631: The
__repr__
method has been updated to include the number of videos in theRecordingSession
. This is a good change as it provides more information about theRecordingSession
object when it is printed.683-688: No significant changes have been made in these lines. The code is still iterating over the
camera_cluster
and skippingCamcorder
s not linked to anyVideo
.692-698: The keys and values in
camcorder_to_video_idx_map
are now being stored as strings instead of integers. This change should be verified to ensure it doesn't break any code that uses this map and expects integer keys/values.728-735: The keys and values in
camcorder_to_video_idx_map
are now being converted back to integers before use. This is consistent with the change made in lines 692-698. However, the same verification should be done to ensure this change doesn't break any code that uses this map and expects integer keys/values.
…talmolab/sleap into liezl/asc-initial-update-instances-across-views
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (4 hunks)
Additional comments (Suppressed): 23
sleap/gui/app.py (3)
1238-1239: The
overlay
variable is now explicitly typed asTrackTrailOverlay
. This is a good practice for improving code readability and maintainability.1241-1243: The
triangulateSession
command is now called after each plot update. This could potentially impact performance if the triangulation operation is computationally expensive. Consider verifying if this change doesn't introduce noticeable lag in the GUI, especially for large datasets or complex multi-camera setups.1245-1246: The function signature of
_after_plot_change
has been changed. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.sleap/gui/commands.py (20)
44-45: The import order has been changed. Ensure that this does not affect the functionality of the code.
56-56: The
Camcorder
class has been imported fromsleap.io.cameras
. Ensure that all references to this class in the code have been updated accordingly.616-632: A new method
triangulateSession
has been added. This method triangulatesInstance
s for selected views in aRecordingSession
. Ensure that this method is called correctly in the code.3390-3818: A new class
TriangulateSession
has been added. This class contains methods for triangulating and reprojecting instances in a session at a frame index. Ensure that this class is used correctly in the code.3422-3423: The
video
andsession
variables are retrieved from theparams
dictionary or from thecontext
object. Ensure that these variables are always available in theparams
dictionary or thecontext
object.3424-3425: The
instances
variable is retrieved directly from theparams
dictionary. Ensure that this variable is always available in theparams
dictionary.3427-3427: The
update_instances
method of theTriangulateSession
class is called. Ensure that this method is implemented correctly and that it updates the instances as expected.3447-3447: The
verify_views_and_instances
method of theTriangulateSession
class is called. Ensure that this method is implemented correctly and that it verifies the views and instances as expected.3469-3472: The
video
,session
, andinstance
variables are retrieved from theparams
dictionary or from thecontext
object. Ensure that these variables are always available in theparams
dictionary or thecontext
object.3474-3476: The
frame_idx
variable is retrieved from theparams
dictionary or from thecontext
object. Ensure that this variable is always available in theparams
dictionary or thecontext
object.3483-3483: The
cams_to_include
variable is retrieved from theparams
dictionary or from thesession
object. Ensure that this variable is always available in theparams
dictionary or thesession
object.3495-3502: The
get_and_verify_enough_instances
method of theTriangulateSession
class is called. Ensure that this method is implemented correctly and that it gets and verifies the instances as expected.3509-3509: The
instances
variable is added to theparams
dictionary. Ensure that this variable is used correctly in the rest of the code.3514-3561: The
get_and_verify_enough_instances
method of theTriangulateSession
class is implemented. This method gets all instances across views at a given frame index. If not enoughInstance
s are available at this frame index, then it returns False. Ensure that this method is implemented correctly and that it gets and verifies the instances as expected.3564-3595: The
verify_enough_views
method of theTriangulateSession
class is implemented. This method checks if there are enoughCamcorder
s available/specified. If not, it returns False. Ensure that this method is implemented correctly and that it verifies the views as expected.3599-3655: The
get_instances_across_views
method of theTriangulateSession
class is implemented. This method gets allInstances
across all views at a given frame index. Ensure that this method is implemented correctly and that it gets the instances as expected.3657-3696: The
get_all_views_at_frame
method of theTriangulateSession
class is implemented. This method gets all views at a given frame index. Ensure that this method is implemented correctly and that it gets the views as expected.3698-3710: The
get_instances_matrices
method of theTriangulateSession
class is implemented. This method gathers instances from views into an array. Ensure that this method is implemented correctly and that it gets the instances matrices as expected.3722-3741: The
calculate_excluded_views
method of theTriangulateSession
class is implemented. This method gets excluded views from a dictionary ofCamcorder
toInstance
. Ensure that this method is implemented correctly and that it calculates the excluded views as expected.3743-3792: The
calculate_reprojected_points
method of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/commands.py (4 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 5
sleap/gui/commands.py (1)
- 41-45: The import from
sleap_anipose
is new and should be checked for correct usage in the rest of the file.
Verification result:
The new imports from
sleap_anipose
are correctly used in thesleap/gui/commands.py
file. The functionstriangulate
andreproject
are utilized in the methodstriangulateSession
,calculate_reprojected_points
, and within theTriangulateSession
class. No issues with the usage of these imports were detected based on the provided context.tests/gui/test_commands.py (4)
5-10: The addition of the
numpy
import aligns with the PR's objective to enhance mathematical operations, likely for the new triangulation methods.18-27: The addition of
TriangulateSession
and related methods to the import list is consistent with the PR's objective to add methods for updatingInstance
s across views.955-957: The addition of the
test_AddSession
function is consistent with the PR's objective to add methods related toRecordingSession
. It is important to ensure that the new session-related functionality is covered by tests.960-1298: The addition of multiple test functions related to the
TriangulateSession
class is consistent with the PR's objective to add methods for updatingInstance
s across views. These tests are crucial for validating the new functionality.
* 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 17
Actionable comments outside the diff hunks (19)
tests/gui/test_commands.py (11)
Line range hint
227-227
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
236-236
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
244-244
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
253-253
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
265-265
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
274-274
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
281-281
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
299-299
: Useif okay:
instead ofif okay == True:
for truth checks.- assert okay == True + assert okay
Line range hint
360-360
: Remove unused local variablelast_lf_frame
.- last_lf_frame = get_last_lf_in_video(labels, videos[0])
Line range hint
368-368
: Useif video.backend.grayscale:
instead ofif video.backend.grayscale == True:
for truth checks.- assert video.backend.grayscale == True + assert video.backend.grayscale
Line range hint
528-528
: Remove unnecessary f-string as it contains no placeholders.- default_name += f".{adaptor.default_ext}" + default_name += f"{adaptor.default_ext}"sleap/instance.py (3)
Line range hint
89-89
: Undefined variableself
used in the method. This might be a typo or a missing parameter.- self.isnan() + # Correct the usage of `self` or define it appropriately.
Line range hint
411-411
: Replace type comparison withisinstance()
for better practice and to handle subclasses properly.- if type(from_predicted) != PredictedInstance: + if not isinstance(from_predicted, PredictedInstance):
Line range hint
1652-1652
: Undefined nameLabels
. This might be a typo or a missing import.- base_labels: The :class:`Labels` into which we are merging. + base_labels: The :class:`YourCorrectClassName` into which we are merging.sleap/io/cameras.py (1)
Line range hint
79-79
: The f-string does not contain any placeholders, which makes it a regular string. Consider removing the f-string notation.- f"{self.__class__.__name__}(name={self.name}, size={self.size})" + "{self.__class__.__name__}(name={self.name}, size={self.size})"sleap/gui/commands.py (4)
Line range hint
196-196
: Undefined nameMainWindow
may cause a runtime error if not handled properly.Please ensure that
MainWindow
is defined or imported correctly in this context.
Line range hint
838-838
: The local variablefile_dir
is assigned but never used, which could be an oversight or unnecessary code.Consider removing or using the
file_dir
variable as intended.
Line range hint
2470-2470
: This f-string does not contain any placeholders, which makes the use of an f-string unnecessary.- logger.warning(f"This video type {type(video.backend)} for video at index {idx} does not support grayscale yet.") + logger.warning("This video type does not support grayscale yet.")
Line range hint
2802-2802
: This f-string does not contain any placeholders, which makes the use of an f-string unnecessary.- logger.warning(f"Recieved the following error while replacing skeleton:\n{''.join(tb_str)}") + logger.warning("Recieved the following error while replacing skeleton:\n" + ''.join(tb_str))
sleap/io/cameras.py
Outdated
import tempfile | ||
from pathlib import Path | ||
from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import cast
.
- from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set
+ from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, Set
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.
from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, cast, Set | |
from typing import Any, Dict, Iterator, List, Optional, Tuple, Union, Set |
labels = multiview_min_session_labels | ||
session = labels.sessions[0] | ||
for vid, cam in zip(labels.videos, session.camera_cluster.cameras): | ||
session.add_video(vid, cam) | ||
|
||
new_path = str(Path(tmpdir, "test.slp")) | ||
labels.save(new_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an assertion after the save operation to verify the saved data.
labels.save(new_path)
loaded_labels = Labels.load_file(new_path)
assert loaded_labels == labels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Actionable comments outside the diff hunks (2)
tests/gui/test_commands.py (2)
Line range hint
357-357
: Local variablelast_lf_frame
is assigned to but never used.
Line range hint
525-525
: f-string without any placeholders.- default_name += f".{adaptor.default_ext}" + default_name += f"{adaptor.default_ext}"
…ngSession` serialization) (#1749) * [wip] Serialize FrameGroup class * [wip] Serialize InstanceGroup class * [untested] Serialize FrameGroup and InstanceGroup * Test InstanceGroup serialization and deserialization * Test FrameGroup serialization and deserialization * Move FrameGroup._cams_to_include and _excluded_views class attributes to RecordingSession instance attributes * Change InstanceGroup.camera_cluster from a class attribute to an instance attribute * Test RecordingSession serialize and deserialize * Get `Labels.from_json` working with instances_list * (De)serialize using `Labels.labeled_frames` instead of `Labels.instances()` * Fix `RecordingSession` unstructure hook bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Actionable comments outside the diff hunks (11)
sleap/io/format/labels_json.py (3)
Line range hint
385-385
: Useisinstance()
instead of comparing types directly.- if type(data) is str: + if isinstance(data, str):
Line range hint
410-410
: Avoid using bareexcept
statements; specify exception types where possible.- except: + except Exception:
Line range hint
434-434
: Investigate the unused variablenew_skel
. If it's not needed, consider removing it to clean up the code.sleap/io/dataset.py (8)
Line range hint
69-69
: Replace the bareexcept
with a specific exception type to avoid catching unexpected exceptions.
Line range hint
934-934
: Useisinstance()
for type checking instead of comparing types directly.
Line range hint
1454-1454
: Useisinstance()
for type checking instead of comparing types directly.
Line range hint
2360-2360
: Undefined nameglob
. Importglob
from theglob
module.+ from glob import glob
Line range hint
2455-2455
: Avoid equality comparisons toFalse
; useif not ret:
for false checks.
Line range hint
2543-2543
: Avoid equality comparisons toFalse
; useif not ret:
for false checks.
Line range hint
2558-2558
: Undefined namesleap
. Ensure that thesleap
module is imported or available in the scope where it's being used.
Line range hint
2648-2648
: Useisinstance()
for type checking instead of comparing types directly.
* Return InstanceGroup when FrameGroup.add_instance_group * Restructure RecordingSession after LabeledFrames * Always use Labels.labeled_frames even if skip_labels * Add test while creating new fixture * Add min_session_frame_groups fixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Out of diff range and nitpick comments (17)
sleap/io/format/labels_json.py (1)
Line range hint
238-239
: Improve the error logging to provide more context.- logger.warning(e) + logger.warning("Error while loading `RecordingSession`s: %s", str(e))sleap/io/dataset.py (16)
Line range hint
55-55
: Consider removing the duplicate import ofCallable
from line 45.
Line range hint
61-61
: Remove the unused import ofh5py
.
Line range hint
69-69
: Replace the bareexcept
with a more specific exception to avoid catching unexpected exceptions.
Line range hint
70-70
: Remove the unused import oftyping._ForwardRef
.
Line range hint
365-365
: Replace the lambda expression with adef
for better readability and debugging.
Line range hint
367-370
: Replace the lambda expressions withdef
for better readability and debugging.
Line range hint
372-375
: Replace the lambda expressions withdef
for better readability and debugging.
Line range hint
934-934
: Useisinstance()
for type checking instead of comparing types directly.
Line range hint
1454-1454
: Useisinstance()
for type checking instead of comparing types directly.
Line range hint
2359-2359
: Theglob
function is used but not imported. Ensure to importglob
from theglob
module.
Line range hint
2454-2454
: Simplify the boolean check by usingif not ret:
instead of comparing toFalse
.
Line range hint
2542-2542
: Simplify the boolean check by usingif not ret:
instead of comparing toFalse
.
Line range hint
2557-2557
: Thesleap
module is referenced but not imported. Ensure to import necessary components from thesleap
module.
Line range hint
2647-2647
: Useisinstance()
for type checking instead of comparing types directly.
Line range hint
2650-2650
: The variablee
is assigned but never used. Consider removing it or using it appropriately.
Line range hint
2652-2652
: The f-string does not contain any placeholders. Use a regular string if no formatting is needed.
instance = self.instances[0] | ||
|
||
# Use the example instance to create a dummy instance | ||
skeleton: "Skeleton" = instance.skeleton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined name 'Skeleton' in this scope.
Ensure that Skeleton
is imported or defined before use. This issue was flagged by static analysis and is a repeat of a previous comment.
""" | ||
|
||
# Get the `Skeleton` | ||
skeleton: "Skeleton" = self.dummy_instance.skeleton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined name 'Skeleton' in this scope.
Ensure that Skeleton
is imported or defined before use. This issue was flagged by static analysis and is a repeat of a previous comment.
@@ -414,7 +408,7 @@ def from_json_data( | |||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify exception types to improve error handling.
- except:
+ except Exception:
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: |
* Add (failing) test for TrangulateSession.has_enough_instances * Add (failing) test for TriangulateSession.do_action * Directly pass in session to triangulateSession for now * Add (failing) tests for relevant FrameGroup and InstanceGroup methods * Fix failing InstanceGroup and FrameGroup tests * Return from TriangulateSession.do_action if no session or frame_group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Out of diff range and nitpick comments (3)
sleap/io/cameras.py (3)
18-18
: Consider adding a brief class-level docstring toCamcorder
to improve code readability and maintainability.
18-18
: Consider adding a brief class-level docstring toCameraCluster
to improve code readability and maintainability.
18-18
: Consider adding a brief class-level docstring toInstanceGroup
to improve code readability and maintainability.
# Replot connected views for multi-camera projects | ||
# TODO(LM): Use context.state["session"] in command instead (when implemented) | ||
session = self.labels.get_session(video) | ||
self.commands.triangulateSession(session=session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider addressing the TODO regarding the use of context.state["session"]
for future improvements.
Would you like me to help implement this or should I open a GitHub issue to track this enhancement?
def triangulateSession( | ||
self, | ||
session: Optional[RecordingSession] = None, | ||
frame_idx: Optional[int] = None, | ||
instance: Optional[Instance] = None, | ||
): | ||
"""Triangulates `Instance`s for selected views in a `RecordingSession`.""" | ||
self.execute( | ||
TriangulateSession, | ||
session=session, | ||
frame_idx=frame_idx, | ||
instance=instance, | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper documentation for the triangulateSession
method.
The method triangulateSession
lacks a docstring. Adding a docstring would help other developers understand the purpose of the method, its parameters, and its return type, if any.
instance_groups = ( | ||
[instance_group] | ||
if instance_group is not None | ||
else frame_group.instance_groups | ||
) | ||
|
||
# Retain instance groups that have enough views/instances for triangulation | ||
instance_groups = TriangulateSession.has_enough_instances( | ||
frame_group=frame_group, | ||
instance_groups=instance_groups, | ||
frame_idx=frame_idx, | ||
instance=instance, | ||
) | ||
if instance_groups is None or len(instance_groups) == 0: | ||
return # Not enough instances for triangulation | ||
|
||
# Get the `FrameGroup` of shape M=include x T x N x 2 | ||
fg_tensor = frame_group.numpy(instance_groups=instance_groups, pred_as_nan=True) | ||
|
||
# Add extra dimension for number of frames | ||
frame_group_tensor = np.expand_dims(fg_tensor, axis=1) # M=include x F=1 xTxNx2 | ||
|
||
# Triangulate to one 3D pose per instance | ||
points_3d = triangulate( | ||
p2d=frame_group_tensor, | ||
calib=session.camera_cluster, | ||
excluded_views=frame_group.excluded_views, | ||
) # F x T x N x 3 | ||
|
||
# Reproject onto all views | ||
pts_reprojected = reproject( | ||
points_3d, | ||
calib=session.camera_cluster, | ||
excluded_views=frame_group.excluded_views, | ||
) # M=include x F=1 x T x N x 2 | ||
|
||
# Sqeeze back to the original shape | ||
points_reprojected = np.squeeze(pts_reprojected, axis=1) # M=include x TxNx2 | ||
|
||
# Update or create/insert ("upsert") instance points | ||
frame_group.upsert_points( | ||
points=points_reprojected, | ||
instance_groups=instance_groups, | ||
exclude_complete=True, | ||
) | ||
|
||
@classmethod | ||
def has_enough_instances( | ||
cls, | ||
frame_group: FrameGroup, | ||
instance_groups: Optional[List[InstanceGroup]] = None, | ||
frame_idx: Optional[int] = None, | ||
instance: Optional[Instance] = None, | ||
) -> Optional[List[InstanceGroup]]: | ||
"""Filters out instance groups without enough instances for triangulation. | ||
|
||
Args: | ||
frame_group: The `FrameGroup` object to use. | ||
instance_groups: A list of `InstanceGroup` objects to use. Default is None. | ||
frame_idx: The frame index to use (only used in logging). Default is None. | ||
instance: The `Instance` object to use (only used in logging). Default None. | ||
|
||
Returns: | ||
A list of `InstanceGroup` objects with enough instances for triangulation. | ||
""" | ||
|
||
if instance is None: | ||
instance = "" # Just used for logging | ||
|
||
if frame_idx is None: | ||
frame_idx = "" # Just used for logging | ||
|
||
if instance_groups is None: | ||
instance_groups = frame_group.instance_groups | ||
|
||
if len(instance_groups) < 1: | ||
logger.warning( | ||
f"Require at least 1 instance group, but found " | ||
f"{len(frame_group.instance_groups)} for frame group {frame_group} at " | ||
f"frame {frame_idx}." | ||
f"\nSkipping triangulation." | ||
) | ||
return None # No instance groups found | ||
|
||
# Assert that there are enough views and instances | ||
instance_groups_to_tri = [] | ||
for instance_group in instance_groups: | ||
instances = instance_group.get_instances(frame_group.cams_to_include) | ||
if len(instances) < 2: | ||
# Not enough instances | ||
logger.warning( | ||
f"Not enough instances in {instance_group} for triangulation." | ||
f"\nSkipping instance group." | ||
) | ||
continue | ||
instance_groups_to_tri.append(instance_group) | ||
|
||
return instance_groups_to_tri # `InstanceGroup`s with enough instances | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the implementation of TriangulateSession
class.
- The method
do_action
in theTriangulateSession
class is quite complex and could benefit from further decomposition into smaller, more manageable functions. This would improve readability and maintainability. - The use of logging for error handling in the
has_enough_instances
method is noticed. It's generally a good practice to handle errors more robustly than just logging, especially in a method that performs important operations like triangulation. - The method uses numpy operations extensively, which are generally efficient, but the specific usage should be profiled to ensure that there are no performance bottlenecks, especially since this method might be dealing with large datasets.
…1757) * Add new `PredictedInstance`s to `LabeledFrame` * Replace `PredictedInstance` with `Instance` when double click * Remove (or replace) `Instance` when `Labels.remove_instance`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Out of diff range and nitpick comments (9)
sleap/io/dataset.py (2)
Line range hint
61-61
: Unused import detected.- import h5py as h5
Remove the unused import to clean up the code and avoid confusion about its necessity.
Line range hint
69-69
: Avoid using bareexcept
statements.- except: + except Exception:It's a good practice to specify the exception type to avoid catching unexpected exceptions and potentially masking other bugs.
sleap/gui/commands.py (7)
Line range hint
196-196
: Undefined name 'MainWindow' used.Please define
MainWindow
or import it if it's defined in another module.
Line range hint
834-834
: Local variablefile_dir
is assigned to but never used.Consider removing the assignment to
file_dir
since it's not used anywhere in the method.
Line range hint
1708-1708
: Avoid using bareexcept
statements.It's a good practice to specify the exceptions you are catching. This prevents catching unexpected exceptions and makes the code easier to understand.
- except: + except Exception as e:
Line range hint
1729-1729
: Avoid using bareexcept
statements.It's a good practice to specify the exceptions you are catching. This prevents catching unexpected exceptions and makes the code easier to understand.
- except: + except Exception as e:
Line range hint
2466-2466
: Remove unnecessary f-string.Since there are no placeholders in the string, the f-string can be converted to a regular string.
- f"An error occured when attempting to save:\n {e}\n\n" + "An error occured when attempting to save:\n {e}\n\n"
Line range hint
2798-2798
: Remove unnecessary f-string.Since there are no placeholders in the string, the f-string can be converted to a regular string.
- f"Importing {len(csv_files)} DeepLabCut datasets..." + "Importing {len(csv_files)} DeepLabCut datasets..."
Line range hint
3161-3161
: Avoid using bareexcept
statements.It's a good practice to specify the exceptions you are catching. This prevents catching unexpected exceptions and makes the code easier to understand.
- except: + except Exception as e:
self.remove_session_video(video=video) | ||
|
||
def remove_session_video(self, video: Video): | ||
"""Remove video from session in cache.""" | ||
|
||
# TODO(LM): Also remove LabeledFrames from frame_group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper cleanup of related data when removing a session video.
- if video in self._session_by_video:
- del self._session_by_video[video]
+ if video in self._session_by_video:
+ session = self._session_by_video[video]
+ del self._session_by_video[video]
+ session.remove_video(video) # Ensure the video is also removed from the session object
This change ensures that when a video is removed from the session in the cache, it is also properly removed from the session object itself, maintaining consistency across the data structure.
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.
self.remove_session_video(video=video) | |
def remove_session_video(self, video: Video): | |
"""Remove video from session in cache.""" | |
# TODO(LM): Also remove LabeledFrames from frame_group | |
self.remove_session_video(video=video) | |
def remove_session_video(self, video: Video): | |
"""Remove video from session in cache.""" | |
if video in self._session_by_video: | |
session = self._session_by_video[video] | |
del self._session_by_video[video] | |
session.remove_video(video) # Ensure the video is also removed from the session object | |
# TODO(LM): Also remove LabeledFrames from frame_group |
|
||
# TODO(LM): Remove LabeledFrame from any frame groups it's in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider implementing the removal of LabeledFrame
from frame groups.
This TODO comment suggests a need to handle the removal of LabeledFrame
from frame groups when removing empty frames. If you need assistance with this, I can help implement the necessary changes or open an issue to track this enhancement.
def remove_session_video(self, video: Video): | ||
"""Remove a video from its linked recording session (if any). | ||
|
||
Args: | ||
session: `RecordingSession` instance | ||
video: `Video` instance | ||
""" | ||
|
||
self._cache._session_by_video.pop(video, None) | ||
if video in session.videos: | ||
session = self.get_session(video) | ||
|
||
if session is None: | ||
return | ||
|
||
# Need to remove from cache first to avoid circular reference | ||
self._cache.remove_session_video(video=video) | ||
if session.get_camera(video) is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refine the method to remove a video from its linked recording session.
- if session is None:
- return
- # Need to remove from cache first to avoid circular reference
- self._cache.remove_session_video(video=video)
- if session.get_camera(video) is not None:
- session.remove_video(video)
+ if session:
+ # Ensure the video is removed from the session and cache properly
+ self._cache.remove_session_video(video=video)
+ session.remove_video(video)
This refactor ensures that the video is only removed if the session exists, and it consolidates the checks and operations into a more logical sequence, improving readability and maintainability.
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.
def remove_session_video(self, video: Video): | |
"""Remove a video from its linked recording session (if any). | |
Args: | |
session: `RecordingSession` instance | |
video: `Video` instance | |
""" | |
self._cache._session_by_video.pop(video, None) | |
if video in session.videos: | |
session = self.get_session(video) | |
if session is None: | |
return | |
# Need to remove from cache first to avoid circular reference | |
self._cache.remove_session_video(video=video) | |
if session.get_camera(video) is not None: | |
def remove_session_video(self, video: Video): | |
"""Remove a video from its linked recording session (if any). | |
Args: | |
video: `Video` instance | |
""" | |
session = self.get_session(video) | |
if session: | |
# Ensure the video is removed from the session and cache properly | |
self._cache.remove_session_video(video=video) | |
session.remove_video(video) |
@@ -35,14 +35,15 @@ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused imports to clean up the code.
- from itertools import permutations, product
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.
from itertools import permutations, product |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, Union
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.
from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union, cast | |
from typing import Callable, Dict, Iterator, List, Optional, Tuple, Type, Union |
@@ -53,7 +54,7 @@ | |||
from sleap.gui.state import GuiState | |||
from sleap.gui.suggestions import VideoFrameSuggestions | |||
from sleap.instance import Instance, LabeledFrame, Point, PredictedInstance, Track | |||
from sleap.io.cameras import RecordingSession | |||
from sleap.io.cameras import Camcorder, InstanceGroup, FrameGroup, RecordingSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import to clean up the code.
- from sleap.io.cameras import Camcorder, InstanceGroup, FrameGroup, RecordingSession
+ from sleap.io.cameras import InstanceGroup, FrameGroup, RecordingSession
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.
from sleap.io.cameras import Camcorder, InstanceGroup, FrameGroup, RecordingSession | |
from sleap.io.cameras import InstanceGroup, FrameGroup, RecordingSession |
* Add more tests for * Add more tests for `FrameGroup` * Add more tests * Add fixes (caught by tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
sleap/io/cameras.py (3)
Line range hint
79-79
: This f-string does not contain any placeholders, which makes the use of an f-string unnecessary. Consider using a regular string instead.- f"{self.__class__.__name__}(name={self.name}, size={self.size})" + "{self.__class__.__name__}(name={self.name}, size={self.size})"
Line range hint
80-80
: This f-string does not contain any placeholders, which makes the use of an f-string unnecessary. Consider using a regular string instead.- f"{self.__class__.__name__}(name={self.name}, size={self.size})" + "{self.__class__.__name__}(name={self.name}, size={self.size})"
Line range hint
81-81
: This f-string does not contain any placeholders, which makes the use of an f-string unnecessary. Consider using a regular string instead.- f"{self.__class__.__name__}(name={self.name}, size={self.size})" + "{self.__class__.__name__}(name={self.name}, size={self.size})"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name Labels
is undefined in this scope. Ensure that Labels
is imported or defined before use.
# isinstance(idx_or_key, Instance): | ||
try: | ||
return self.get_cam(idx_or_key) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using a bare except
as it can catch unexpected exceptions. Specify the exception type that you expect to handle.
- except:
+ except SpecificExceptionType:
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 SpecificExceptionType: |
Description
Initial implementation of updating
Instance
s across "views" := frame atframe_idx
forVideo
s inRecordingSession
. This PR does not handle selectively updating selected points in anInstance
, but it does it support filteringCamcorder
s for use in triangulation.Side note: As I was thinking of the best way to make something like this... it seems SLEAP could benefit from an
Observer
design pattern which would also help us reduce the amount of loops we do during callbacks (by ideally only looping through lists/items once and calling update for each listener within those loops). I did some skim reading, but did not dive into implementing the pattern due to time sensitivities - to be explored later.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
New Features
TriangulateSession
with various methods for session triangulation.update_points
to theInstance
class for updating instance points.RecordingSession
.Improvements
Bug Fixes
Documentation
Tests