Fix quality computation for tasks with skeletons and normal labels #8100
Conversation
|
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 WalkthroughThis update enhances the quality computation in the Computer Vision Annotation Tool (CVAT) by addressing how tasks with skeleton and normal labels are managed. A new parameter, 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
zhiltsov-max
changed the title
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
cvat/apps/quality_control/quality_reports.py (1)
[!TIP]
Codebase VerificationMissing
label_id_mapperParameter in_make_zero_confusion_matrixCallsThe
_make_zero_confusion_matrixmethod requires alabel_id_mapperparameter for correct functionality. However, the following instances do not include this parameter:
cvat/apps/quality_control/quality_reports.py, line 1973cvat/apps/quality_control/quality_reports.py, line 1995Please ensure that the
label_id_mapperparameter is provided in these method calls.Analysis chain
Line range hint
1933-1992: Refactor_make_zero_confusion_matrixmethod to includelabel_id_mapperThe changes to the
_make_zero_confusion_matrixmethod include the addition of alabel_id_mapperparameter and its usage in the method. This modification is essential for correctly mapping label IDs to indices in the confusion matrix, which is crucial for handling tasks with both skeleton and non-skeleton labels correctly.
- Ensure that the
label_id_mapperis always provided when this method is called.- Verify that the
label_id_mapperfunction is thoroughly tested, especially its ability to handle unexpected or edge case inputs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `label_id_mapper` is provided everywhere `_make_zero_confusion_matrix` is called. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'_make_zero_confusion_matrix'Length of output: 77
Script:
#!/bin/bash # Description: Verify that `label_id_mapper` is provided everywhere `_make_zero_confusion_matrix` is called. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type py '_make_zero_confusion_matrix'Length of output: 389
Script:
#!/bin/bash # Description: Display the surrounding lines of the calls to `_make_zero_confusion_matrix` to verify if `label_id_mapper` is included. # Show surrounding lines for the first call rg --type py '_make_zero_confusion_matrix()' -A 3 -B 3 # Show surrounding lines for the second call rg --type py '_make_zero_confusion_matrix()' -A 3 -B 3Length of output: 3853
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- changelog.d/20240628_183747_mzhiltso_fix_quality_for_skeletons.md (1 hunks)
- cvat/apps/quality_control/quality_reports.py (3 hunks)
- tests/python/rest_api/test_quality_control.py (2 hunks)
Files skipped from review due to trivial changes (1)
- changelog.d/20240628_183747_mzhiltso_fix_quality_for_skeletons.md
Additional comments not posted (2)
tests/python/rest_api/test_quality_control.py (1)
1248-1281: Ensure proper cleanup and isolation in tests.This test modifies task labels and annotations which could affect other tests if not properly isolated or cleaned up. Consider using fixtures for setup and teardown to ensure test isolation.
Verification successful
Proper cleanup and isolation verified.
The
restore_db_per_classfixture is used in theautousefixtures, ensuring that the database state is reset between tests. This provides the necessary cleanup and isolation for the tests.
restore_db_per_classfixture is defined intests/python/shared/fixtures/init.pyand restores the database state usingdocker_restore_dborkube_restore_db.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of fixtures that ensure proper cleanup and isolation. # Search for pytest fixtures related to database restoration or cleanup. rg --type python '@pytest.fixture' 'tests/python/rest_api/test_quality_control.py'Length of output: 111
Script:
#!/bin/bash # Description: Check for usage of fixtures that ensure proper cleanup and isolation. # Search for pytest fixtures related to database restoration or cleanup. rg --type py '@pytest.fixture' 'tests/python/rest_api/test_quality_control.py'Length of output: 178
Script:
#!/bin/bash # Description: Extract definitions of pytest fixtures to check for proper cleanup and isolation. # Search for the definitions of pytest fixtures in the test file. rg --type py '@pytest.fixture' -A 10 'tests/python/rest_api/test_quality_control.py'Length of output: 1447
Script:
#!/bin/bash # Description: Extract the definition of the `restore_db_per_class` fixture to verify its role in test isolation. # Search for the definition of the `restore_db_per_class` fixture. rg --type py 'def restore_db_per_class' -A 10Length of output: 757
cvat/apps/quality_control/quality_reports.py (1)
1943-1944: Use oflabel_id_mapperin_generate_frame_annotation_conflictsmethodThe
label_id_mapperis correctly utilized to map label IDs to their corresponding indices in the confusion matrix. This is a crucial aspect of ensuring that labels are correctly accounted for in the confusion matrix, particularly when handling tasks with a mix of skeleton and non-skeleton labels.
- Double-check that the
label_id_mapperhandles all possible label IDs correctly, including any that might not be directly mapped due to errors or omissions in the dataset.Verification successful
Use of
label_id_mapperin_generate_frame_annotation_conflictsmethodThe
label_id_mapperis correctly utilized to map label IDs to their corresponding indices in the confusion matrix. This is a crucial aspect of ensuring that labels are correctly accounted for in the confusion matrix, particularly when handling tasks with a mix of skeleton and non-skeleton labels.
- The
label_id_mapperis defined aslabel_id_idx_map.__getitem__, which ensures proper mapping of label IDs to their indices.- The usage of
label_id_mapperin the_generate_frame_annotation_conflictsmethod appears to handle bothds_ann.labelandgt_ann.labelcorrectly, defaulting to_UNMATCHED_IDXwhen necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all label IDs are correctly handled by `label_id_mapper`. # Test: Search for the function usage and verify proper mapping. rg --type python $'label_id_mapper'Length of output: 65
Script:
#!/bin/bash # Description: Verify that all label IDs are correctly handled by `label_id_mapper`. # Test: Search for the function usage and verify proper mapping. rg --type py $'label_id_mapper'Length of output: 627
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8100 +/- ##
========================================
Coverage 83.63% 83.64%
========================================
Files 383 383
Lines 40503 40507 +4
Branches 3831 3830 -1
========================================
+ Hits 33876 33881 +5
+ Misses 6627 6626 -1
|
zhiltsov-max
requested review from
SpecLad
and removed request for
nmanovic,
azhavoro and
Marishka17
changelog.d/20240628_183747_mzhiltso_fix_quality_for_skeletons.md
Outdated
Show resolved
Hide resolved
|
|
||
| max_label_id = label_id | ||
|
|
||
| label_id_idx_map[max_label_id + 1] = len(label_names) |
There was a problem hiding this comment.
Why is this needed? It shouldn't be possible for the client(s) of this function to index the map with this key.
There was a problem hiding this comment.
This is needed to make the matrix size and label names list the same size, mostly to avoid confusion.
There was a problem hiding this comment.
It's still confusing, because max_label_id + 1 can be a valid label ID (if there's a sublabel following the last non-sublabel). IMO, this map entry is unnecessary and you shouldn't add it.
|
Motivation and context
How has this been tested?
Unit tests
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
Bug Fixes
Tests