fix confusion matrix in model evaluation panel #5186
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
imanjra
force-pushed
the
bugfix/model-eval-conf-matrix
branch
from
6d568a0 to
2a72908
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/utils/eval/base.py (1)
362-365: Consider optimizing the numpy delete operationsWhile the current implementation is correct, you could optimize the nested np.delete operations by using a single operation with both axes.
Consider this optimization:
- ids = np.delete( - np.delete(ids, rm_inds, axis=0), rm_inds, axis=1 - ) + ids = np.delete(np.delete(ids, rm_inds, 0), rm_inds, 1)This change:
- Makes the code more concise
- Maintains the same functionality
- Slightly improves readability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/core/plots/plotly.py(7 hunks)fiftyone/utils/eval/base.py(2 hunks)
🔇 Additional comments (6)
fiftyone/utils/eval/base.py (3)
336-337: LGTM: Proper handling of ids for (other, other) case
The addition correctly maintains consistency between the confusion matrix and ids array when omitting the (other, other) case.
344-346: LGTM: Proper handling of ids for missing label cases
The addition correctly maintains consistency between the confusion matrix and ids array when handling (other, missing) and (missing, other) cases.
Line range hint 336-365: Verify the impact on model evaluation panel
The changes look good and maintain proper synchronization between the confusion matrix and ids array. Let's verify the impact on the UI.
✅ Verification successful
Changes are safe and properly integrated with the UI
The changes to the confusion matrix and ids array handling are well-integrated with the model evaluation panel. The panel uses the confusion matrix functionality through a clean interface that's not affected by the internal changes to handle tabulate_ids. The UI implementation in fiftyone/operators/builtins/panels/model_evaluation/__init__.py shows proper abstraction where it only interacts with the high-level confusion matrix API.
- The model evaluation panel uses
confusion_matrix()method without direct dependency on the ids array implementation - The changes maintain backward compatibility with existing confusion matrix usage
- The UI's matrix display and colorscale generation remain unaffected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any UI components that might be affected by these changes
# Look for UI components using confusion matrix
rg -l "confusion.*matrix" --type tsx --type jsx
# Look for components importing this module
rg -l "fiftyone.*eval.*base" --type tsx --type jsx --type py
Length of output: 168
Script:
#!/bin/bash
# Search for confusion matrix usage in JavaScript/TypeScript files
rg -l "confusion.*matrix" --type js
# Search for imports of the eval base module in JavaScript/TypeScript files
rg -l "fiftyone.*eval.*base" --type js
# Search for Python files that import or use this module
rg -l "from fiftyone.utils.eval.base|import fiftyone.utils.eval.base" --type py
# Look for any UI components or tests related to confusion matrix
rg -l "ConfusionMatrix" --type js --type py
Length of output: 204
Script:
#!/bin/bash
# Search for any Python files that use or test confusion matrix functionality
rg -l "confusion_matrix" --type py
# Search for any files containing model evaluation panel references
rg -l "ModelEvaluationPanel|model_evaluation" --type py
# Look for test files related to evaluation
fd "test.*eval.*\.py$"
# Check for usage of the evaluation module in the app
rg -l "fiftyone\.utils\.eval" --type py
Length of output: 774
Script:
#!/bin/bash
# Check the model evaluation panel implementation for confusion matrix usage
rg -C 5 "confusion_matrix" fiftyone/operators/builtins/panels/model_evaluation/__init__.py
# Check evaluation tests for confusion matrix with ids
rg -C 5 "tabulate_ids.*confusion_matrix|confusion_matrix.*tabulate_ids" tests/unittests/evaluation_tests.py tests/intensive/evaluation_tests.py
Length of output: 1815
fiftyone/core/plots/plotly.py (3)
106-107: LGTM: Parameter additions for confusion matrix fields
The addition of gt_field and pred_field parameters enhances the confusion matrix visualization by allowing customization of field names in the hover information.
Also applies to: 130-131
137-138: LGTM: Enhanced hover template with field names
The hover template now properly displays customized field names from gt_field and pred_field parameters, with sensible defaults of "truth" and "predicted" when not provided.
Also applies to: 142-143
1972-1975: LGTM: Consistent InteractiveHeatmap implementation
The InteractiveHeatmap class has been properly updated to support custom field names:
- Well-documented parameters in the docstring
- Proper initialization and storage of the parameters
- Consistent hover template implementation
Also applies to: 1991-1992, 2010-2011, 2206-2207, 2211-2212
There was a problem hiding this comment.
This is looking much better! 🥇
Can you add two more small tweaks to the confusion matrix display?
- Use the
hovertemplateas implemented here so that the names of the ground truth and predicted fields are shown in the tooltip:
- Flip the y-axis so that the confusion matrix has the standard descending diagonal as pictured below. You should be able to achieve this via
autorange="reversed"
|
to confirm, using y axis as |
yes correct, thanks ✅ |
| hovertemplate: | ||
| [ | ||
| "<b>count: %{z:d}</b>", | ||
| `ground_truth: %{y}`, |
There was a problem hiding this comment.
@lanzhenw ideally we would show the actual field names here, rather than the hard-coded ground_truth and predictions strings, like is implemented here.
In order to achieve that, you'd need access to config.gt_field and config.pred_field from the evaluation's config. Do you have access to those in the frontend? If you do, then let's use the actual field names.
If you don't easily have access to the field names, then please just use ground truth rather than ground_truth and we can add field names in a follow-up PR.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
Outdated
Show resolved
Hide resolved
What changes are proposed in this pull request?
fix confusion matrix in model evaluation panel
How is this patch tested? If it is not, please explain why.
Using model evaluation panel
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Chores