(2->1) Add method to get RecordingSession via Video through Labels

[roomrys]

Description

Add a Labels.get_session method that uses the LabelsDataCache._session_by_video dictionary to look-up a RecordingSession using a Video.

Types of changes

• Bugfix
• New feature
• Refactor / Code style update (no logical changes)
• Build / CI changes
• Documentation Update
• Other (explain)

Does this address any currently open issues?

[list open issues here]

Outside contributors checklist

• Review the guidelines for contributing to this repository
• Read and sign the CLA and add yourself to the authors list
• Make sure you are making a pull request against the develop branch (not main). Also you should start your branch off develop
• Add tests that prove your fix is effective or that your feature works
• Add necessary documentation (if appropriate)

Thank you for contributing to SLEAP!

❤️

Summary by CodeRabbit

• New Feature: Added support for managing RecordingSession objects in the Labels class, including adding and removing sessions and videos.
• New Feature: Introduced a mapping between videos and sessions in the LabelsDataCache class to improve data retrieval efficiency.
• Refactor: Updated the update method in LabelsDataCache to handle different types of input and rebuild the cache accordingly.
• Test: Enhanced test coverage for session and video management in the Labels class.

[roomrys]

Update

The issue described below ended up being caused by a change in the data structure for SingleImageVideos. After finding the issue, the data structure change was reversed in #1330 and never made it to the main branch, but was on develop for a good while....

---

There's a pretty annoying and seemingly not easy to get rid of problem with tests/data/videos/robot0.jpg that results in intermittent failures in our tests. I previously tried replacing the slp that loads these images, but upon further inspection, it seems I should replace the jpg. Another idea is that this might happen due to having both the Windows and Ubuntu tests accessing these files at the same time? I need to look into that to be sure.

Traceback

# Test reset does not break deserialization of older slp
        labels: Labels = Labels.load_file(
>           TEST_SLP_SIV_ROBOT, video_search="tests/data/videos/"
        )

tests/io/test_video.py:552: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
sleap/io/dataset.py:2068: in load_file
    filename, for_object="labels", video_search=video_search, *args, **kwargs
sleap/io/format/main.py:113: in read
    return disp.read(filename, *args, **kwargs)
sleap/io/format/dispatch.py:56: in read
    return adaptor.read(file, *args, **kwargs)
sleap/io/format/hdf5.py:139: in read
    labels = cls.read_headers(file, video_search, match_to)
sleap/io/format/hdf5.py:124: in read_headers
    labels = labels_json.LabelsJsonAdaptor.from_json_data(dicts, match_to=match_to)
sleap/io/format/labels_json.py:391: in from_json_data
    videos = Video.cattr().structure(dicts["videos"], List[Video])
/usr/share/miniconda/envs/sleap_ci/lib/python3.7/site-packages/cattr/converters.py:201: in structure
    return self._structure_func.dispatch(cl)(obj, cl)
/usr/share/miniconda/envs/sleap_ci/lib/python3.7/site-packages/cattr/converters.py:336: in _structure_list
    for e in obj
/usr/share/miniconda/envs/sleap_ci/lib/python3.7/site-packages/cattr/converters.py:336: in <listcomp>
    for e in obj
