-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(1->0) Add RecordingSession
data structure and integrate with Labels
#1260
base: liezl/add-camera-group
Are you sure you want to change the base?
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
RecordingSession
data structure (enhance Camcorder
and CameraCluster
)RecordingSession
data structure and integrate with Labels
RecordingSession
data structure and integrate with Labels
RecordingSession
data structure and integrate with Labels
RecordingSession
data structure and integrate with Labels
RecordingSession
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 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
addSession
command which allows users to load aRecordingSession
from a file and add it to the project. Ensure that theaddSession
command 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
RecordingSession
fromsleap.io.cameras
is 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
addSession
has been added which triggers theAddSession
command. This command is responsible for adding a new recording session to the project. The implementation looks correct.1939-1954: The
AddSession
class has been introduced to handle the addition of a new recording session. It uses theRecordingSession.load
method 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.load
method handles errors appropriately, such as when the specified file does not exist or is not a valid session file.1956-1968: The
ask
method shows a GUI for adding a video to the project. It uses aFileDialog
to allow the user to select a camera calibration file. The selected filename is stored in theparams
dictionary under the key"camera_calibration"
. This method returnsTrue
if a filename was selected andFalse
otherwise. This seems like a reasonable approach, but make sure that the rest of your code can handle the case whereask
returnsFalse
.sleap/io/dataset.py (4)
65-65: The import of
RecordingSession
fromsleap.io.cameras
is new. Ensure that theRecordingSession
class is correctly implemented and tested insleap.io.cameras
.420-426: A new attribute
sessions
has been added to storeRecordingSession
objects. This change seems appropriate given the context provided.1929-1946: The serialization process now includes
RecordingSession
instances. Ensure that themake_cattr
method in theRecordingSession
class is correctly implemented and tested.1952-1958: The serialized data now includes
sessions
. This change aligns with the addition of thesessions
attribute and its inclusion in the serialization process.sleap/util.py (2)
36-65: The
_DeepIterableConverter
class 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_converter
to each member of the iterable and then applies theiterable_converter
to the entire iterable if it is not None. The__repr__
method returns a string representation of the converter.69-82: The
deep_iterable_converter
function has been added. This function creates a_DeepIterableConverter
object with the givenmember_converter
anditerable_converter
. Ifmember_converter
is a list or tuple, it combines them using theand_
function fromattrs.validators
.sleap/io/format/labels_json.py (5)
7-13: The import of
logging
is 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
RecordingSession
fromsleap.io.cameras
is new. Make sure that theRecordingSession
class 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
RecordingSession
s. 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 ofRecordingSession
objects. Ensure that the lambda function correctly handles the conversion from dictionary toRecordingSession
object.556-562: The
sessions
attribute has been added to theLabels
object creation. Ensure that theLabels
class 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_AddSession
is well-written and covers two important scenarios: when no session is selected and when a session is already selected. It checks that theAddSession
command 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_sessions
checks 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_session
checks if aRecordingSession
can be added to aLabels
object. It verifies that after adding the session, thesessions
attribute 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_camcorder
has been updated to include two new parameters:min_session_session
andcentered_pair_vid
. Ensure that these fixtures are correctly defined and provide the expected types of objects (RecordingSession
andVideo
, respectively). TheCamcorder
object is now retrieved from theRecordingSession
object instead of directly from aCameraCluster
object. This change reflects the introduction of theRecordingSession
class and its integration with the existing classes.36-52: New tests have been added to check the
videos
andsessions
properties of theCamcorder
class, as well as the__getitem__
method. These tests reflect the new functionality introduced in theCamcorder
class to support theRecordingSession
structure. Ensure that these methods behave as expected when interacting withVideo
andRecordingSession
objects.54-123: The
test_camera_cluster
function has been significantly expanded to test new functionality related to theRecordingSession
class. New tests include checking thesessions
property and theadd_session
method, as well as thevideos
property and the__getitem__
method with different types of keys (Video
,Camcorder
, andRecordingSession
). These tests validate the changes made to theCameraCluster
class to support theRecordingSession
structure. Ensure that these methods behave as expected when interacting withVideo
andRecordingSession
objects.125-244: A new test function
test_recording_session
has been added to test theRecordingSession
data structure. This function includes comprehensive tests for various methods and properties of theRecordingSession
class, including loading from a calibration file, adding and removing videos, accessing linked and unlinked cameras, and serialization/deserialization. These tests validate the functionality of theRecordingSession
class and its integration with the existing classes.sleap/io/cameras.py (3)
156-166: The
cameras
attribute ofCameraCluster
is defined with a validator and converter that ensure its elements are instances ofCamcorder
. This is good practice as it ensures data integrity for thecameras
list.241-243: The
get_videos_from_camcorder
method raises aValueError
if the camcorder is not found inself.cameras
. This is a good practice as it makes error handling more explicit.284-314: The
__getitem__
method ofCameraCluster
is well-implemented with clear handling for different types of keys. It correctly raises aKeyError
when the key is not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 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
RecordingSession
fromsleap.io.cameras
is new. Ensure that theRecordingSession
class has been properly implemented and tested insleap.io.cameras
.423-423: A new attribute
sessions
of typeList[RecordingSession]
has been added to theLabels
class. This attribute will hold a list of recording sessions associated with the labels.1589-1600: A new method
add_session
has been added to theLabels
class. This method takes aRecordingSession
instance as an argument and adds it to thesessions
list if it's not already present. The method checks if the input is an instance ofRecordingSession
, raising aTypeError
if it's not. This is a good practice for ensuring data integrity.1937-1938: A new unstructure hook has been registered for
RecordingSession
instances. This hook converts aRecordingSession
instance to its index in thesessions
list. Make sure this conversion is appropriate for your serialization needs.1949-1950: A converter for
RecordingSession
instances is created using themake_cattr
method of theRecordingSession
class, passing thevideos
list as an argument. Ensure that themake_cattr
method ofRecordingSession
handles this correctly.1959-1959: The
sessions
attribute is now included in the serialized output. This change ensures thatRecordingSession
instances associated with theLabels
object are preserved when the object is serialized.tests/gui/test_commands.py (3)
11-14: The import statement has been updated to include the
AddSession
command 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
RecordingSession
class fromsleap.io.cameras
is now being imported. This aligns with the PR summary that mentions the introduction of a newRecordingSession
data structure.929-955: A new test function
test_AddSession
has 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 ofCamcorder
but 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_camera
method raises aTypeError
if the input is not aCamera
instance. 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_session
method returnsNone
if 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_video
method returnsNone
if 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (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 referencingname
andsize
attributes which are not defined in theCamcorder
class. This will raise anAttributeError
when__repr__
is called. You should update this to reference the actual attributes of theCamcorder
class.- 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_camera
method raises aTypeError
if the input is not aCamera
instance. 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_session
method returnsNone
if 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_video
method returnsNone
if 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 theCameraCluster
class is initializing thecameras
attribute withself.cameras
. This seems redundant asself.cameras
is 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_camcorder
function has been updated to include testing of the newRecordingSession
class. The test now includes aRecordingSession
andVideo
object as parameters, which are used to test theCamcorder
data structure's interaction with these new objects. This is a good addition as it ensures that theCamcorder
class works correctly with the newRecordingSession
class.38-50: The tests for the
videos
andsessions
properties, as well as the__getitem__
method, have been added to thetest_camcorder
function. These tests ensure that videos can be added to aCamcorder
object through aRecordingSession
, and that theCamcorder
object 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_cluster
function has been significantly expanded to test the new functionality related to theRecordingSession
class. It now tests thesessions
property, theadd_session
method, thevideos
property, and the__getitem__
method with various types of keys. It also tests theto_calibration_dict
andfrom_calibration_dict
methods, ensuring that they correctly serialize and deserialize the state of theCameraCluster
. All these additions are important for verifying the correct behavior of theCameraCluster
class when interacting withRecordingSession
objects.124-243: A new test function,
test_recording_session
, has been added to test theRecordingSession
data structure. This function tests all aspects of theRecordingSession
class, including loading from a calibration file, accessing metadata, adding videos, removing videos, and serialization/deserialization. This is a comprehensive test that ensures theRecordingSession
class behaves as expected in various scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- sleap/io/cameras.py (4 hunks)
Additional comments (Suppressed): 8
sleap/io/cameras.py (8)
54-58: The
__attrs_post_init__
method in theCamcorder
class is still initializing thecamera_cluster
attribute with a new instance ofCameraCluster
if 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 ofCamcorder
but 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 thecamera
attribute for generating the hash. If twoCamcorder
instances have the samecamera
but differentvideos
, they will still have the same hash. However, based on the previous discussion, if theCamera
attribute is the primary identifier for aCamcorder
, then it makes sense to only include that in the hash computation.111-112: The
__repr__
method is referencingname
andsize
attributes which are not defined in theCamcorder
class. This will raise anAttributeError
when__repr__
is called. You should update this to reference the actual attributes of theCamcorder
class. Based on the previous discussion, ifname
andsize
are attributes that aCamcorder
should have, I would recommend adding them explicitly to theCamcorder
class. If they are attributes of specific types ofcamera
objects, then consider checking for these attributes in the__repr__
method and handle cases when they are not present.143-150: The
from_camera
method raises aTypeError
if the input is not aCamera
instance. However, the method signature suggests that it can also accept aFisheyeCamera
. You should update the check to includeFisheyeCamera
. Based on the previous discussion, sinceFisheyeCamera
is a subclass ofCamera
, theisinstance(cam, Camera)
check will returnTrue
for bothCamera
andFisheyeCamera
instances. Therefore, the current implementation is correct and doesn't need to be changed.204-209: The
get_videos_from_session
method returnsNone
if 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_video
method returnsNone
if the video is not found inself.videos
. Consider raising an exception instead.261-265: The
__attrs_post_init__
method in theCameraCluster
class is initializing thecameras
attribute withself.cameras
. This seems redundant asself.cameras
is 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
RecordingSession
data structure (and a few changes to existingCamcorder
andCameraCluster
data 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
RecordingSession
class insleap.io.cameras
, enabling management of videos and cameras within a session.Camcorder
andCameraCluster
classes insleap.io.cameras
for better interaction withRecordingSession
.RecordingSession
objects insleap.io.dataset
andsleap.io.format.labels_json
.tests/gui/test_commands.py
,tests/io/test_cameras.py
, andtests/io/test_dataset.py
.