Change match_empty_frames to empty_is_annotated #8888
Conversation
zhiltsov-max
requested review from
azhavoro,
bsekachev,
SpecLad and
Marishka17
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a comprehensive renaming of a property related to frame matching across multiple files in the CVAT (Computer Vision Annotation Tool) project. The change involves replacing Changes
Poem
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
🧹 Nitpick comments (2)
tests/python/shared/assets/cvat_db/data.json (1)
18176-18176: Consider adding test cases withempty_is_annotated: true.All current test configurations set
empty_is_annotatedtofalse. Given that the PR aims to enhance functionality for scenarios where either GT or DS annotations are empty, it would be beneficial to include test cases withempty_is_annotated: trueto ensure both behaviors are properly tested.Would you like me to help create additional test configurations with
empty_is_annotated: true?Also applies to: 18200-18200, 18224-18224, 18248-18248, 18272-18272, 18296-18296, 18320-18320, 18344-18344, 18368-18368, 18392-18392, 18416-18416, 18440-18440, 18464-18464, 18488-18488, 18512-18512, 18536-18536, 18560-18560, 18584-18584, 18608-18608, 18632-18632, 18656-18656, 18680-18680, 18704-18704, 18728-18728
cvat/schema.yml (1)
9778-9782: API schema changes look good but consider versioning strategy.The addition of the
empty_is_annotatedproperty to quality settings is well-documented and the implementation is consistent across schema definitions. The property serves a clear purpose in controlling how empty frames affect quality metrics.Since this is a breaking change to the API schema, consider:
- Implementing API versioning (e.g., using URL versioning like
/api/v1/vs/api/v2/)- Adding version compatibility notes in the schema description
- Documenting migration steps for API clients
Also applies to: 10285-10289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cvat-core/src/quality-settings.ts(4 hunks)cvat-core/src/server-response-types.ts(1 hunks)cvat-ui/src/components/quality-control/quality-control-page.tsx(1 hunks)cvat-ui/src/components/quality-control/task-quality/quality-settings-form.tsx(3 hunks)cvat/apps/quality_control/migrations/0006_rename_match_empty_frames_qualitysettings_empty_is_annotated.py(1 hunks)cvat/apps/quality_control/models.py(1 hunks)cvat/apps/quality_control/quality_reports.py(5 hunks)cvat/apps/quality_control/serializers.py(2 hunks)cvat/schema.yml(2 hunks)tests/python/rest_api/test_quality_control.py(2 hunks)tests/python/shared/assets/cvat_db/data.json(24 hunks)tests/python/shared/assets/quality_settings.json(24 hunks)
🔥 Files not summarized due to errors (1)
- tests/python/shared/assets/cvat_db/data.json: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (2)
- cvat/apps/quality_control/migrations/0006_rename_match_empty_frames_qualitysettings_empty_is_annotated.py
- tests/python/shared/assets/quality_settings.json
🔇 Additional comments (25)
tests/python/shared/assets/cvat_db/data.json (1)
18176-18176: LGTM! Property renamed consistently across test data.
The renaming from match_empty_frames to empty_is_annotated has been applied consistently across all test configurations, maintaining the same default value of false.
Also applies to: 18200-18200, 18224-18224, 18248-18248, 18272-18272, 18296-18296, 18320-18320, 18344-18344, 18368-18368, 18392-18392, 18416-18416, 18440-18440, 18464-18464, 18488-18488, 18512-18512, 18536-18536, 18560-18560, 18584-18584, 18608-18608, 18632-18632, 18656-18656, 18680-18680, 18704-18704, 18728-18728
cvat/apps/quality_control/models.py (1)
238-238: Great name change for clarity!
The new empty_is_annotated field better expresses its purpose compared to the old match_empty_frames.
cvat-core/src/quality-settings.ts (5)
41-41: Addition of the private member field
Declaring #emptyIsAnnotated is consistent with the refactor; it cleanly encapsulates the property within the class.
63-63: Constructor alignment
Initializing #emptyIsAnnotated from initialData.empty_is_annotated ensures the new field is fully integrated with the serialized data structure.
203-204: Getter for emptyIsAnnotated
The getter neatly exposes the property for external usage. Looks good and adheres to the new naming convention.
207-208: Setter for emptyIsAnnotated
The setter properly handles the boolean value. Make sure to test any UI or logic flows that might rely on the old setter name.
239-239: Ensure correct serialization
Replacing match_empty_frames with empty_is_annotated in toJSON() maintains consistency between data models and server representation.
cvat/apps/quality_control/serializers.py (3)
95-95: Renamed field in fields list
Properly lists empty_is_annotated. Confirm that no references to match_empty_frames remain.
103-103: Added default constraint
The default False matches the legacy behavior, safeguarding backward compatibility.
169-170: Updated help text
The revised description accurately reflects the new behavior regarding empty frames. This is crucial for user understanding.
cvat-core/src/server-response-types.ts (1)
261-261: Consistent interface rename
Renaming match_empty_frames to empty_is_annotated aligns with the updated data model across the codebase. Good job!
cvat-ui/src/components/quality-control/quality-control-page.tsx (1)
226-226: Validate all usages of the new property
You have correctly replaced settings.matchEmptyFrames with settings.emptyIsAnnotated. Please ensure that any existing calls or references using the old name throughout the codebase (especially if it was documented or used in external components) are fully aligned with the new naming.
cvat-ui/src/components/quality-control/task-quality/quality-settings-form.tsx (4)
37-37: Consistency of initial data
The new property emptyIsAnnotated is properly integrated into initialValues. Verify that legacy references (if any) to matchEmptyFrames are fully replaced and that the new property is tested for form submission.
84-84: Tooltip clarity
The updated tooltip for "Empty frames are annotated" clearly conveys the new property’s purpose. Great work on ensuring user-friendly messaging.
201-201: Form item name alignment
Renaming the form item to emptyIsAnnotated matches the new property name. This is consistent with the rest of the changes. Excellent.
206-206: Accurate label
The checkbox label "Empty frames are annotated" clearly communicates the effect of the new setting. This is an improvement over the old naming (“Match empty frames”).
tests/python/rest_api/test_quality_control.py (1)
1216-1216: Check param coverage in tests
The introduction of "empty_is_annotated" in the parameter list is consistent with the updated naming. Ensure that you have sufficient test coverage for scenarios when empty_is_annotated is turned on/off.
[approve]
cvat/apps/quality_control/quality_reports.py (8)
218-218: Renaming field reflects the new meaning
Introducing empty_is_annotated: bool = False aligns with the overall refactor. Make sure any references or related logic for the old match_empty_frames field is removed or migrated.
220-222: Documentation clarity
These lines explain precisely how empty frames are interpreted if empty_is_annotated is enabled. The updated docstring is very clear. Good job!
1981-1990: Metric adjustment for simultaneously empty frames
Handling cases where both GT and DS frames have no annotations is critical for accurate metrics. The approach of setting valid counts when both are empty is correct. Confirm that the final reporting logic handles small edge cases (e.g., tasks with no annotations at all).
1992-1996: Validate partial emptiness
When either GT or DS is empty (but not both), these lines ensure the DS or GT shape counts are marked as 1. Verify that no additional side effects are introduced, such as artificially inflating counts where partial data is present in only one side.
2089-2100: Empty frames in confusion matrix
Here, you handle a scenario in which summary.total_count is set to 1 when a frame is entirely empty. This is consistent with the docstring expectations for empty_is_annotated. The logic looks good!
2124-2125: Edge case references
These lines track empty GT or DS frames separately. Ensure that related logic (like partial updates in a multi-job scenario) doesn’t incorrectly classify frames.
2131-2143: Confusion matrix emptiness check
The code properly checks whether the confusion matrix indicates any annotations. This is critical when adjusting metrics. Great attention to detail with these checks.
2156-2162: Accumulate valid counts for empty frames
The logic of adding a valid count for frames that are empty on both sides is well-implemented. This ensures that metrics reflect successful “matches” when no annotations exist.
| if parameter == "empty_is_annotated": | ||
| assert new_report["summary"]["valid_count"] != old_report["summary"]["valid_count"] | ||
| assert new_report["summary"]["total_count`"] != old_report["summary"]["total_count`"] | ||
| assert new_report["summary"]["ds_count"] != old_report["summary"]["ds_count"] | ||
| assert new_report["summary"]["gt_count"] != old_report["summary"]["gt_count"] |
There was a problem hiding this comment.
Potential typo in the code snippet
The lines refer to "total_count\"with a backtick, which might be unintentional. Confirm whether the backtick in"total_count`"` is a typo or intended. If it is a typo, please correct it to ensure clarity and avoid runtime errors.
- assert new_report["summary"]["total_count`"] != old_report["summary"]["total_count`"]
+ assert new_report["summary"]["total_count"] != old_report["summary"]["total_count"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if parameter == "empty_is_annotated": | |
| assert new_report["summary"]["valid_count"] != old_report["summary"]["valid_count"] | |
| assert new_report["summary"]["total_count`"] != old_report["summary"]["total_count`"] | |
| assert new_report["summary"]["ds_count"] != old_report["summary"]["ds_count"] | |
| assert new_report["summary"]["gt_count"] != old_report["summary"]["gt_count"] | |
| if parameter == "empty_is_annotated": | |
| assert new_report["summary"]["valid_count"] != old_report["summary"]["valid_count"] | |
| assert new_report["summary"]["total_count"] != old_report["summary"]["total_count"] | |
| assert new_report["summary"]["ds_count"] != old_report["summary"]["ds_count"] | |
| assert new_report["summary"]["gt_count"] != old_report["summary"]["gt_count"] |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8888 +/- ##
===========================================
- Coverage 73.89% 73.89% -0.01%
===========================================
Files 408 411 +3
Lines 44131 44183 +52
Branches 3986 3993 +7
===========================================
+ Hits 32611 32649 +38
- Misses 11520 11534 +14
|
Motivation and context
Improves the implementation of the
match_empty_framesquality setting to work in cases when only GT or DS annotations are empty. This allows to use precision as the primary metric in cases when all frames are required to have at least 1 annotation.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
Refactor
match_empty_framestoempty_is_annotatedacross multiple components and filesDocumentation
Tests