sleap icon indicating copy to clipboard operation
sleap copied to clipboard
verified publisher icon talmolab

(3a -> 3) Add method to match instances across views

Open roomrys opened this issue 2 years ago • 3 comments

Description

This PR aims to add a method to match instances across views (without knowing Tracks).

Types of changes

  • [ ] Bugfix
  • [x] 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!

:heart:

Summary by CodeRabbit

  • Refactor
    • Improved type annotations and method naming for clarity in the AddInstances class.
    • Updated method parameters to accept more versatile data types, enhancing flexibility.
  • Tests
    • Updated test functions to align with the refactored AddInstances class methods.

roomrys avatar Oct 31 '23 05:10 roomrys

Walkthrough

The recent changes across the codebase focused on enhancing the AddInstances class in sleap/gui/commands.py. These updates include method renaming, parameter adjustments, and return type refinements. Additionally, related test functions in tests/gui/test_commands.py have been modified to align with these class changes.

Changes

File Path Summary
sleap/gui/commands.py Various updates to the AddInstances class, including method renaming, parameter changes, and return type enhancements. The typing import statement has also been revised.
sleap/gui/dialogs/delete.py Logic change in the _delete method to call remove_frame instead of remove when no instances are left in a labeled frame.
sleap/instance.py Updates in handling points within the update_points method, skipping NaN values, extracting attributes, and updating the points dictionary.
sleap/io/dataset.py Additions like a new method remove_session_video, updated docstrings, and TODO comments for handling LabeledFrame removal in various methods.
tests/fixtures/datasets.py Addition of multiview_min_session_user_labels fixture for loading user-labeled data in a minimal session.
tests/gui/test_commands.py Removal of test functions related to triangulating sessions and verifying instances across views.
tests/io/test_cameras.py Modifications in declarations and tests for InstanceGroup and FrameGroup entities.
tests/io/test_dataset.py Modification in calling remove_session_video with explicit parameter naming.

Poem

🐇 Code changes dance like leaves in the wind, 🍃

Bringing clarity, a new path to find. 🛤️

Methods refactored, tests now aligned, 🛠️

In the realm of code, improvements designed. 🌌

A tale of updates, in lines and in tests, 📜

Progress whispers in the coder's quests. 🌠

Recent Review Details

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between d83ea6457d4ee75caa4ee8549564040ae8db0f7d and 176efb26e30961980ddcb076c73032a14e8bba85.
Files selected for processing (9)
  • sleap/gui/commands.py (4 hunks)
  • sleap/gui/dialogs/delete.py (1 hunks)
  • sleap/instance.py (2 hunks)
  • sleap/io/cameras.py (8 hunks)
  • sleap/io/dataset.py (10 hunks)
  • tests/fixtures/datasets.py (1 hunks)
  • tests/gui/test_commands.py (2 hunks)
  • tests/io/test_cameras.py (3 hunks)
  • tests/io/test_dataset.py (1 hunks)
Files not summarized due to errors (1)
  • sleap/io/cameras.py: Error: Message exceeds token limit
Additional Context Used
Ruff (64)
sleap/gui/commands.py (10)

38-38: itertools.permutations imported but unused

38-38: itertools.product imported but unused

40-40: typing.cast imported but unused

196-196: Undefined name MainWindow

838-838: Local variable file_dir is assigned to but never used

1712-1712: Do not use bare except

1733-1733: Do not use bare except

2470-2470: f-string without any placeholders

2802-2802: f-string without any placeholders

3145-3145: Do not use bare except

sleap/instance.py (6)

89-89: Undefined name self

89-89: Undefined name self

411-411: Do not compare types, use isinstance()

449-449: Do not compare types, use isinstance()

503-503: Do not use bare except

1652-1652: Undefined name Labels

sleap/io/cameras.py (8)

6-6: typing.cast imported but unused

79-79: f-string without any placeholders

80-80: f-string without any placeholders

81-81: f-string without any placeholders

449-449: Undefined name Skeleton

602-602: Undefined name Skeleton

829-829: Do not use bare except

917-917: Undefined name Labels

sleap/io/dataset.py (16)

55-55: Redefinition of unused Callable from line 45

61-61: h5py imported but unused

69-69: Do not use bare except

70-70: typing._ForwardRef imported but unused

365-365: Do not assign a lambda expression, use a def

367-370: Do not assign a lambda expression, use a def

372-375: Do not assign a lambda expression, use a def

934-934: Do not compare types, use isinstance()

1454-1454: Do not compare types, use isinstance()

2355-2355: Undefined name glob

2450-2450: Avoid equality comparisons to False; use if not ret: for false checks

2538-2538: Avoid equality comparisons to False; use if not ret: for false checks

2553-2553: Undefined name sleap

2643-2643: Do not compare types, use isinstance()

2646-2646: Local variable e is assigned to but never used

2648-2648: f-string without any placeholders

tests/fixtures/datasets.py (1)

1-1: os imported but unused

tests/gui/test_commands.py (17)

5-5: typing.Dict imported but unused

7-7: numpy imported but unused

21-21: sleap.gui.commands.TriangulateSession imported but unused

25-25: sleap.io.cameras.Camcorder imported but unused

75-75: Ambiguous variable name: l

