(3->2) Add method to update Instances 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
roomrys
changed the title
Instances across views in RecordingSessionInstances 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 (
|
![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: 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
videoandinstanceare retrieved from thestatedictionary. Ensure that these keys always exist in thestatedictionary before this method is called, or add error handling for cases where they might not be present.1236-1242: The
update_viewsmethod is called on thesessionobject withframe_idxandtrackas arguments. Make sure that all calls to this function throughout the codebase have been updated to match the new signature.1244-1247: The
updatePointsmethod is called withcomplete=Falseanduser_change=False. Ensure that these parameters are correctly handled in theupdatePointsmethod and that their implications are well understood.sleap/instance.py (1)
- 1397-1405: The sorting of instances in the
findfunction is a good idea to ensure thatPredictedInstancesare 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
triangulateandreprojectfunctions from thesleap_aniposemodule 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.
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.
How would you propose we optimize this part of the code?
There was a problem hiding this comment.
@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.
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.
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_videomethod 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_camerasretrieves allVideos from selectedCamcorders. If noCamcorders 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_viewsretrieves allInstancesacross all views at a given frame index. It also allows for filtering byCamcorders andTrack. Ifrequire_multiple_viewsis 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_pointstriangulates and reprojects instance coordinates. It first gathers instances into an array, then uses thetriangulateandreprojectfunctions fromsleap_aniposeto 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_viewsupdates the views of theRecordingSessionby triangulating 3D points from multiple views and updating the instances with these new points. It first checks if there are enoughCamcorders 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.
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
Labelsfromsleap.io.datasethas been added. Ensure that this new import is used in the subsequent code changes.63-66: The expected length of
camera_clusterhas 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_mapdictionary 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_sessionshas 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_videomethod 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_camerasretrieves 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_viewsretrieves all instances across all views at a given frame index. It allows for optional filtering by camcorders and tracks. Ifrequire_multiple_viewsis 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_pointstriangulates 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_viewsupdates 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_mapare 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_mapare 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.
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, andLabelsfromsleap.io.datasetis 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: Labelshas 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_mapdictionary 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_camerasmethod. These tests seem to cover different scenarios and look well-written.262-303: New tests have been added for the
get_instances_across_viewsmethod. 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, reprojecthas 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 selectedCamcorders. 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
RecordingSessionclass 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_mapare 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_mapare now converted back to integers when retrievingCamcorderandVideoobjects. 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.
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_clusterhas 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: Labelshas 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_mapdictionary 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_camerasmethod. 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_viewsmethod. 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_videomethod 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_camerasretrieves videos from selected cameras. It's a good addition for modularity and reusability.647-672: The new method
calculate_reprojected_pointstriangulates and reprojects instance coordinates. The use of numpy operations should ensure good performance.673-728: The new method
update_viewsupdates the views of theRecordingSession. It uses the previously defined methodsget_instances_across_viewsandcalculate_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_mapare 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_mapare 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.
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 theRecordingSessionclass. 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_mapare 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_mapare 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.
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, andLabelsDataCachefromsleap.io.datasetis 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_clusterhas 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_mapdictionary 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
RecordingSessionclass. Ensure that these tests cover all possible edge cases and that they pass successfully.sleap/gui/app.py (3)
1228-1233: The
videoandinstanceare retrieved from thestatedictionary. Ensure that these keys are always present in thestatedictionary 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_viewsmethod is called on thesessionobject. 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 thesessionobject is always non-null before calling methods on it.1248-1249: The
updatePointsmethod 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_camerasretrieves allVideoobjects from selectedCamcorderobjects. If noCamcorderobjects 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_frameretrieves all views at a given frame index. It uses theget_videos_from_selected_camerasmethod to get the videos and then retrieves theLabeledFrameobjects 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_viewsretrieves allInstanceobjects across all views at a given frame index. It uses theget_all_views_at_framemethod to get the views and then retrieves theInstanceobjects 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_viewscalculates the views that are excluded from a dictionary ofCamcordertoInstance. This method improves the modularity of the code by providing a reusable function for calculating excluded views.686-732: The new method
calculate_reprojected_pointstriangulates and reprojects instance coordinates. It uses thecalculate_excluded_viewsmethod 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_instancestriangulates, reprojects, and updates the coordinates ofInstanceobjects. It uses thecalculate_reprojected_pointsmethod 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_viewsupdates the views of theRecordingSession. It uses theget_instances_across_viewsandupdate_instancesmethods 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 theRecordingSessionin 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_mapare 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_mapare now converted to integers before being used to index intosession.camera_cluster.camerasandvideos_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.
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_aniposefor triangulation and reprojection functions is new. Ensure that this package is included in the project dependencies.614-637: A new method
triangulateSessionhas been added. This method seems to be used for triangulatingInstances for selected views in aRecordingSession. Ensure that this method is being called correctly in the codebase.3292-3726: A new class
TriangulateSessionhas 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_websitefunction has been moved to the end of the file.Ensure that the
TriangulateSessionclass is being used correctly in the codebase and that theopen_websitefunction is still accessible after being moved.tests/gui/test_commands.py (3)
- 5-5: The
Dictimport is missing in the old hunk. Ensure that the addition ofDictdoes not break any existing functionality.- from typing import List + from typing import Dict, List
- 21-21: The
TriangulateSessionimport is new. Ensure that the addition ofTriangulateSessiondoes not break any existing functionality.+ TriangulateSession,
- 960-1298: The new tests added for
TriangulateSessionare 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, andLabelsDataCachefromsleap.io.datasetis new. Ensure that these are used appropriately in the tests.68-68: The number of cameras in the
camera_clusterhas 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_camerashas been updated to compare the length ofsession_1.camera_clusterwithsession_2.camera_clusterinstead ofsession.camera_cluster. This change makes the function more generic and reusable.210-212: The
camcorder_to_video_idx_mapnow 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_camerasandremove_videoof theRecordingSessionclass. 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_updatemethod has been updated to include a call toself.commands.triangulateSessionfor replotting connected views in multi-camera projects. However, thecams_to_includeparameter 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_changemethod has been changed to include aselected_instparameter. 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 theRecordingSessionobject when it is printed.683-688: No significant changes have been made in these lines. The code is still iterating over the
camera_clusterand skippingCamcorders not linked to anyVideo.692-698: The keys and values in
camcorder_to_video_idx_mapare 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_mapare 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.
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
overlayvariable is now explicitly typed asTrackTrailOverlay. This is a good practice for improving code readability and maintainability.1241-1243: The
triangulateSessioncommand 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_changehas 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
Camcorderclass 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
triangulateSessionhas been added. This method triangulatesInstances for selected views in aRecordingSession. Ensure that this method is called correctly in the code.3390-3818: A new class
TriangulateSessionhas 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
videoandsessionvariables are retrieved from theparamsdictionary or from thecontextobject. Ensure that these variables are always available in theparamsdictionary or thecontextobject.3424-3425: The
instancesvariable is retrieved directly from theparamsdictionary. Ensure that this variable is always available in theparamsdictionary.3427-3427: The
update_instancesmethod of theTriangulateSessionclass is called. Ensure that this method is implemented correctly and that it updates the instances as expected.3447-3447: The
verify_views_and_instancesmethod of theTriangulateSessionclass is called. Ensure that this method is implemented correctly and that it verifies the views and instances as expected.3469-3472: The
video,session, andinstancevariables are retrieved from theparamsdictionary or from thecontextobject. Ensure that these variables are always available in theparamsdictionary or thecontextobject.3474-3476: The
frame_idxvariable is retrieved from theparamsdictionary or from thecontextobject. Ensure that this variable is always available in theparamsdictionary or thecontextobject.3483-3483: The
cams_to_includevariable is retrieved from theparamsdictionary or from thesessionobject. Ensure that this variable is always available in theparamsdictionary or thesessionobject.3495-3502: The
get_and_verify_enough_instancesmethod of theTriangulateSessionclass is called. Ensure that this method is implemented correctly and that it gets and verifies the instances as expected.3509-3509: The
instancesvariable is added to theparamsdictionary. Ensure that this variable is used correctly in the rest of the code.3514-3561: The
get_and_verify_enough_instancesmethod of theTriangulateSessionclass is implemented. This method gets all instances across views at a given frame index. If not enoughInstances 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_viewsmethod of theTriangulateSessionclass is implemented. This method checks if there are enoughCamcorders 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_viewsmethod of theTriangulateSessionclass is implemented. This method gets allInstancesacross 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_framemethod of theTriangulateSessionclass 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_matricesmethod of theTriangulateSessionclass 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_viewsmethod of theTriangulateSessionclass is implemented. This method gets excluded views from a dictionary ofCamcordertoInstance. Ensure that this method is implemented correctly and that it calculates the excluded views as expected.3743-3792: The
calculate_reprojected_pointsmethod of
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 (4 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments: 5
sleap/gui/commands.py (1)
- 41-45: The import from
sleap_aniposeis new and should be checked for correct usage in the rest of the file.
Verification result:
The new imports from
sleap_aniposeare correctly used in thesleap/gui/commands.pyfile. The functionstriangulateandreprojectare utilized in the methodstriangulateSession,calculate_reprojected_points, and within theTriangulateSessionclass. 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
numpyimport aligns with the PR's objective to enhance mathematical operations, likely for the new triangulation methods.18-27: The addition of
TriangulateSessionand related methods to the import list is consistent with the PR's objective to add methods for updatingInstances across views.955-957: The addition of the
test_AddSessionfunction 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
TriangulateSessionclass is consistent with the PR's objective to add methods for updatingInstances 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.
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 variableselfused 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 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 could be an oversight or unnecessary code.Consider removing or using the
file_dirvariable 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.
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, 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 |
| 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.
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.
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_frameis 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.
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 bareexceptstatements; 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 bareexceptwith 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. Importglobfrom theglobmodule.+ 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 thesleapmodule 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.
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 ofCallablefrom line 45.
Line range hint
61-61: Remove the unused import ofh5py.
Line range hint
69-69: Replace the bareexceptwith 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 adeffor better readability and debugging.
Line range hint
367-370: Replace the lambda expressions withdeffor better readability and debugging.
Line range hint
372-375: Replace the lambda expressions withdeffor 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: Theglobfunction is used but not imported. Ensure to importglobfrom theglobmodule.
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: Thesleapmodule is referenced but not imported. Ensure to import necessary components from thesleapmodule.
Line range hint
2647-2647: Useisinstance()for type checking instead of comparing types directly.
Line range hint
2650-2650: The variableeis 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.
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.
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.
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.
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 toCamcorderto improve code readability and maintainability.
18-18: Consider adding a brief class-level docstring toCameraClusterto improve code readability and maintainability.
18-18: Consider adding a brief class-level docstring toInstanceGroupto 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.
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.
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.
Review the implementation of TriangulateSession class.
- The method
do_actionin theTriangulateSessionclass 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_instancesmethod 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.
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 h5Remove the unused import to clean up the code and avoid confusion about its necessity.
Line range hint
69-69: Avoid using bareexceptstatements.- 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
MainWindowor import it if it's defined in another module.
Line range hint
834-834: Local variablefile_diris assigned to but never used.Consider removing the assignment to
file_dirsince it's not used anywhere in the method.
Line range hint
1708-1708: Avoid using bareexceptstatements.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 bareexceptstatements.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 bareexceptstatements.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.
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 objectThis 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.
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.
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.
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 |
| 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 |
| @@ -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.
Remove unused import to clean up the code.
- from sleap.io.cameras import Camcorder, InstanceGroup, FrameGroup, RecordingSession
+ from sleap.io.cameras import InstanceGroup, FrameGroup, RecordingSessionCommittable 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.
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})"
| # 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.
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.
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
Instances across "views" := frame atframe_idxforVideos inRecordingSession. This PR does not handle selectively updating selected points in anInstance, but it does it support filteringCamcorders 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
Observerdesign 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
TriangulateSessionwith various methods for session triangulation.update_pointsto theInstanceclass for updating instance points.RecordingSession.Improvements
Bug Fixes
Documentation
Tests