model evaluation bug fixes #5166
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (2)
Line range hint
1472-1476: Consider enhancing type safety for configuration objectsWhile the type definitions are good, consider making the configuration object properties more strictly typed by using literal types for the sort options.
type PLOT_CONFIG_TYPE = { - sortBy?: string; + sortBy?: 'default' | 'az' | 'za' | 'best' | 'worst' | 'mc' | 'lc'; limit?: number; log?: boolean; };
Line range hint
39-46: Consider optimizing state managementThe component uses multiple separate useState hooks for dialog configurations. Consider combining related states to reduce potential re-renders and improve maintainability.
- const [classPerformanceConfig, setClassPerformanceConfig] = - useState<PLOT_CONFIG_TYPE>({}); - const [classPerformanceDialogConfig, setClassPerformanceDialogConfig] = - useState<PLOT_CONFIG_DIALOG_TYPE>(DEFAULT_BAR_CONFIG); - const [confusionMatrixConfig, setConfusionMatrixConfig] = - useState<PLOT_CONFIG_TYPE>({ log: true }); - const [confusionMatrixDialogConfig, setConfusionMatrixDialogConfig] = - useState<PLOT_CONFIG_DIALOG_TYPE>(DEFAULT_BAR_CONFIG); + const [configs, setConfigs] = useState({ + classPerformance: { + current: {} as PLOT_CONFIG_TYPE, + dialog: DEFAULT_BAR_CONFIG as PLOT_CONFIG_DIALOG_TYPE + }, + confusionMatrix: { + current: { log: true } as PLOT_CONFIG_TYPE, + dialog: DEFAULT_BAR_CONFIG as PLOT_CONFIG_DIALOG_TYPE + } + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
app/packages/components/package.jsonis excluded by!**/*.jsonapp/packages/core/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(1 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(1 hunks)
🧰 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.
🔇 Additional comments (2)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
357-357: LGTM: Bug fix for storing pending evaluations
The change correctly uses pending_evaluations_in_store instead of pending_evaluations, ensuring that the updated state of pending evaluations is properly stored. This fixes a bug where pending evaluation updates might have been lost.
Let's verify the usage of these variables in the method:
✅ Verification successful
LGTM: Correct usage of pending evaluations store variable
The change correctly uses pending_evaluations_in_store to store the updated state. The code search confirms that:
- The variable is properly initialized from the store at the start of the method
- It maintains a per-dataset mapping of pending evaluations
- The same pattern of using
pending_evaluations_in_storeis consistently used throughout the codebase - The fix ensures that updates to pending evaluations are properly persisted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of pending evaluations handling
# Look for other places where pending evaluations are stored
rg -A 5 "pending_evaluations.*store\.set"
Length of output: 43
Script:
#!/bin/bash
# Let's try a broader search to understand the context of pending evaluations
rg -B 5 -A 5 "pending_evaluations_in_store"
Length of output: 3029
Script:
#!/bin/bash
# Let's also look for the method definition to understand the full context
ast-grep --pattern 'def load_pending_evaluations($$$)'
Length of output: 56
Script:
#!/bin/bash
# Search for any store operations related to pending evaluations with a broader pattern
rg -B 5 -A 5 'store.*pending_evaluations'
Length of output: 4018
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1353-1353: LGTM: Dialog cancel handler implementation is correct
The cancel button's onClick handler properly calls the close function and follows React best practices.
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
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
These updates aim to streamline user interactions and improve the overall evaluation management experience.