82-82: Ambiguous variable name: l

227-227: Avoid equality comparisons to True; use if okay: for truth checks

236-236: Avoid equality comparisons to True; use if okay: for truth checks

244-244: Avoid equality comparisons to True; use if okay: for truth checks

253-253: Avoid equality comparisons to True; use if okay: for truth checks

265-265: Avoid equality comparisons to True; use if okay: for truth checks

274-274: Avoid equality comparisons to True; use if okay: for truth checks

281-281: Avoid equality comparisons to True; use if okay: for truth checks

299-299: Avoid equality comparisons to True; use if okay: for truth checks

360-360: Local variable last_lf_frame is assigned to but never used

368-368: Avoid equality comparisons to True; use if video.backend.grayscale: for truth checks

528-528: f-string without any placeholders

tests/io/test_dataset.py (6)

119-119: Test for object identity should be is not

120-120: Test for membership should be not in

121-121: Test for object identity should be is not

692-692: Local variable new_labels_json is assigned to but never used

708-708: Local variable new_labels_json is assigned to but never used

1495-1495: Local variable track_insts is assigned to but never used

Learnings (1)
sleap/io/dataset.py (1)
User: roomrys"
PR: talmolab/sleap#1654
File: sleap/io/dataset.py:1717-1725
Timestamp: 2024-03-15T23:59:34.765Z
Learning: When removing a recording session using the `remove_recording_session` method in the `Labels` class, it's necessary to also update the `LabelsDataCache._session_by_video` dictionary to reflect the removal. This ensures that the mapping between videos and their corresponding recording sessions remains accurate.
Additional comments not posted (12)
sleap/gui/dialogs/delete.py (1)

219-219: Ensure proper handling when no instances are left in a labeled frame.

This change optimizes the removal process by directly removing the frame when it's empty, which is a logical improvement. Please ensure that remove_frame handles all necessary cleanup and state updates.

tests/fixtures/datasets.py (1)

283-288: New fixture for loading user-labeled data in a minimal session.

This fixture correctly loads the specified dataset and follows the established pattern for dataset fixtures in this file. Good job on maintaining consistency.

tests/io/test_cameras.py (2)

299-348: New function to create InstanceGroup objects.

This function is well-implemented and provides flexibility with the add_dummy parameter to test different scenarios. Ensure comprehensive tests cover all paths, including error handling and edge cases.

173-179: New method to create FrameGroup objects.

The method new_frame_group is correctly implemented and integrates well with the existing RecordingSession structure. Good use of assertions to validate the behavior.

tests/io/test_dataset.py (1)

1033-1033: Explicitly naming the video parameter in the function call enhances readability and reduces potential errors related to positional arguments. Good practice!

sleap/instance.py (1)

732-754: Consider handling NaN values more robustly in update_points.

The current implementation skips updating points if the new point is NaN or the old point is marked as complete. While this is generally a good approach, consider adding a mechanism to handle unexpected NaN values more gracefully, perhaps by logging or raising an exception if NaN values are not expected in this context.

sleap/io/dataset.py (1)

Line range hint 365-365: Avoid assigning lambda expressions directly to variables.

Consider defining a function with def instead of using a lambda expression for better readability and debuggability.

sleap/gui/commands.py (5)

Line range hint 1712-1712: Avoid using bare except statements as they can catch unexpected exceptions and hide bugs.

Consider specifying the exception types that you expect to handle.

Line range hint 1733-1733: Avoid using bare except statements as they can catch unexpected exceptions and hide bugs.

Consider specifying the exception types that you expect to handle.

Line range hint 2470-2470: The f-string does not contain any placeholders, which makes the use of f-string unnecessary.

Consider using a regular string if no formatting is needed.

Line range hint 2802-2802: The f-string does not contain any placeholders, which makes the use of f-string unnecessary.

Consider using a regular string if no formatting is needed.

Line range hint 3145-3145: Avoid using bare except statements as they can catch unexpected exceptions and hide bugs.

Consider specifying the exception types that you expect to handle.

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Oct 31 '23 05:10 coderabbitai[bot]

Codecov Report

Attention: Patch coverage is 52.03837% with 200 lines in your changes are missing coverage. Please review.

Project coverage is 73.29%. Comparing base (a2326d3) to head (c3a8173).

:exclamation: Current head c3a8173 differs from pull request most recent head 176efb2. Consider uploading reports for the commit 176efb2 to get more accurate results

Files Patch % Lines
sleap/io/cameras.py 54.23% 173 Missing :warning:
sleap/gui/commands.py 16.12% 26 Missing :warning:
sleap/instance.py 87.50% 1 Missing :warning:
Additional details and impacted files 
@@                                 Coverage Diff                                 @@
##           liezl/asc-initial-update-instances-across-views    #1579      +/-   ##
===================================================================================
- Coverage                                            73.71%   73.29%   -0.43%     
===================================================================================
  Files                                                  135      135              
  Lines                                                24502    24810     +308     
===================================================================================
+ Hits                                                 18062    18184     +122     
- Misses                                                6440     6626     +186

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 31 '23 05:10 codecov[bot]

@coderabbitai review

roomrys avatar Nov 03 '23 23:11 roomrys