(1->0) Add RecordingSession data structure and integrate with Labels
#1260
Conversation
Codecov Report
@@ Coverage Diff @@
## liezl/add-camera-group #1260 +/- ##
=========================================================
Coverage ? 73.57%
=========================================================
Files ? 135
Lines ? 24318
Branches ? 0
=========================================================
Hits ? 17893
Misses ? 6425
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
* Add GUI/API for loading `RecordingSession` into `Labels` * Add structure/unstructure method for `CameraCluster` * Add test for (un)structuring `RecordingSession` * Add `RecordingSession.make_cattr` method * Add `RecordingSession` (un)structure method to `Labels` reader/writer
roomrys
changed the title
RecordingSession data structure (enhance Camcorder and CameraCluster)RecordingSession data structure and integrate with Labels
roomrys
changed the title
RecordingSession data structure and integrate with LabelsRecordingSession data structure and integrate with Labels
roomrys
changed the title
RecordingSession data structure and integrate with LabelsRecordingSession data structure and integrate with Labels
…sleap into liezl/acg-add-recording-session
WalkthroughThis pull request introduces significant enhancements to the SLEAP application, primarily focusing on adding support for recording sessions. It includes changes to the GUI, command structure, and data handling classes, along with corresponding updates to the test suite. Changes
TipsChat with CodeRabbit Bot (
|
![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: 8
Files selected for processing (10)
- sleap/gui/app.py (1 hunks)
- sleap/gui/commands.py (4 hunks)
- sleap/io/cameras.py (4 hunks)
- sleap/io/dataset.py (5 hunks)
- sleap/io/format/labels_json.py (6 hunks)
- sleap/util.py (2 hunks)
- tests/fixtures/cameras.py (1 hunks)
- tests/gui/test_commands.py (2 hunks)
- tests/io/test_cameras.py (2 hunks)
- tests/io/test_dataset.py (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/fixtures/cameras.py
Additional comments (Suppressed): 27
sleap/gui/app.py (1)
- 467-476: The new menu item "Add Recording Session..." has been added to the File menu. This triggers the
addSessioncommand which allows users to load aRecordingSessionfrom a file and add it to the project. Ensure that theaddSessioncommand is implemented correctly and handles all possible edge cases, such as invalid or non-existent files.sleap/gui/commands.py (4)
54-57: The import of
RecordingSessionfromsleap.io.camerasis new and seems to be necessary for the added functionality of handling recording sessions. Ensure that this module and class are available in the project.434-439: A new method
addSessionhas been added which triggers theAddSessioncommand. This command is responsible for adding a new recording session to the project. The implementation looks correct.1939-1954: The
AddSessionclass has been introduced to handle the addition of a new recording session. It uses theRecordingSession.loadmethod to load a session from a file and then adds it to the labels context. If no session is currently loaded, it sets the newly loaded session as the current session. This logic seems sound, but ensure that theRecordingSession.loadmethod handles errors appropriately, such as when the specified file does not exist or is not a valid session file.1956-1968: The
askmethod shows a GUI for adding a video to the project. It uses aFileDialogto allow the user to select a camera calibration file. The selected filename is stored in theparamsdictionary under the key"camera_calibration". This method returnsTrueif a filename was selected andFalseotherwise. This seems like a reasonable approach, but make sure that the rest of your code can handle the case whereaskreturnsFalse.sleap/io/dataset.py (4)
65-65: The import of
RecordingSessionfromsleap.io.camerasis new. Ensure that theRecordingSessionclass is correctly implemented and tested insleap.io.cameras.420-426: A new attribute
sessionshas been added to storeRecordingSessionobjects. This change seems appropriate given the context provided.1929-1946: The serialization process now includes
RecordingSessioninstances. Ensure that themake_cattrmethod in theRecordingSessionclass is correctly implemented and tested.1952-1958: The serialized data now includes
sessions. This change aligns with the addition of thesessionsattribute and its inclusion in the serialization process.sleap/util.py (2)
36-65: The
_DeepIterableConverterclass has been added. This class is a callable that performs deep conversion of an iterable. It takes two arguments:member_converter, which is a converter to apply to iterable members, anditerable_converter, which is an optional converter to apply to the iterable itself. The__call__method applies themember_converterto each member of the iterable and then applies theiterable_converterto the entire iterable if it is not None. The__repr__method returns a string representation of the converter.69-82: The
deep_iterable_converterfunction has been added. This function creates a_DeepIterableConverterobject with the givenmember_converteranditerable_converter. Ifmember_converteris a list or tuple, it combines them using theand_function fromattrs.validators.sleap/io/format/labels_json.py (5)
7-13: The import of
loggingis new in this hunk. Ensure that it's used appropriately throughout the code and doesn't introduce unnecessary overhead or verbosity in the application logs.18-24: The import of
RecordingSessionfromsleap.io.camerasis new. Make sure that theRecordingSessionclass is properly defined and tested, and that its usage in this file aligns with its intended purpose.503-514: Error handling has been added for loading
RecordingSessions. This is a good practice as it prevents the application from crashing due to exceptions during the loading process. However, ensure that these exceptions are handled appropriately and not just logged and ignored, as this could lead to silent failures.542-547: A structure hook has been registered for
RecordingSession. This is necessary for the correct deserialization ofRecordingSessionobjects. Ensure that the lambda function correctly handles the conversion from dictionary toRecordingSessionobject.556-562: The
sessionsattribute has been added to theLabelsobject creation. Ensure that theLabelsclass has been updated to include this new attribute and handle it correctly in all relevant methods.tests/gui/test_commands.py (1)
- 932-958: The new test function
test_AddSessionis well-written and covers two important scenarios: when no session is selected and when a session is already selected. It checks that theAddSessioncommand correctly adds a session to the labels and updates the current session in the context state. This test will help ensure that the new functionality for adding recording sessions works as expected.tests/io/test_dataset.py (3)
971-998: The test
test_save_labels_with_sessionschecks if the labels with sessions attribute can be saved and loaded correctly. It verifies that the loaded session is an instance ofRecordingSession, not the same object in memory, and has the same number of cameras and videos as the original session. The test also checks if all cameras in the loaded session are equal to the corresponding cameras in the original session. This test seems to cover all necessary aspects of saving and loading labels with sessions.1000-1007: The test
test_add_sessionchecks if aRecordingSessioncan be added to aLabelsobject. It verifies that after adding the session, thesessionsattribute of the labels contains the added session. This test is simple and straightforward.1010-1012: Ensure that the changes made in this PR do not affect the functionality tested by
test_labels_hdf5. Run this test after making the changes to verify that it still passes.tests/io/test_cameras.py (4)
11-21: The test function
test_camcorderhas been updated to include two new parameters:min_session_sessionandcentered_pair_vid. Ensure that these fixtures are correctly defined and provide the expected types of objects (RecordingSessionandVideo, respectively). TheCamcorderobject is now retrieved from theRecordingSessionobject instead of directly from aCameraClusterobject. This change reflects the introduction of theRecordingSessionclass and its integration with the existing classes.36-52: New tests have been added to check the
videosandsessionsproperties of theCamcorderclass, as well as the__getitem__method. These tests reflect the new functionality introduced in theCamcorderclass to support theRecordingSessionstructure. Ensure that these methods behave as expected when interacting withVideoandRecordingSessionobjects.54-123: The
test_camera_clusterfunction has been significantly expanded to test new functionality related to theRecordingSessionclass. New tests include checking thesessionsproperty and theadd_sessionmethod, as well as thevideosproperty and the__getitem__method with different types of keys (Video,Camcorder, andRecordingSession). These tests validate the changes made to theCameraClusterclass to support theRecordingSessionstructure. Ensure that these methods behave as expected when interacting withVideoandRecordingSessionobjects.125-244: A new test function
test_recording_sessionhas been added to test theRecordingSessiondata structure. This function includes comprehensive tests for various methods and properties of theRecordingSessionclass, including loading from a calibration file, adding and removing videos, accessing linked and unlinked cameras, and serialization/deserialization. These tests validate the functionality of theRecordingSessionclass and its integration with the existing classes.sleap/io/cameras.py (3)
156-166: The
camerasattribute ofCameraClusteris defined with a validator and converter that ensure its elements are instances ofCamcorder. This is good practice as it ensures data integrity for thecameraslist.241-243: The
get_videos_from_camcordermethod raises aValueErrorif the camcorder is not found inself.cameras. This is a good practice as it makes error handling more explicit.284-314: The
__getitem__method ofCameraClusteris well-implemented with clear handling for different types of keys. It correctly raises aKeyErrorwhen the key is not found.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (3)
- sleap/io/cameras.py (4 hunks)
- sleap/io/dataset.py (5 hunks)
- tests/gui/test_commands.py (3 hunks)
Additional comments (Suppressed): 13
sleap/io/dataset.py (6)
65-65: The import of
RecordingSessionfromsleap.io.camerasis new. Ensure that theRecordingSessionclass has been properly implemented and tested insleap.io.cameras.423-423: A new attribute
sessionsof typeList[RecordingSession]has been added to theLabelsclass. This attribute will hold a list of recording sessions associated with the labels.1589-1600: A new method
add_sessionhas been added to theLabelsclass. This method takes aRecordingSessioninstance as an argument and adds it to thesessionslist if it's not already present. The method checks if the input is an instance ofRecordingSession, raising aTypeErrorif it's not. This is a good practice for ensuring data integrity.1937-1938: A new unstructure hook has been registered for
RecordingSessioninstances. This hook converts aRecordingSessioninstance to its index in thesessionslist. Make sure this conversion is appropriate for your serialization needs.1949-1950: A converter for
RecordingSessioninstances is created using themake_cattrmethod of theRecordingSessionclass, passing thevideoslist as an argument. Ensure that themake_cattrmethod ofRecordingSessionhandles this correctly.1959-1959: The
sessionsattribute is now included in the serialized output. This change ensures thatRecordingSessioninstances associated with theLabelsobject are preserved when the object is serialized.tests/gui/test_commands.py (3)
11-14: The import statement has been updated to include the
AddSessioncommand fromsleap.gui.commands. This is in line with the summary of the pull request which mentions the addition of a new commandAddSession.23-23: The
RecordingSessionclass fromsleap.io.camerasis now being imported. This aligns with the PR summary that mentions the introduction of a newRecordingSessiondata structure.929-955: A new test function
test_AddSessionhas been added. This test checks the functionality of adding a recording session to the labels. It verifies that the session is correctly added and selected in the context state, and that multiple sessions can be added. The test seems comprehensive and covers different scenarios (no session selected, session selected).+def test_AddSession( + min_tracks_2node_labels: Labels, + min_session_calibration_toml_path: str, +): + """Test that adding a session works.""" + labels = min_tracks_2node_labels + camera_calibration = min_session_calibration_toml_path + + # Set-up CommandContext + context: CommandContext = CommandContext.from_labels(labels) + + # Case 1: No session selected + assert context.state["session"] is None + assert labels.sessions == [] + + params = {"camera_calibration": camera_calibration} + AddSession.do_action(context, params) + assert len(labels.sessions) == 1 + session = labels.sessions[0] + assert context.state["session"] is session + + # Case 2: Session selected + params = {"camera_calibration": camera_calibration} + AddSession.do_action(context, params) + assert len(labels.sessions) == 2 + assert context.state["session"] is session + assert labels.sessions[1] is not sessionsleap/io/cameras.py (4)
- 57-58: The
__eq__method is incomplete, it only checks if the other object is an instance ofCamcorderbut does not compare any attributes. You should complete this method by comparing relevant attributes of the two objects.- def __eq__(self, other): - if not isinstance(other, Camcorder): + def __eq__(self, other: Any) -> bool: + if not isinstance(other, Camcorder): + return False + return self.camera == other.camera and self.videos == other.videos
- 141-148: The
from_cameramethod raises aTypeErrorif the input is not aCamerainstance. However, the method signature suggests that it can also accept aFisheyeCamera. You should update the check to includeFisheyeCamera.- if not isinstance(cam, Camera): + if not isinstance(cam, (Camera, FisheyeCamera)):
- 202-207: The
get_videos_from_sessionmethod returnsNoneif the session is not found inself.sessions. This could potentially lead to issues if the return value is used without checking forNone. Consider raising an exception instead to make error handling more explicit.- return None + raise ValueError(f"RecordingSession {session} not found in {self}.")
- 219-222: Similar to the previous comment, the
get_session_from_videomethod returnsNoneif the video is not found inself.videos. Consider raising an exception instead.- return None + raise ValueError(f"Video {video} not found in any RecordingSession in {self}.")
sleap/io/cameras.py
Outdated
| def __attrs_post_init__(self): | ||
| self.camera_cluster = CameraCluster() |
There was a problem hiding this comment.
The __attrs_post_init__ method in the Camcorder class is initializing the camera_cluster attribute with a new instance of CameraCluster. This could potentially overwrite any existing CameraCluster that was previously assigned to the Camcorder. If this is the intended behavior, then it's fine. However, if you want to preserve any existing CameraCluster, you should check if camera_cluster is None before assigning a new CameraCluster.
def __attrs_post_init__(self):
- self.camera_cluster = CameraCluster()
+ if self.camera_cluster is None:
+ self.camera_cluster = CameraCluster()
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- sleap/io/cameras.py (4 hunks)
- tests/gui/test_commands.py (3 hunks)
- tests/io/test_cameras.py (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/gui/test_commands.py
Additional comments (Suppressed): 9
sleap/io/cameras.py (5)
- 111-112: The
__repr__method is referencingnameandsizeattributes which are not defined in theCamcorderclass. This will raise anAttributeErrorwhen__repr__is called. You should update this to reference the actual attributes of theCamcorderclass.- return f"{self.__class__.__name__}(name={self.name}, size={self.size})" + return f"{self.__class__.__name__}(camera={self.camera}, videos={len(self.videos)})"
- 143-150: The
from_cameramethod raises aTypeErrorif the input is not aCamerainstance. However, the method signature suggests that it can also accept aFisheyeCamera. You should update the check to includeFisheyeCamera.- if not isinstance(cam, Camera): + if not isinstance(cam, (Camera, FisheyeCamera)):
- 204-209: The
get_videos_from_sessionmethod returnsNoneif the session is not found inself.sessions. This could potentially lead to issues if the return value is used without checking forNone. Consider raising an exception instead to make error handling more explicit.- return None + raise ValueError(f"RecordingSession {session} not found in {self}.")
- 221-224: Similar to the previous comment, the
get_session_from_videomethod returnsNoneif the video is not found inself.videos. Consider raising an exception instead.- return None + raise ValueError(f"Video {video} not found in any RecordingSession in {self}.")
- 261-265: The
__attrs_post_init__method in theCameraClusterclass is initializing thecamerasattribute withself.cameras. This seems redundant asself.camerasis already being passed to the superclass's__init__method. If there's no additional processing needed forself.cameras, you can remove this line.def __attrs_post_init__(self): """Initialize `CameraCluster` object.""" - super().__init__(cameras=self.cameras, metadata=self.metadata) + super().__init__(metadata=self.metadata) for cam in self.cameras: cam.camera_cluster = selftests/io/test_cameras.py (4)
10-20: The
test_camcorderfunction has been updated to include testing of the newRecordingSessionclass. The test now includes aRecordingSessionandVideoobject as parameters, which are used to test theCamcorderdata structure's interaction with these new objects. This is a good addition as it ensures that theCamcorderclass works correctly with the newRecordingSessionclass.38-50: The tests for the
videosandsessionsproperties, as well as the__getitem__method, have been added to thetest_camcorderfunction. These tests ensure that videos can be added to aCamcorderobject through aRecordingSession, and that theCamcorderobject correctly maintains references to its associated sessions and videos. The__getitem__method is tested with both valid and invalid keys, ensuring that it behaves as expected in both scenarios.53-122: The
test_camera_clusterfunction has been significantly expanded to test the new functionality related to theRecordingSessionclass. It now tests thesessionsproperty, theadd_sessionmethod, thevideosproperty, and the__getitem__method with various types of keys. It also tests theto_calibration_dictandfrom_calibration_dictmethods, ensuring that they correctly serialize and deserialize the state of theCameraCluster. All these additions are important for verifying the correct behavior of theCameraClusterclass when interacting withRecordingSessionobjects.124-243: A new test function,
test_recording_session, has been added to test theRecordingSessiondata structure. This function tests all aspects of theRecordingSessionclass, including loading from a calibration file, accessing metadata, adding videos, removing videos, and serialization/deserialization. This is a comprehensive test that ensures theRecordingSessionclass behaves as expected in various scenarios.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- sleap/io/cameras.py (4 hunks)
Additional comments (Suppressed): 8
sleap/io/cameras.py (8)
54-58: The
__attrs_post_init__method in theCamcorderclass is still initializing thecamera_clusterattribute with a new instance ofCameraClusterif it's not already an instance ofCameraCluster. This could potentially overwrite any existing value ofcamera_cluster. Consider using a lazy initialization approach as suggested in previous comments.60-60: The
__eq__method is still incomplete. It only checks if the other object is an instance ofCamcorderbut does not compare any attributes. You should complete this method by comparing relevant attributes of the two objects.108-109: The
__hash__method is currently only considering thecameraattribute for generating the hash. If twoCamcorderinstances have the samecamerabut differentvideos, they will still have the same hash. However, based on the previous discussion, if theCameraattribute is the primary identifier for aCamcorder, then it makes sense to only include that in the hash computation.111-112: The
__repr__method is referencingnameandsizeattributes which are not defined in theCamcorderclass. This will raise anAttributeErrorwhen__repr__is called. You should update this to reference the actual attributes of theCamcorderclass. Based on the previous discussion, ifnameandsizeare attributes that aCamcordershould have, I would recommend adding them explicitly to theCamcorderclass. If they are attributes of specific types ofcameraobjects, then consider checking for these attributes in the__repr__method and handle cases when they are not present.143-150: The
from_cameramethod raises aTypeErrorif the input is not aCamerainstance. However, the method signature suggests that it can also accept aFisheyeCamera. You should update the check to includeFisheyeCamera. Based on the previous discussion, sinceFisheyeCamerais a subclass ofCamera, theisinstance(cam, Camera)check will returnTruefor bothCameraandFisheyeCamerainstances. Therefore, the current implementation is correct and doesn't need to be changed.204-209: The
get_videos_from_sessionmethod returnsNoneif the session is not found inself.sessions. This could potentially lead to issues if the return value is used without checking forNone. Consider raising an exception instead to make error handling more explicit.221-224: Similar to the previous comment, the
get_session_from_videomethod returnsNoneif the video is not found inself.videos. Consider raising an exception instead.261-265: The
__attrs_post_init__method in theCameraClusterclass is initializing thecamerasattribute withself.cameras. This seems redundant asself.camerasis already being passed to the superclass's__init__method. If there's no additional processing needed forself.cameras, you can remove this line.
…sleap into liezl/acg-add-recording-session
|
CodeRabbit review skipped By default, CodeRabbit only reviews PRs on the default branch. If you wish to have PRs reviewed on additional branches, you can configure that under the repository settings in the UI. |
Description
RecordingSessiondata structure (and a few changes to existingCamcorderandCameraClusterdata structures)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
RecordingSessionclass insleap.io.cameras, enabling management of videos and cameras within a session.CamcorderandCameraClusterclasses insleap.io.camerasfor better interaction withRecordingSession.RecordingSessionobjects insleap.io.datasetandsleap.io.format.labels_json.tests/gui/test_commands.py,tests/io/test_cameras.py, andtests/io/test_dataset.py.