Main -> develop #5435
Main -> develop #5435
Conversation
Cherry pick builtin use lists
Add support for custom evaluation metrics
* fix test and make safer * wrap in try finally * use oid from var
Add Model Evaluation panel callbacks for segmentation tasks
Update release-notes for v1.3.0
Release v1.3.0
WalkthroughThis pull request introduces comprehensive enhancements to FiftyOne's evaluation and metrics infrastructure across multiple modules. The changes focus on adding support for custom metrics, improving evaluation configurations, and refactoring sample retrieval methods in core dataset and view classes. Key modifications span evaluation utilities, dataset core classes, and various utility modules, with a particular emphasis on providing more flexible and extensible evaluation capabilities. Changes
Possibly related PRs
Suggested labels
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
fiftyone/utils/eval/detection.py (1)
Line range hint
290-486: Dynamic field usage cautionWhen registering samples and cleaning up in
DetectionEvaluation, multiple dynamic fields (eval_key,tp_field,fn_field, etc.) are added or removed. If your dataset is concurrently accessed in multi-threaded scenarios or by multiple processes, you might encounter race conditions on field creation or deletion. Consider sync mechanisms or appropriate documentation regarding concurrency constraints.
🧹 Nitpick comments (28)
fiftyone/core/dataset.py (2)
Line range hint
1318-1331: Consider using f-string for error message.The implementation is good, but the error message could be more readable using an f-string.
- "Expected one matching sample, but found %d matches" - % len(view) + f"Expected one matching sample, but found {len(view)} matches"
1238-1240: Preserve exception context in error handling.The bare raise in the except clause loses the original exception context. Consider preserving it for better debugging.
- except ValueError: - raise ValueError("%s is empty" % self.__class__.__name__) + except ValueError as err: + raise ValueError("%s is empty" % self.__class__.__name__) from err🧰 Tools
🪛 Ruff (0.8.2)
1239-1239: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
fiftyone/core/collections.py (2)
926-929: Improve exception handling infirst()Consider using
raise ... from Noneto explicitly indicate that this is a new exception rather than a re-raised one.- raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
929-929: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1013-1031: Improve exception handling inone()Consider using
raise ... from Noneto explicitly indicate that these are new exceptions rather than re-raised ones.- raise ValueError("No samples match the given expression") + raise ValueError("No samples match the given expression") from None - raise ValueError( + raise ValueError( "Expected one matching sample, but found %d matches" - % len(view) - ) + % len(view) + ) from None🧰 Tools
🪛 Ruff (0.8.2)
1019-1019: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
plugins/panels/model_evaluation/__init__.py (4)
17-17: Remove the unused import.
Static analysis indicatesfiftyone.core.fieldsis not used. Eliminating unused imports keeps dependencies clear.-import fiftyone.core.fields as fof🧰 Tools
🪛 Ruff (0.8.2)
17-17:
fiftyone.core.fieldsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
151-156: Consider logging exceptions inget_custom_metrics.
It silently returnsNonewhen exceptions occur, which could hamper debugging. Logging the exception would improve visibility.+import logging def get_custom_metrics(self, results): try: return results.custom_metrics except Exception as e: + logging.warning(f"Error retrieving custom metrics: {e}") return None
604-675: Expand documentation for the segmentation evaluation flow.
This large block supports various segmentations use-cases. Adding inline docstrings or comments would aid readability when handling differentview_typepaths.
698-707: Refine the naming of_get_mask_targets.
The logic for retrieving mask targets is sound, but consider using a name like_retrieve_mask_targetsto better convey intent.fiftyone/operators/evaluation_metric.py (2)
12-43: Clarify parameter docstring and usageIn
EvaluationMetricConfig, consider explicitly documenting howeval_typesis intended to be used. The parameter references possible evaluation types, but no further usage is described in this file. This additional clarity will help developers understand when and how to supply these types.
45-80: Keep method stubs well-documentedIn the
EvaluationMetricbase class,get_parameters,parse_parameters, andcomputeare placeholders that raiseNotImplementedErroror provide no functionality. To improve clarity for future maintainers, consider adding docstring notes or comments explaining typical usage and what subclasses should implement.fiftyone/utils/eval/regression.py (5)
Line range hint
43-115: Enhance validation ofcustom_metricsparameterWhen adding
custom_metricstoevaluate_regressions, ensure you handle type-checking and potential edge cases (e.g., strings vs. lists vs. dictionaries). This will help avoid runtime errors if callers provide invalid inputs.
121-137: External usage clarity
RegressionEvaluationConfignow supportscustom_metrics. Document more explicitly how users can pass in custom metrics (list vs. dict) and give examples. This fosters a smoother onboarding for new contributors.
254-268: Suggest explicit example forSimpleEvaluationConfigThe docstring references custom metrics but does not illustrate usage. Provide a short code snippet demonstrating how to specify and access custom metrics to improve developer clarity.
Line range hint
381-421: Assess performance for large-scale usage
RegressionResultsstores arrays (ytrue,ypred, etc.) in memory. For very large datasets, memory consumption may spike. Consider providing documentation or checks for extremely large data, especially given the introduction of multiple custom metrics that may perform additional computations.
587-590: Adopt the static analysis suggestion forcustom_metricsAt lines 587 and 641 in
_parse_config, replacekwargs.get("custom_metrics", None)withkwargs.get("custom_metrics")as suggested by static analysis. This minor improvement clarifies the code and removes the redundantNoneargument.- custom_metrics = kwargs.get("custom_metrics", None) + custom_metrics = kwargs.get("custom_metrics")Also applies to: 641-644
🧰 Tools
🪛 Ruff (0.8.2)
587-587: Use
kwargs.get("custom_metrics")instead ofkwargs.get("custom_metrics", None)Replace
kwargs.get("custom_metrics", None)withkwargs.get("custom_metrics")(SIM910)
fiftyone/utils/eval/detection.py (3)
Line range hint
139-173: Validate custom metrics input typeIn
evaluate_detections(),custom_metricscan be a list of metrics or a dict. To improve reliability, consider adding a brief check or docstring examples showing acceptable formats. This fosters better usage across the detection evaluation ecosystem.
Line range hint
239-272: Improve coverage for new config parameters
DetectionEvaluationConfignow includescustom_metrics. Make sure your test suite validates scenarios where custom metrics are defined, used, renamed, or cleaned up, so that potential schema or logic mismatches are quickly detected.Would you like a suggested test snippet covering multiple custom metrics with different parameter types?
531-532: Ensure thorough removal of custom metric fields
cleanup_custom_metrics()is invoked at line 531. If external code references these fields later, or the stored data is being used for a second pass, you might trigger exceptions. Add a cautionary note in the docstring that any dependent processes on these fields must be completed first.fiftyone/utils/eval/base.py (3)
85-85: Usekey in dictinstead ofkey in dict.keys()Per the static analysis hints (SIM118), it is more concise to iterate directly on the dictionary rather than its
.keys()method.- for metric in self._get_custom_metrics().keys(): + for metric in self._get_custom_metrics():Also applies to: 101-101, 113-113
🧰 Tools
🪛 Ruff (0.8.2)
85-85: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
179-179: Rename unused loop control variableThe variable
uriis not referenced in the loop body; renaming it to_or_uriclarifies that it is unused.- for uri, metric in self.custom_metrics.items(): + for _uri, metric in self.custom_metrics.items():🧰 Tools
🪛 Ruff (0.8.2)
179-179: Loop control variable
urinot used within loop bodyRename unused
urito_uri(B007)
57-75: Guard against operator misconfigurationWhile already handled by
try/except, consider logging more context (e.g., operator config) or re-raising with a custom exception when an operator is missing required parameters, to aid debugging.fiftyone/utils/eval/segmentation.py (3)
602-605: Usekwargs.get("custom_metrics")As per the static analysis hint (SIM910), replace
kwargs.get("custom_metrics", None)withkwargs.get("custom_metrics")for cleaner code.- custom_metrics = kwargs.get("custom_metrics", None) + custom_metrics = kwargs.get("custom_metrics")🧰 Tools
🪛 Ruff (0.8.2)
602-602: Use
kwargs.get("custom_metrics")instead ofkwargs.get("custom_metrics", None)Replace
kwargs.get("custom_metrics", None)withkwargs.get("custom_metrics")(SIM910)
389-389: DocumentmatchesusageThe
matcheslist captures pixel-level matches for each segmentation pair. Consider documenting its definition more formally (e.g., in method docstrings) for future maintainers.
432-442: Potential memory usage concernsAppending every
(gt_label, pred_label, pixel_count, gt_id, pred_id)could be memory-intensive for large masks. If performance is a concern, consider optional sampling or summary stats to reduce memory overhead.fiftyone/utils/eval/classification.py (1)
926-926: Usekwargs.get("custom_metrics")Per the static analysis hint (SIM910), we can simplify code by omitting the default argument when calling
.get().- custom_metrics = kwargs.get("custom_metrics", None) + custom_metrics = kwargs.get("custom_metrics")🧰 Tools
🪛 Ruff (0.8.2)
926-926: Use
kwargs.get("custom_metrics")instead ofkwargs.get("custom_metrics", None)Replace
kwargs.get("custom_metrics", None)withkwargs.get("custom_metrics")(SIM910)
fiftyone/operators/__init__.py (1)
15-15: Add new imports to__all__.The imported
EvaluationMetricConfigandEvaluationMetricclasses should be added to__all__to make them available at the package level.from .evaluation_metric import EvaluationMetricConfig, EvaluationMetric +__all__ = [ + "EvaluationMetricConfig", + "EvaluationMetric", + # ... existing entries ... +]🧰 Tools
🪛 Ruff (0.8.2)
15-15:
.evaluation_metric.EvaluationMetricConfigimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
15-15:
.evaluation_metric.EvaluationMetricimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
fiftyone/core/fields.py (1)
1624-1684: LGTM! Well-implemented color conversion utilities.The color conversion functions are well-documented and efficiently implemented. Consider a minor optimization for
rgb_array_to_int:- return ( - np.left_shift(mask[:, :, 0], 16, dtype=int) - + np.left_shift(mask[:, :, 1], 8, dtype=int) - + mask[:, :, 2] - ) + return np.left_shift(mask[:, :, 0], 16) | np.left_shift(mask[:, :, 1], 8) | mask[:, :, 2]Using bitwise OR operations can be more efficient than addition for combining bit-shifted values.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1782-1828: Consider these improvements for better type safety and performance.
- The
CustomMetricstype could be more specific about the key type:- [operatorUri: string]: CustomMetric; + [operatorUri: `${string}/${string}`]: CustomMetric;
- Add input validation to handle undefined/null inputs:
function formatCustomMetricRows(evaluationMetrics, comparisonMetrics) { + if (!evaluationMetrics) return []; const results = [] as SummaryRow[];
- Consider memoizing the function to prevent unnecessary recalculations:
-function formatCustomMetricRows(evaluationMetrics, comparisonMetrics) { +const formatCustomMetricRows = React.useMemo(() => (evaluationMetrics, comparisonMetrics) => { // ... existing implementation -} +}, []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(3 hunks)docs/source/release-notes.rst(1 hunks)fiftyone/__public__.py(1 hunks)fiftyone/core/collections.py(5 hunks)fiftyone/core/dataset.py(6 hunks)fiftyone/core/fields.py(1 hunks)fiftyone/core/view.py(0 hunks)fiftyone/operators/__init__.py(1 hunks)fiftyone/operators/evaluation_metric.py(1 hunks)fiftyone/operators/registry.py(4 hunks)fiftyone/server/utils.py(1 hunks)fiftyone/utils/eval/activitynet.py(5 hunks)fiftyone/utils/eval/base.py(6 hunks)fiftyone/utils/eval/classification.py(20 hunks)fiftyone/utils/eval/coco.py(5 hunks)fiftyone/utils/eval/detection.py(16 hunks)fiftyone/utils/eval/openimages.py(5 hunks)fiftyone/utils/eval/regression.py(18 hunks)fiftyone/utils/eval/segmentation.py(23 hunks)plugins/panels/model_evaluation/__init__.py(6 hunks)tests/unittests/dataset_tests.py(0 hunks)tests/unittests/operators/registry_tests.py(2 hunks)tests/unittests/utils_tests.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/unittests/dataset_tests.py
- fiftyone/core/view.py
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🪛 Ruff (0.8.2)
fiftyone/__public__.py
166-166: .core.sample.Sample imported but unused
Remove unused import: .core.sample.Sample
(F401)
fiftyone/operators/__init__.py
15-15: .evaluation_metric.EvaluationMetricConfig imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
15-15: .evaluation_metric.EvaluationMetric imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
fiftyone/utils/eval/detection.py
641-641: Use kwargs.get("custom_metrics") instead of kwargs.get("custom_metrics", None)
Replace kwargs.get("custom_metrics", None) with kwargs.get("custom_metrics")
(SIM910)
plugins/panels/model_evaluation/__init__.py
17-17: fiftyone.core.fields imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
fiftyone/utils/eval/segmentation.py
602-602: Use kwargs.get("custom_metrics") instead of kwargs.get("custom_metrics", None)
Replace kwargs.get("custom_metrics", None) with kwargs.get("custom_metrics")
(SIM910)
fiftyone/utils/eval/regression.py
587-587: Use kwargs.get("custom_metrics") instead of kwargs.get("custom_metrics", None)
Replace kwargs.get("custom_metrics", None) with kwargs.get("custom_metrics")
(SIM910)
fiftyone/utils/eval/base.py
85-85: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
101-101: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
113-113: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
179-179: Loop control variable uri not used within loop body
Rename unused uri to _uri
(B007)
fiftyone/utils/eval/classification.py
926-926: Use kwargs.get("custom_metrics") instead of kwargs.get("custom_metrics", None)
Replace kwargs.get("custom_metrics", None) with kwargs.get("custom_metrics")
(SIM910)
fiftyone/core/dataset.py
1239-1239: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
fiftyone/core/collections.py
929-929: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1019-1019: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (44)
fiftyone/__public__.py (1)
166-166: Verify ifSampleimport can be safely removed.The static analysis tool flags
Sampleas an unused import. However, since this is a public interface file and there are significant changes to sample handling, we should verify if this import is required for backward compatibility.Let's verify the usage of
Samplein the codebase:🧰 Tools
🪛 Ruff (0.8.2)
166-166:
.core.sample.Sampleimported but unusedRemove unused import:
.core.sample.Sample(F401)
fiftyone/core/dataset.py (4)
1228-1228: LGTM! Good refactoring.The change simplifies the code by delegating to the parent class implementation, reducing duplication while maintaining functionality.
1237-1241: LGTM! Clean implementation.The change improves readability by using Python's slice syntax while maintaining proper error handling for empty datasets.
🧰 Tools
🪛 Ruff (0.8.2)
1239-1239: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1255-1258: LGTM! Nice Pythonic implementation.The change improves both readability and performance by using list comprehension and array slicing.
1272-1275: LGTM! Clean and efficient implementation.The change improves both readability and performance by using list comprehension and negative array slicing.
fiftyone/core/collections.py (1)
Line range hint
926-1031: LGTM! Clean implementation of collection access methodsThe implementations of
first(),last(),head(),tail()andone()are clean and efficient:
first()/last()use iterator/slice accesshead()/tail()use simple list comprehensionsone()provides good error handling for common cases- All methods have consistent error handling
plugins/panels/model_evaluation/__init__.py (8)
13-13: No concerns about this import usage.
TheObjectIdimport is utilized in_to_object_ids, which is appropriate.
351-359: Initialize segmentation results cautiously.
The call to_init_segmentation_resultsis fine. Ensure no redundant re-initialization is performed when working with segmentation data.
382-382: Newcustom_metricskey aligns with the introduced method.
Adding"custom_metrics": self.get_custom_metrics(results)consistently populates evaluation data with user-defined metrics.
709-771: Safeguard against potentialKeyErrorin_init_segmentation_results.
If a label appears inresults.ytrueorresults.ypredbut not inresults.classes_map, aKeyErrormay occur. Consider adding checks for missing keys.
772-789: Check for nonexistent class keys in_get_segmentation_class_ids.
Use caution when accessingresults._classes_map[x]. Missing entries could triggerKeyError.
791-797: Ensure class mapping exists in_get_segmentation_conf_mat_ids.
Ifxoryis missing fromresults._classes_map, an exception may occur. Handling unexpected keys can make the function more robust.
799-816: TP/FP/FN logic appears well-structured.
The conditions for true positives, false negatives, and false positives align with the established segmentation approach.
818-819: Conversion of IDs to ObjectId is straightforward and correct.
UsingObjectIdfor database referencing is consistent with the rest of the system.fiftyone/operators/evaluation_metric.py (3)
1-7: Docstring consistency checkThe file header and module docstring appear consistent with the rest of the codebase. However, confirm that the year range
2017-2025is correct and update it if necessary.
95-115: Consider additional error handlingIn
rename(), a failure in field renaming (e.g., if a field no longer exists or has unexpected naming) may cause unexpected dataset states. Consider handling such errors gracefully or logging them more explicitly to aid debugging.
116-133: Potential data consistency issues when cleaning up
cleanup()removes metric fields from the dataset. Verify that downstream processes expecting these fields aren’t impacted if cleanup is called prematurely. Consider adding checks or warnings if removing these fields might conflict with in-progress evaluations or reliant operations.fiftyone/utils/eval/regression.py (2)
Line range hint
144-236: Base class extension alignment
RegressionEvaluationincludes calls likecompute_custom_metrics(),rename_custom_metrics(), andcleanup_custom_metrics()from its base. Verify that all relevant behaviors are covered in unit tests to confirm no oversight in edge cases where the user-defined metrics might conflict with standard fields.
Line range hint
447-492: Check edge cases for combined standard + custom metricsWhen merging standard metrics (MSE, R^2, etc.) with custom metrics in
metrics(), confirm that naming conflicts (like overlapping metric keys) are handled gracefully. For instance, a user-defined metric namedmean_squared_errorcould overwrite the standard metric.fiftyone/utils/eval/detection.py (3)
23-27: Consistent import usageYou now import additional base classes from
.base. If the goal is to unify all evaluation methods under these classes, ensure that any future renames or reorganizations keep their references consistent across all evaluation modules to avoid confusion or import cycles.
233-236: Compute custom metrics consistentlyWhen calling
compute_custom_metrics(), verify that the detection evaluation pipeline has completed other computations (like enumerating TP/FP/FN) first. This ensures the custom metric computations include necessary data.
Line range hint
538-553: Type alignment inDetectionResults
DetectionResultsnow inherits fromBaseClassificationResults. This is a logical approach for storing arrays of true/pred labels. Still, confirm that all detection-specific attributes (e.g., bounding boxes, IoU arrays) remain fully compatible when combined with classification-based functionality.fiftyone/utils/eval/base.py (2)
45-53: Validatecustom_metricsusageThe
_get_custom_metricsmethod returns potentially user-defined metrics. Make sure any conflicts with reserved metric names or special keys are handled gracefully.
161-188: Print method looks goodThe
print_metricsand_print_metricsmethods effectively display the computed metrics. Usage oftabulateis a nice touch for readability.🧰 Tools
🪛 Ruff (0.8.2)
179-179: Loop control variable
urinot used within loop bodyRename unused
urito_uri(B007)
fiftyone/utils/eval/segmentation.py (1)
476-480: ValidatemissinglogicConditionally setting
missing = classes[0] if values[0] in (0, "#000000") else Nonemay inadvertently lump uninitialized or unknown classes into background. Verify that this defaulting won't cause confusion in large-scale or multi-class setups.fiftyone/utils/eval/classification.py (3)
368-373: Ensurekis validWhen
k >= len(_logits), top-k evaluation logic is forced to treat all logits as present. Confirm thatkcannot exceed the number of classes in common usage and handle potential mismatches gracefully (e.g., iflogitsis empty).
713-713: New classification results class extends base resultsGood approach. The additional parameters for
custom_metricsintegrate well with the base results. Make sure there's thorough unit test coverage for these new results flows.
Line range hint
770-785: Implicit negative class assumptionThe constructor uses
missing=classes[0]to mark negative classes by default. For tasks with reversed class ordering, ensure that custom workflow or reordering doesn’t break your logic.tests/unittests/operators/registry_tests.py (1)
12-23: LGTM!The reformatting of mock object definitions improves code readability while maintaining the test's functionality.
fiftyone/operators/registry.py (2)
9-10: LGTM!The addition of
eta.core.utilsimport supports the enhanced type checking functionality.
111-114: Verify dynamic class loading.The implementation now supports dynamic class loading for operator types. Ensure that proper error handling is in place for invalid class paths.
Run this script to verify the error handling:
✅ Verification successful
Dynamic class loading implementation is sufficiently safe
The current implementation is adequately safe as it's used in an optional type filtering context where failures gracefully result in no matching operators. The
isinstancecheck provides an additional safety layer, ensuring type compatibility regardless of class resolution success.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test error handling for invalid class paths in list_operators. # Test: Search for try-except blocks around get_class calls rg -A 5 "get_class.*try|try.*get_class" .Length of output: 43
Script:
#!/bin/bash # Find implementation and usage of etau.get_class ast-grep --pattern 'def get_class' # Search for broader error handling around operator type handling rg -B 3 -A 3 "isinstance.*type|if.*type.*:|type\s*=\s*" fiftyone/operators/ # Find imports to locate etau module rg "^from.*etau.*import|^import.*etau" fiftyone/Length of output: 19165
tests/unittests/utils_tests.py (4)
447-448: LGTM!The added comment clearly indicates that this is not a unit test and warns about database modifications.
452-475: LGTM!The test setup is well-structured with:
- Clear test data creation
- Proper cleanup of existing test data
- Well-defined test configs with different versions
479-487: LGTM!The assertions properly handle different scenarios:
- Database admin: Verifies automatic cleanup
- Non-admin: Verifies duplicates remain
- Both: Verifies config consistency
488-490: LGTM!The cleanup in the finally block ensures test isolation by removing test configs.
fiftyone/utils/eval/activitynet.py (2)
43-44: LGTM! Clean implementation of custom metrics support.The addition of the
custom_metricsparameter is well-documented and properly integrated with the superclass.Also applies to: 55-55, 59-64
334-334: LGTM! Consistent implementation of custom metrics support.The changes in the results class maintain consistency with the evaluation config class.
Also applies to: 351-352, 361-362
fiftyone/utils/eval/coco.py (2)
71-72: LGTM! Consistent implementation of custom metrics support.The changes maintain consistency with the ActivityNet implementation.
Also applies to: 89-89, 93-98
275-275: LGTM! Consistent implementation of custom metrics support.The changes maintain consistency with other detection results classes.
Also applies to: 292-293, 302-303
fiftyone/utils/eval/openimages.py (2)
73-74: LGTM! Consistent implementation of custom metrics support.The changes maintain consistency with other evaluation config classes.
Also applies to: 94-94, 98-103
299-299: LGTM! Consistent implementation of custom metrics support.The changes maintain consistency with other detection results classes.
Also applies to: 314-315, 324-325
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
489-489: LGTM!The spread operator is used correctly to include custom metric rows in the summary table.
docs/source/release-notes.rst (2)
Line range hint
22-1164: LGTM!The release notes are well-structured and provide comprehensive documentation of changes across different components. The content follows documentation best practices with appropriate cross-references and explanations.
6-14:⚠️ Potential issueFix incorrect release date in Teams 2.5.0 notes.
The release date is set to January 24, 2025, which is in the future. This should be updated to reflect the actual release date.
-*Released January 24, 2025* +*Released January 24, 2024*Likely invalid or redundant comment.
| custom_metrics = kwargs.get("custom_metrics", None) | ||
| if etau.is_str(custom_metrics): | ||
| kwargs["custom_metrics"] = [custom_metrics] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adopt the static analysis hint for custom_metrics retrieval
Similar to regression.py, remove the redundant second argument in kwargs.get("custom_metrics", None):
- custom_metrics = kwargs.get("custom_metrics", None)
+ custom_metrics = kwargs.get("custom_metrics")📝 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.
| custom_metrics = kwargs.get("custom_metrics", None) | |
| if etau.is_str(custom_metrics): | |
| kwargs["custom_metrics"] = [custom_metrics] | |
| custom_metrics = kwargs.get("custom_metrics") | |
| if etau.is_str(custom_metrics): | |
| kwargs["custom_metrics"] = [custom_metrics] | |
🧰 Tools
🪛 Ruff (0.8.2)
641-641: Use kwargs.get("custom_metrics") instead of kwargs.get("custom_metrics", None)
Replace kwargs.get("custom_metrics", None) with kwargs.get("custom_metrics")
(SIM910)
| def cache_dataset(dataset): | ||
| """Caches the given dataset. | ||
|
|
||
| This method ensures that subsequent calls to | ||
| :func:`fiftyone.core.dataset.load_dataset` in async calls will return this | ||
| dataset singleton. | ||
|
|
||
| See :meth:`load_and_cache_dataset` for additional details. | ||
|
|
||
| Args: | ||
| dataset: a :class:`fiftyone.core.dataset.Dataset` | ||
| """ | ||
| _cache[dataset.name] = dataset | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and input validation.
The function should include type hints and validate its input:
-def cache_dataset(dataset):
+def cache_dataset(dataset: "fiftyone.core.dataset.Dataset") -> None:
"""Caches the given dataset.
This method ensures that subsequent calls to
:func:`fiftyone.core.dataset.load_dataset` in async calls will return this
dataset singleton.
See :meth:`load_and_cache_dataset` for additional details.
Args:
dataset: a :class:`fiftyone.core.dataset.Dataset`
+
+ Raises:
+ ValueError: If dataset is None or not a Dataset instance
"""
+ if dataset is None:
+ raise ValueError("Dataset cannot be None")
+ if not isinstance(dataset, fod.Dataset):
+ raise ValueError("Expected a Dataset instance")
_cache[dataset.name] = datasetCommittable suggestion skipped: line range outside the PR's diff.
| FiftyOne 1.3.0 | ||
| -------------- | ||
| *Released January 24, 2025* | ||
|
|
There was a problem hiding this comment.
Fix incorrect release date in FiftyOne 1.3.0 notes.
The release date is set to January 24, 2025, which is in the future. This should be updated to reflect the actual release date.
-*Released January 24, 2025*
+*Released January 24, 2024*📝 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.
| FiftyOne 1.3.0 | |
| -------------- | |
| *Released January 24, 2025* | |
| FiftyOne 1.3.0 | |
| -------------- | |
| *Released January 24, 2024* | |
Add documentation for custom metrics
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/source/user_guide/evaluation.rst (2)
2004-2188: Documentation for custom evaluation metrics looks great!The section is well-structured and provides comprehensive coverage of both using and developing custom metrics. The examples are clear and the implementation details are thorough.
Consider adding a note about error handling best practices when implementing the
compute()method, such as handling edge cases where the evaluation results might be invalid or incomplete.
Line range hint
2189-2999: Documentation for custom evaluation backends is comprehensive!The section effectively covers all evaluation types and provides clear instructions for implementing custom backends. The configuration documentation is particularly thorough.
Consider adding a complete working example of a custom backend implementation for at least one evaluation type. This would help users better understand how all the pieces fit together in practice.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/source/images/evaluation/custom-evaluation-metric.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (2)
docs/source/release-notes.rst(1 hunks)docs/source/user_guide/evaluation.rst(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (2)
docs/source/release-notes.rst (2)
6-15: LGTM! Well-structured release notes for Teams 2.5.0.The release notes are properly formatted and provide clear information about the changes in this version.
18-21:⚠️ Potential issueFix incorrect release date in FiftyOne 1.3.0 notes.
The release date is set to January 24, 2025, which is in the future. This should be updated to reflect the actual release date.
Apply this diff to fix the date:
-*Released January 24, 2025* +*Released January 24, 2024*Likely invalid or redundant comment.
Addresses merge conflicts from #5434.
Summary by CodeRabbit
FiftyOne Release Notes
New Features
Bug Fixes
Documentation
Chores