/usr/share/miniconda/envs/sleap_ci/lib/python3.7/site-packages/cattr/converters.py:[323](https://github.com/talmolab/sleap/actions/runs/4727432169/jobs/8388031323?pr=1278#step:8:324): in structure_attrs_fromdict
    dispatch(type_)(val, type_) if type_ is not None else val
/usr/share/miniconda/envs/sleap_ci/lib/python3.7/site-packages/cattr/converters.py:[407](https://github.com/talmolab/sleap/actions/runs/4727432169/jobs/8388031323?pr=1278#step:8:408): in _structure_union
    return self._structure_func.dispatch(cl)(obj, cl)
sleap/io/video.py:1540: in fixup_video
    return Video.make_specific_backend(cl, x)
sleap/io/video.py:1522: in make_specific_backend
    return backend_class(**attribute_kwargs)
<attrs generated init sleap.io.video.NumpyVideo>:3: in __init__
    self.__attrs_post_init__()
sleap/io/video.py:538: in __attrs_post_init__
    self.__data = np.load(self.filename)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

file = 'tests/data/videos/robot0.jpg', mmap_mode = None, allow_pickle = False
fix_imports = True, encoding = 'ASCII'

    @set_module('numpy')
    def load(file, mmap_mode=None, allow_pickle=False, fix_imports=True,
             encoding='ASCII'):
        """
        Load arrays or pickled objects from ``.npy``, ``.npz`` or pickled files.
        """

        ....
                if not allow_pickle:
>                   raise ValueError("Cannot load file containing pickled data "
                                     "when allow_pickle=False")
E                   ValueError: Cannot load file containing pickled data when allow_pickle=False 

[codecov]

Codecov Report

Merging #1278 (81be658) into liezl/acg-add-recording-session (24a9e82) will increase coverage by 0.02%.
The diff coverage is 91.17%.

@@                         Coverage Diff                         @@
##           liezl/acg-add-recording-session    #1278      +/-   ##
===================================================================
+ Coverage                            73.57%   73.60%   +0.02%     
===================================================================
  Files                                  135      135              
  Lines                                24318    24366      +48     
===================================================================
+ Hits                                 17893    17935      +42     
- Misses                                6425     6431       +6     

Files	Coverage Δ
sleap/io/cameras.py	89.70% <80.00%> (-0.19%)	⬇️
sleap/io/dataset.py	84.10% <92.06%> (+0.18%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[coderabbitai]

Walkthrough

This update enhances the Labels and RecordingSession classes with new methods for managing sessions and videos. It also introduces changes to the LabelsDataCache class to handle different types of input and maintain a mapping between videos and sessions.

Changes

File	Summary
sleap/io/cameras.py	Added import statements, updated add_video and remove_video methods in RecordingSession class to manage labels cache, added __attrs_post_init__ method.
sleap/io/dataset.py	Renamed _sessions attribute to sessions in Labels class, added new methods for session management, modified update method in LabelsDataCache class to handle various inputs, added _session_by_video dictionary.
tests/io/test_dataset.py	Updated test functions to validate new functionality of adding and removing sessions and videos from a Labels object.

"With each commit we make, 🐇
Our code becomes less opaque. 💻
Now managing sessions is a piece of cake, 🍰
And our tests ensure no mistake." 🧪

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• Mention @coderabbitai in any review comment for bot assistance.
• Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

• Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
• Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
  resume reviews, enter @coderabbitai resume as any issue or review comment.

[coderabbitai]

Review Status

Actionable comments generated: 3

Commits

Files that changed from the base of the PR and between 348a656 and 5a7dc09.

Files selected for processing (3)

• sleap/io/cameras.py (4 hunks)
• sleap/io/dataset.py (8 hunks)
• tests/io/test_dataset.py (2 hunks)

Additional comments (Suppressed): 13

sleap/io/cameras.py (4)

• 17-18: The commented import statement indicates a circular import issue. This can lead to problems in the code execution and is generally considered a bad practice. It's mentioned that an Observer pattern might be implemented to resolve this, which could be a good solution as it would allow Labels to be notified of changes without directly importing it. Please ensure this is addressed before merging.

• 403-407: A new attribute labels has been added to the RecordingSession class. This attribute is optional and can hold an instance of the Labels class. Ensure that all instances where RecordingSession is instantiated are updated to include this new attribute if necessary.

• 513-515: The labels cache is being updated when a video is added to the RecordingSession. This is a good practice as it ensures that the cache is always up-to-date with the latest state of the RecordingSession.

• 537-539: Similar to the addition of a video, the labels cache is also updated when a video is removed from the RecordingSession. This maintains the consistency of the cache with the current state of the RecordingSession.

tests/io/test_dataset.py (2)

• 1-21: No significant changes in the import statements. The import statement for load_labels_json_old from sleap.io.legacy has been removed, which is not used in the new hunks.

• 999-1004: The test case checks if the loaded session's labels match the loaded labels. This is a good practice to ensure data consistency after loading.

sleap/io/dataset.py (7)

• 106-111: The function signature of update has been changed to accept a new parameter new_item which can be of type LabeledFrame, RecordingSession, or a tuple of RecordingSession and Video. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 128-141: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [119-135]

A new dictionary _session_by_video is introduced to map videos to their respective sessions. This is a good approach for quick lookups, but it's important to ensure that this dictionary is always kept in sync with the actual data. The code seems to handle this well by updating the dictionary in the update method whenever a new item is added.

• 248-249: Good job on ensuring that when a video is removed, it is also removed from the _session_by_video dictionary. This helps keep the data consistent.

• 448-451: The attribute sessions has been made private (_sessions) and a property getter and setter have been added for it. This is a good practice as it allows for better control over how this attribute is accessed and modified.

• 1637-1642: The add_session method has been updated to also update the cache when a new session is added. This is a good practice as it ensures that the cache is always up-to-date with the actual data.

• 1659-1668: The get_session method uses the _session_by_video dictionary in the cache to quickly get the session associated with a video. This is an efficient way to retrieve this information.

• 1670-1680: The remove_session_video method removes a video from a session and also updates the cache accordingly. This is a good practice as it ensures that the cache is always in sync with the actual data.

[coderabbitai]

Review Status

Actionable comments generated: 0

Commits

Files that changed from the base of the PR and between 5a7dc09 and 6f564f7.

Files selected for processing (2)

• sleap/io/cameras.py (4 hunks)
• sleap/io/dataset.py (6 hunks)

Additional comments (Suppressed): 12

sleap/io/cameras.py (3)

• 414-418: The addition of the labels attribute to the RecordingSession class is a significant change. This attribute links a RecordingSession to a Labels object, which represents a set of labels for a video dataset. The comment on line 414 suggests that an Observer pattern might be implemented in the future for camera_cluster and labels. This would allow changes in these objects to automatically update related objects, improving code maintainability and reducing potential errors.

• 521-527: The add_video method now updates the labels cache when a video is added to the RecordingSession. This ensures that the labels object is kept up-to-date with the current state of the RecordingSession, improving data consistency. However, it's important to ensure that the update_session method in the Labels class correctly handles this update.

if self.labels is not None:
    self.labels.update_session(self, video)

• 545-550: Similarly, the remove_video method now updates the labels cache when a video is removed from the RecordingSession. This also helps maintain data consistency. Again, it's crucial to verify that the remove_session_video method in the Labels class correctly handles this update.

if self.labels is not None and self.labels.get_session(video) is not None:
    self.labels.remove_session_video(self, video)

sleap/io/dataset.py (9)

• 103-131: The rebuild_cache method has been updated to include a new dictionary _session_by_video. This dictionary maps each video to its corresponding recording session. The logic seems correct and efficient as it loops through all sessions and videos only once. However, please ensure that the labels.sessions attribute is correctly populated before this method is called.

• 146-155: The add_recording_session method has been added to update the _session_by_video cache when a new recording session is added. It correctly updates the cache for each video in the new session.

• 156-165: The add_video_to_session method has been added to update the _session_by_video cache when a new video is added to a session. It correctly updates the cache with the new video-session pair.

• 166-185: The update method has been refactored to handle different types of input and call the appropriate methods to update the cache. It correctly identifies the type of the new item and calls the corresponding method. However, it might be more maintainable to split this into separate methods for each type of input instead of using isinstance checks.

def update_labeled_frame(self, new_frame: LabeledFrame):
    self.add_labeled_frame(new_frame)

def update_recording_session(self, new_session: RecordingSession):
    self.add_recording_session(new_session)

def update_video_to_session(self, session_video_pair: Tuple[RecordingSession, Video]):
    self.add_video_to_session(*session_video_pair)

• 270-279: The remove_video method has been updated to remove a video from the _session_by_video cache if it exists. This is a necessary step to keep the cache consistent.

• 477-480: The _sessions attribute has been renamed to sessions and a new setter method has been added. This change makes the attribute private and prevents direct assignment, which is good for encapsulation.

• 492-506: The __attrs_post_init__ method has been updated to set the labels property of each session to self. This is necessary to keep the back-reference from each session to its labels object up-to-date.

• 652-658: A setter method has been added for the sessions attribute that raises a ValueError when an attempt is made to directly assign to it. This enforces the use of the add_session method to add sessions, which is good for maintaining consistency in the cache.

• 1669-1715: New methods update_session, get_session, and remove_session_video have been added to update the session-video map in the cache, retrieve the associated session of a video, and remove a video from a session respectively. These methods correctly interact with the cache and the session objects.

[coderabbitai]

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.