Fixing import and export issues with invalid tracks #8553
Conversation
WalkthroughThe changes encompass modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnnotationIR
participant TrackManager
User->>AnnotationIR: Request to slice track
AnnotationIR->>AnnotationIR: Validate shapes
AnnotationIR->>TrackManager: Request interpolated shapes
TrackManager->>TrackManager: Check for duplicates
TrackManager-->>AnnotationIR: Return valid shapes
AnnotationIR-->>User: Return sliced track
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
dev/format_python_code.sh (1)
33-33: LGTM! Consider grouping related paths for better maintainability.The addition of "cvat/apps/dataset_manager/tests/test_annotation.py" to the list of paths for formatting is appropriate and aligns with the goal of maintaining consistent code style across the project.
For improved maintainability, consider grouping related paths together. For example, you could move this new test file path next to other test-related paths or create a new group for dataset manager related files. This would make it easier to manage and update the list of paths in the future.
cvat/apps/dataset_manager/tests/test_annotation.py (2)
329-373: LGTM: New test case for handling duplicated shapes.The new
test_duplicated_shape_interpolationmethod is a valuable addition to the test suite. It verifies that theTrackManager.get_interpolated_shapesmethod correctly handles tracks with duplicated shapes, which is an important edge case to consider.Suggestion for improvement:
Consider adding an assertion to verify that theoutsideproperty is correctly maintained in the interpolated shapes. This would ensure that the interpolation not only removes duplicates but also preserves the correct state of each shape.You could add the following assertion after line 373:
self.assertEqual([shape['outside'] for shape in expected_shapes], [shape['outside'] for shape in interpolated_shapes])This will verify that the
outsideproperty is correctly maintained in the interpolated shapes.
376-425: LGTM: New test class for AnnotationIR slicing functionality.The new
AnnotationIRTestclass and its test methodtest_slice_track_does_not_duplicate_outside_frame_on_the_endare valuable additions to the test suite. They verify important behavior of theAnnotationIR.slicemethod, ensuring that it correctly handles tracks with outside frames at the end.Suggestion for improvement:
Consider adding more test cases to cover different scenarios, such as:
- Slicing a track with multiple outside frames in the middle.
- Slicing a track where the slice end is beyond the last frame of the track.
You could add these additional test methods to provide more comprehensive coverage:
def test_slice_track_with_multiple_outside_frames(self): # Add test case for slicing a track with multiple outside frames in the middle def test_slice_track_beyond_last_frame(self): # Add test case for slicing a track where the slice end is beyond the last frame
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- cvat/apps/dataset_manager/annotation.py (2 hunks)
- cvat/apps/dataset_manager/tests/test_annotation.py (20 hunks)
- dev/format_python_code.sh (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
cvat/apps/dataset_manager/tests/test_annotation.py (3)
7-8: LGTM: New imports added correctly.The new imports for
AnnotationIRandDimensionTypeare correctly added and necessary for the new test classAnnotationIRTest.
13-13: LGTM: Consistent string delimiter usage and improved dictionary formatting.The changes improve code consistency by:
- Using double quotes for string literals consistently across all test methods.
- Adding trailing commas to the
attributesfield in shape dictionaries, which improves readability and makes future additions easier.These changes align with Python best practices and improve overall code quality.
Also applies to: 28-28, 45-45, 53-53, 61-61, 82-82, 90-90, 98-98, 120-120, 129-129, 138-138, 160-160, 169-169, 178-178, 200-200, 218-219
Line range hint
1-425: Overall assessment: Improved test coverage and code consistency.The changes in this file significantly enhance the test suite for the
TrackManagerandAnnotationIRclasses. Key improvements include:
- Consistent use of string delimiters and improved dictionary formatting throughout the file.
- New test case for handling duplicated shapes in track interpolation.
- New test class for verifying the slicing functionality of
AnnotationIR.These changes will help ensure the robustness of the annotation handling code, particularly in edge cases involving duplicated shapes and track slicing. The improved consistency also enhances code readability and maintainability.
To further strengthen the test suite, consider implementing the suggested additional test cases for
AnnotationIRslicing and the extra assertion for the duplicated shape test.cvat/apps/dataset_manager/annotation.py (1)
134-135: Logic for appending shapes is correctThe condition and the append operation appropriately ensure that the last shape from
scoped_shapesis added tosegment_shapeswhen necessary. This maintains the correct sequence of shapes based on frame numbers.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8553 +/- ##
===========================================
+ Coverage 74.25% 74.27% +0.01%
===========================================
Files 403 403
Lines 43313 43315 +2
Branches 3914 3914
===========================================
+ Hits 32163 32173 +10
+ Misses 11150 11142 -8
|
|
Hwllo, @Eldies |
|
@PMazarovich Thank you for the backup! I checked it with this PR and it seems the PR fixes the problem. |
| if curr_frame == prev_shape["frame"]: | ||
| if dict(shape, id=None, keyframe=None) == dict(prev_shape, id=None, keyframe=None): |
There was a problem hiding this comment.
These 2 if statements can be combined into one.
Why do we need to exclude keyframe when comparing shapes?
There was a problem hiding this comment.
They can be combined, but then this line becomes too long (137 cahracters) and should be split to 2 lines anyway, and I think it is more readable the way it is now.
keyframe should be excluded, because it is always True in prev_shape due to shape["keyframe"] = True on line 967.
There was a problem hiding this comment.
They can be combined, but then this line becomes too long (137 cahracters) and should be split to 2 lines anyway, and I think it is more readable the way it is now.
if (
condition1
and condition2
):
...
| @@ -0,0 +1,4 @@ | |||
| ### Fixed | |||
|
|
|||
| - Track shape duplication on import | |||
There was a problem hiding this comment.
Now there should be no errors during export tracks with duplicated shapes. Why not mention that?
|
@Eldies, Could you please resolve conflicts? |
# Conflicts: # dev/format_python_code.sh
resolved |
Eldies
force-pushed
the
dl/fix-invalid-tracks
branch
from
5aa4e42 to
2ec6756
Compare
|
Motivation and context
Sometimes tracks have duplicated shapes and export/import fails here:
cvat/cvat/apps/dataset_manager/annotation.py
Line 953 in 49ec1d1
it can be reproduced with following task backup (export fails):
InvalidTracksTaskBackup.zip
Also it can be reproduced with following CVAT 1.1 annotations (import to the task above fails)
InvalidImportAnnotations.zip
PR fixes #8411
And (probably) fixes the following issues:
#4968
#5638
#6779
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
TrackManagerandAnnotationIRclasses, including new test methods and classes.Chores