-
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
Import DLC with uniquebodyparts, add Tracks #1562
Conversation
WalkthroughThis pull request introduces significant changes to the 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: 3
Files ignored due to filter (3)
- tests/data/dlc/labeled-data/video/maudlc_testdata.csv
- tests/data/dlc/labeled-data/video/maudlc_testdata_v2.csv
- tests/data/dlc/maudlc_230_config.yaml
Files selected for processing (3)
- sleap/io/format/deeplabcut.py (5 hunks)
- sleap/nn/system.py (1 hunks)
- tests/io/test_formats.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sleap/nn/system.py
Additional comments (Suppressed): 6
tests/io/test_formats.py (1)
- 235-242: The
test_maudlc
function is parameterized with three different test data files. However, the functionread
is called only once inside the test function. This means that the same test will be run three times with different data, but only the results of the last run will be checked. If the intention is to run the test with each data file separately, theread
function call and subsequent assertions should be inside a loop that iterates over the test data files. If the intention is to run the test with all data files together, the test data files should be passed as a list to theread
function, assuming it supports this. Please verify the intended behavior.sleap/io/format/deeplabcut.py (5)
22-28: The import of
Track
fromsleap.instance
is new and is used later in the code to createTrack
objects for each animal. This change aligns with the PR summary which mentions the creation of Tracks with animal names.119-130: The
animal_names
list has been replaced with atracks
dictionary. This dictionary will storeTrack
objects for each animal, with the animal name as the key. This is a significant change and should be verified to ensure that it doesn't break any existing functionality that relies on theanimal_names
list.178-210: The logic for creating instances has been updated to include the creation of
Track
objects for each animal. This is a significant change and should be verified to ensure that it doesn't break any existing functionality that relies on the old instance creation logic.281-287: The code now checks for the presence of "uniquebodyparts" in the project data and adds them to the skeleton if they exist. This change aligns with the PR summary which mentions handling of unique body parts.
311-333: The logic for finding video files has been updated to handle different file extensions. This change aligns with the PR summary which mentions a bug fix related to assumptions about video file extensions.
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 ignored due to filter (2)
- tests/data/dlc/labeled-data/video/CollectedData_LM.csv
- tests/data/dlc/madlc_230_config.yaml
Files selected for processing (2)
- sleap/io/format/deeplabcut.py (5 hunks)
- tests/io/test_formats.py (2 hunks)
Additional comments (Suppressed): 8
tests/io/test_formats.py (3)
198-202: The test data for
test_madlc
has been reduced from three to two files. Ensure that this change does not affect the test's coverage and that all necessary scenarios are still being tested.234-241: A new test case
test_maudlc
has been introduced. This test case reads labeled data from CSV files and asserts various properties of the data structure. It checks the skeleton node names, video filenames, frame coordinates, number of instances per frame, and track information. This is a good addition to the test suite as it increases the coverage of the code.242-303: The
test_maudlc
function is well-structured and covers a wide range of assertions to ensure the correctness of the data structure. It checks the skeleton node names, video filenames, frame coordinates, number of instances per frame, and track information. This is a comprehensive test that should help catch any potential issues with the data structure.sleap/io/format/deeplabcut.py (5)
22-22: The import of
Dict
from thetyping
module is a new addition. Ensure that it is used appropriately in the code.119-130: The
tracks
dictionary has been introduced to store tracks for each animal. This is a change from the previous version where animal names were stored in a list. This change seems to be in line with the new feature of creating Tracks with animal names.178-210: The logic for creating instances has been modified to use the
tracks
dictionary. This is a significant change from the previous version where instances were created without tracks. Ensure that this change does not introduce any unexpected behavior.281-287: The
read()
function has been updated to add nodes from the "uniquebodyparts" field if present. This is a new feature and should be tested thoroughly to ensure it works as expected.311-334: The logic for finding video files has been modified to handle different file extensions. This is a significant change from the previous version where only ".mp4" files were considered. Ensure that this change does not introduce any unexpected behavior.
b482d53
to
523f48e
Compare
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 ignored due to filter (2)
- tests/data/dlc/labeled-data/video/CollectedData_LM.csv
- tests/data/dlc/madlc_230_config.yaml
Files selected for processing (2)
- sleap/io/format/deeplabcut.py (5 hunks)
- tests/io/test_formats.py (2 hunks)
Additional comments (Suppressed): 11
sleap/io/format/deeplabcut.py (5)
22-28: The import of
Track
fromsleap.instance
is new and is used for creating tracks for each animal. This is part of the new feature introduced in this PR.119-130: The
animal_names
list has been replaced with atracks
dictionary to store tracks for each animal. This is a significant change as it introduces the concept of tracks in the code.178-210: The logic for creating instances has been updated to use the
tracks
dictionary. A new track is created for each animal if it doesn't exist already. This is a significant change as it introduces the concept of tracks in the code.281-287: The code now correctly parses the DLC config file to handle unique body parts that do not belong to any individual animal. This is part of the bug fix in this PR.
311-334: The code has been updated to handle video files with ".avi" extension, as the previous version only supported ".mp4" files. This is part of the bug fix in this PR.
tests/io/test_formats.py (6)
198-202: The new hunk has removed the "tests/data/dlc/madlc_230_config.yaml" from the test data. If this is intentional, ensure that the test case
test_madlc
is still functioning as expected without this data. If it was removed by mistake, consider adding it back.234-240: The new hunk has added a new test case
test_maudlc
with three test data files. Ensure that these files exist and are in the correct format for the test case.249-255: The assertions are checking the properties of the labels object. Ensure that the expected values in the assertions match the actual values in the test data files.
257-263: The assertions are checking the number of labels and instances per frame. Ensure that the expected values in the assertions match the actual values in the test data files.
265-292: The assertions are checking the coordinates of the instances in the labels. Ensure that the expected values in the assertions match the actual values in the test data files.
295-303: The assertions are checking the properties of the tracks in the labels. Ensure that the expected values in the assertions match the actual values in the test data files.
Thanks for the contribution @getzze! Looks like there's still a failing test related to the tracks, but just ping us when you're ready for review :) |
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.
Codecov Report
@@ Coverage Diff @@
## develop #1562 +/- ##
===========================================
- Coverage 73.37% 73.36% -0.01%
===========================================
Files 134 134
Lines 23976 24009 +33
===========================================
+ Hits 17592 17614 +22
- Misses 6384 6395 +11
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@talmo all the tests pass now, it's ready for reviewing. |
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.
Looks great to me!
Description
Importing Deeplabcut project with multiple animals failed because:
uniquebodyparts
list. Unique body parts do not belong to any individual, so parsing the .csv file containing the labeled frames leads to key error of the type: Cannot find key ("animal", "unique_body_part"). Unique body parts belong to the "single" individual.In addition, this PR creates Tracks with the animal names (and "single" for the unique body parts), which is a new feature.
I added tests for importing Multiple animal DLC with uniquebodyparts.
Types of changes
Does this address any currently open issues?
#734
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
summary()
function, improving the readability of the code.test_maudlc
to ensure the correct parsing and organization of data from CSV files.animal_names
list with atracks
dictionary for better data management.