Merging #4758 into develop #4895
Conversation
Speed up evaluation with r-trees to find overlapping detections
WalkthroughThe pull request introduces significant enhancements to the evaluation logic for COCO and Open Images datasets in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (5)
tests/unittests/utils3d_tests.py (1)
393-401: LGTM: Good addition of unit test for_Boxclass.The new
BoxTestsclass provides a good starting point for testing the_Boxclass functionality. Thetest_box_verticesmethod correctly verifies that a default_Boxinstance generates the expected unit box vertices.To improve test coverage, consider adding the following test cases:
- A box with non-unit scale
- A box with non-zero location
- A box with non-zero rotation
- A combination of scale, location, and rotation
These additional tests would ensure that the
_Boxclass correctly handles various transformations.tests/unittests/evaluation_tests.py (3)
1887-1910: Enhance assertions intest_compute_max_iousWhile the test checks that the bounds are not None, it would be more robust to assert specific expected values or ranges for the IoU calculations.
Consider adding more specific assertions, such as:
self.assertGreaterEqual(bounds1[0], 0) self.assertLessEqual(bounds1[1], 1) self.assertGreaterEqual(bounds2[0], 0) self.assertLessEqual(bounds2[1], 1)This will ensure that the IoU values are within the expected range of [0, 1].
1911-1931: Enhancetest_find_duplicateswith more specific checksThe current test only checks the length of the returned duplicate IDs. To make the test more robust, consider adding the following improvements:
- Check that the returned IDs are actually present in the dataset.
- Verify that the duplicates have high IoU with each other.
- Test with different IoU thresholds to ensure the function behaves correctly.
Example additions:
# Check that returned IDs are in the dataset self.assertTrue(all(id in dataset.values("ground_truth.detections.id") for id in dup_ids1)) # Test with different IoU thresholds dup_ids_high = foui.find_duplicates(dataset, "ground_truth", iou_thresh=0.99, method="simple") self.assertEqual(len(dup_ids_high), 0) # Expect no duplicates with very high threshold dup_ids_low = foui.find_duplicates(dataset, "ground_truth", iou_thresh=0.1, method="simple") self.assertGreater(len(dup_ids_low), len(dup_ids1)) # Expect more duplicates with lower threshold
2024-2031: Improve error handling in_check_ioumethodThe modification to use
foui.compute_iousis a good improvement. However, the error handling and checking could be enhanced:
- Add a check to ensure that
iousis not empty before accessing its values.- Use
assertAlmostEqualfor floating-point comparisons instead ofassertTrue(np.isclose()).Consider refactoring the method as follows:
def _check_iou(self, dataset, field1, field2, expected_iou): dets1 = dataset.first()[field1].detections dets2 = dataset.first()[field2].detections ious = foui.compute_ious(dets1, dets2, sparse=True) self.assertNotEqual(len(ious), 0, "No IoUs were computed") result = next(iter(ious.values()), []) if expected_iou == 0: self.assertEqual(len(result), 0, "Expected no overlap, but found some") else: self.assertGreater(len(result), 0, "Expected overlap, but found none") _, actual_iou = result[0] self.assertAlmostEqual(actual_iou, expected_iou, places=7, msg=f"IoU {actual_iou} does not match expected {expected_iou}")This refactoring provides more informative error messages and uses more appropriate assertion methods.
fiftyone/utils/iou.py (1)
95-96: Avoid unnecessary dictionary comprehension when preds is emptyWhen
predsis empty, the comprehension{p.id: [] for p in preds}returns an empty dictionary. Sincepredscan be empty, the checkif preds else {}is redundant.You can simplify the return statement:
- return {p.id: [] for p in preds} if preds else {} + return {p.id: [] for p in preds}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- fiftyone/utils/eval/coco.py (1 hunks)
- fiftyone/utils/eval/openimages.py (1 hunks)
- fiftyone/utils/iou.py (11 hunks)
- fiftyone/utils/utils3d.py (1 hunks)
- setup.py (1 hunks)
- tests/unittests/evaluation_tests.py (2 hunks)
- tests/unittests/utils3d_tests.py (2 hunks)
🧰 Additional context used
🪛 Ruff
fiftyone/utils/iou.py
100-100: Do not assign a
lambdaexpression, use adefRewrite
iscrowdas adef(E731)
100-100: Ambiguous variable name:
l(E741)
570-573: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
582-585: Use ternary operator
gts = preds if is_symmetric else _polylines_to_detections(gts)instead ofif-else-blockReplace
if-else-block withgts = preds if is_symmetric else _polylines_to_detections(gts)(SIM108)
637-640: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
705-708: Use ternary operator
union = pred_area if gt_crowd else pred_area + gt_area - interinstead ofif-else-blockReplace
if-else-block withunion = pred_area if gt_crowd else pred_area + gt_area - inter(SIM108)
727-730: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
802-805: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
847-850: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
🔇 Additional comments (7)
setup.py (1)
63-63: Approved: Addition of "rtree" to INSTALL_REQUIRESThe addition of "rtree" to the INSTALL_REQUIRES list is correctly placed alphabetically. This change ensures that the "rtree" package will be installed along with FiftyOne.
However, I have a few points for clarification:
Could you please explain the necessity of adding "rtree" as a dependency? What new functionality in FiftyOne requires this package?
The PR objectives mention preserving backwards compatibility and setting a default value for 'sparse', but don't mention adding "rtree". Could you clarify how this addition relates to the stated objectives?
It would be helpful to update the PR description to include information about adding "rtree" as a new dependency, to ensure all changes are properly documented.
To verify the usage of "rtree" in the codebase, please run the following script:
This will help us understand where and how "rtree" is being used in the project.
✅ Verification successful
Verified: "rtree" is utilized in the codebase
The usage of "rtree" in
fiftyone/utils/iou.pyconfirms the necessity of adding it to theINSTALL_REQUIRESlist. This ensures that "rtree" will be properly installed with FiftyOne.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for "rtree" usage in Python files # Test: Search for "rtree" imports or usage rg --type python 'import\s+rtree|from\s+rtree\s+import|rtree\.'Length of output: 96
Script:
#!/bin/bash # Description: Search for "rtree" usage in Python files using correct file type # Test: Search for "rtree" imports or usage in .py files rg --type py 'import\s+rtree|from\s+rtree\s+import|rtree\.'Length of output: 109
tests/unittests/utils3d_tests.py (1)
13-13: LGTM: Appropriate import for numpy testing.The addition of
import numpy.testing as nptestis appropriate for the new test case that compares numpy arrays.tests/unittests/evaluation_tests.py (2)
1848-1886: New test class for bounding box operationsThe
BoxesTestsclass has been added to test bounding box-related operations. The_make_datasetmethod creates a small dataset with ground truth and prediction detections for testing purposes.
Line range hint
1848-2031: Overall improvement in bounding box and cuboid testingThe additions and modifications in this file enhance the test coverage for bounding box and cuboid operations in FiftyOne. The new
BoxesTestsclass introduces valuable tests for IoU computations and duplicate detection. The changes in theCuboidTestsclass simplify IoU calculations by leveraging utility functions.While these changes are positive, there are opportunities to further improve the robustness of the tests by:
- Adding more specific assertions in the
BoxesTestsclass.- Enhancing error handling and providing more informative error messages.
- Considering edge cases and different scenarios in the tests.
These improvements will help catch potential issues earlier and make the tests more maintainable in the long run.
fiftyone/utils/eval/coco.py (1)
530-532: Approved change to usesparse=Trueincompute_iousfor performance optimizationThe addition of
sparse=Trueto thefoui.compute_iousfunction call optimizes the computation of IoUs by utilizing a sparse matrix representation. This enhances performance when handling large numbers of detections without altering the existing matching logic or evaluation outcomes.fiftyone/utils/eval/openimages.py (1)
566-567:⚠️ Potential issueVerify the handling of IoU outputs with
sparse=True.Changing
compute_iousto usesparse=Truealters the format of the returnediousdata structure. Ensure that all parts of the code that usepred_iousare updated to correctly handle the new format.Run the following script to find usages of
pred_iousand verify they handle the sparse format:✅ Verification successful
IoU Output Handling with
sparse=TrueConfirmedAll usages of
pred_iousproperly handle the sparse format as per the reviewed code snippets.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `pred_ious` in the codebase to verify correct handling. # Test: Search for `pred_ious` variable usages. rg --type py -A 5 -B 5 'pred_ious' # Additionally, search for other calls to `compute_ious` to check if the `sparse` parameter is consistently used. rg --type py -A 2 -B 2 'compute_ious\('Length of output: 15444
Script:
#!/bin/bash # Description: Verify all accesses and manipulations of `pred_ious` to ensure they handle the sparse format correctly. # Search for indexing of `pred_ious` rg --type py 'pred_ious\[' # Search for iteration over `pred_ious` rg --type py 'for .* in pred_ious' # Search for method calls on `pred_ious` rg --type py 'pred_ious\.\w+'Length of output: 834
fiftyone/utils/utils3d.py (1)
170-170: LGTM!The computation of
self.verticeswith the updated line correctly applies the rotation and translation to each vertex.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
fiftyone/utils/iou.py (7)
90-91: Fix typo in docstringThere's a minor typo in the return description of the
compute_iousfunction.Apply this diff to fix the typo:
- a ``num_preds x num_gts`` array of IoUs when ``sparse=False`, or a dict + a ``num_preds x num_gts`` array of IoUs when ``sparse=False``, or a dict
175-177: Fix typo in docstringThere's a minor typo in the return description of the
compute_segment_iousfunction.Apply this diff to fix the typo:
- a ``num_preds x num_gts`` array of segment IoUs when ``sparse=False`, + a ``num_preds x num_gts`` array of segment IoUs when ``sparse=False``,
561-621: LGTM: Efficient IoU calculation with R-tree and sparse output supportThe updates to
_compute_bbox_iousfunction, including the use of an R-tree spatial index and support for sparse output, significantly improve the efficiency of IoU calculations. The implementation is correct and consistent with the changes made to other functions.Consider optimizing the sparse output case by avoiding the creation of zero-IoU entries:
if sparse: - ious[pred.id].append((gt.id, iou)) + if iou > 0: + ious[pred.id].append((gt.id, iou)) if is_symmetric: - ious[gt.id].append((pred.id, iou)) + if iou > 0: + ious[gt.id].append((pred.id, iou))This optimization would reduce memory usage for sparse outputs with many zero-IoU pairs.
🧰 Tools
🪛 Ruff
570-573: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
582-585: Use ternary operator
gts = preds if is_symmetric else _polylines_to_detections(gts)instead ofif-else-blockReplace
if-else-block withgts = preds if is_symmetric else _polylines_to_detections(gts)(SIM108)
Line range hint
624-719: LGTM: Efficient polygon IoU calculation with R-tree and sparse output supportThe updates to
_compute_polygon_iousfunction, including the use of an R-tree spatial index and support for sparse output, significantly improve the efficiency of polygon IoU calculations. The implementation is correct and consistent with the changes made to other functions.Consider applying the same optimization for the sparse output case as suggested for
_compute_bbox_ious:if sparse: - ious[pred.id].append((gt.id, iou)) + if iou > 0: + ious[pred.id].append((gt.id, iou)) if is_symmetric: - ious[gt.id].append((pred.id, iou)) + if iou > 0: + ious[gt.id].append((pred.id, iou))This optimization would reduce memory usage for sparse outputs with many zero-IoU pairs.
🧰 Tools
🪛 Ruff
705-708: Use ternary operator
union = pred_area if gt_crowd else pred_area + gt_area - interinstead ofif-else-blockReplace
if-else-block withunion = pred_area if gt_crowd else pred_area + gt_area - inter(SIM108)
727-730: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
724-753: LGTM: Polyline similarities with sparse output supportThe updates to
_compute_polyline_similaritiesfunction, including support for sparse output, are consistent with the changes made to other functions. The implementation is correct and maintains the overall structure of the codebase.Consider applying the same optimization for the sparse output case as suggested for previous functions:
if sparse: - sims[pred.id].append((gt.id, sim)) + if sim > 0: + sims[pred.id].append((gt.id, sim)) if is_symmetric: - sims[gt.id].append((pred.id, sim)) + if sim > 0: + sims[gt.id].append((pred.id, sim))This optimization would reduce memory usage for sparse outputs with many zero-similarity pairs.
🧰 Tools
🪛 Ruff
727-730: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
799-839: LGTM: Segment IoU computation with sparse output supportThe updates to
_compute_segment_iousfunction, including support for sparse output, are consistent with the changes made to other functions. The implementation is correct and maintains the overall structure of the codebase.Consider applying the same optimization for the sparse output case as suggested for previous functions:
if sparse: - ious[pred.id].append((gt.id, iou)) + if iou > 0: + ious[pred.id].append((gt.id, iou)) if is_symmetric: - ious[gt.id].append((pred.id, iou)) + if iou > 0: + ious[gt.id].append((pred.id, iou))This optimization would reduce memory usage for sparse outputs with many zero-IoU pairs.
🧰 Tools
🪛 Ruff
802-805: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
844-873: LGTM: Keypoint similarities with sparse output supportThe updates to
_compute_keypoint_similaritiesfunction, including support for sparse output, are consistent with the changes made to other functions. The implementation is correct and maintains the overall structure of the codebase.Consider applying the same optimization for the sparse output case as suggested for previous functions:
if sparse: - sims[pred.id].append((gt.id, sim)) + if sim > 0: + sims[pred.id].append((gt.id, sim)) if is_symmetric: - sims[gt.id].append((pred.id, sim)) + if sim > 0: + sims[gt.id].append((pred.id, sim))This optimization would reduce memory usage for sparse outputs with many zero-similarity pairs.
🧰 Tools
🪛 Ruff
847-850: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
tests/unittests/evaluation_tests.py (2)
1887-1909: Enhance IoU computation test assertionsWhile the test checks for non-None bounds, consider adding more specific assertions to verify the correctness of the IoU computation. For example, you could add assertions to check if the IoU values are within the expected range (0 to 1) or compare them against pre-calculated expected values for the given bounding boxes.
1911-1930: Enhance duplicate detection testConsider the following improvements to the
test_find_duplicatesmethod:
- Add assertions to check the specific duplicate IDs returned, not just the count.
- Include test cases with no duplicates and with multiple duplicates to ensure the function works correctly in various scenarios.
- Verify that the duplicates found are actually duplicates by comparing their bounding boxes or IoU values.
These enhancements will provide more comprehensive coverage of the
find_duplicatesfunction's behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- fiftyone/utils/iou.py (11 hunks)
- tests/unittests/evaluation_tests.py (2 hunks)
🧰 Additional context used
🪛 Ruff
fiftyone/utils/iou.py
100-100: Do not assign a
lambdaexpression, use adefRewrite
iscrowdas adef(E731)
100-100: Ambiguous variable name:
l(E741)
570-573: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
582-585: Use ternary operator
gts = preds if is_symmetric else _polylines_to_detections(gts)instead ofif-else-blockReplace
if-else-block withgts = preds if is_symmetric else _polylines_to_detections(gts)(SIM108)
637-640: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
705-708: Use ternary operator
union = pred_area if gt_crowd else pred_area + gt_area - interinstead ofif-else-blockReplace
if-else-block withunion = pred_area if gt_crowd else pred_area + gt_area - inter(SIM108)
727-730: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
802-805: Use ternary operator
ious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withious = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
847-850: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
🔇 Additional comments (7)
fiftyone/utils/iou.py (5)
38-38: LGTM: New sparse parameter implementationThe addition of the
sparseparameter and its implementation for handling empty input cases looks good. It provides a flexible way to return IoU results either as a full matrix or a sparse dictionary.Also applies to: 77-78, 94-97
105-109: LGTM: Consistent implementation of sparse parameterThe
sparseparameter has been correctly added to all relevant function calls withincompute_ious. This ensures consistent behavior across different types of IoU computations.Also applies to: 114-119, 122-127, 130-135, 150-150, 153-159
162-162: LGTM: Sparse parameter implementation in compute_segment_iousThe addition of the
sparseparameter to thecompute_segment_iousfunction and its implementation for handling empty input cases looks good. It maintains consistency with the changes made to other functions.Also applies to: 171-172, 180-183
530-550: LGTM: New helper functions for bounding box extractionThe new helper functions
_get_detection_boxand_get_poly_boxprovide a clean and efficient way to handle both 2D and 3D bounding boxes. The implementation looks good and should improve code readability and maintainability.Also applies to: 553-559
Line range hint
759-795: LGTM: Mask IoU computation with sparse parameterThe update to
_compute_mask_iousfunction to include thesparseparameter is correct and consistent with the changes made to other functions in the file. This maintains the overall structure and functionality of the codebase.🧰 Tools
🪛 Ruff
705-708: Use ternary operator
union = pred_area if gt_crowd else pred_area + gt_area - interinstead ofif-else-blockReplace
if-else-block withunion = pred_area if gt_crowd else pred_area + gt_area - inter(SIM108)
727-730: Use ternary operator
sims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))instead ofif-else-blockReplace
if-else-block withsims = defaultdict(list) if sparse else np.zeros((len(preds), len(gts)))(SIM108)
tests/unittests/evaluation_tests.py (2)
1849-1885: Well-structured dataset creation methodThe
_make_datasetmethod is well-implemented, creating a suitable dataset for testing with appropriate use of FiftyOne classes and structures.
Line range hint
1-2031: Overall assessment of changesThe additions and modifications to the
tests/unittests/evaluation_tests.pyfile enhance the test coverage for bounding box operations and IoU computations. The newBoxesTestsclass provides valuable tests for max IoU computation and duplicate detection. However, there are opportunities to improve the robustness and comprehensiveness of these tests:
- In
test_compute_max_ious, consider adding more specific assertions for IoU values.- In
test_find_duplicates, expand the test cases and verify the actual duplicate detections.- In the modified
_check_ioumethod ofCuboidTests, add error handling for potential edge cases in IoU retrieval.Implementing these suggestions will further strengthen the test suite and increase confidence in the correctness of the bounding box and IoU-related functionality.
Merges #4758 into develop after adding these changes to preserve backwards-compatibility (
sparse=Falseas default) and therefore ensuring that thecompute_max_ious()andfind_duplicates()methods continue to work.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores
rtreefor improved functionality.