Merge release/v1.2.0 to develop
#5276
Conversation
WalkthroughThe changes in this pull request focus on enhancing the evaluation logic within 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(5 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(5 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 (6)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (3)
100-105: Implementation of is_binary_classification method is appropriate
The method correctly determines if the evaluation is a binary classification based on the evaluation type and method in the configuration.
309-309: Updated method call to get_tp_fp_fn aligns with new signature
The call to get_tp_fp_fn now passes info and results, matching the updated method definition.
429-434: Verify necessity of converting field to uppercase in binary classification
In the load_view method, for binary classification, the field is converted to uppercase before matching:
uppercase_field = field.upper()
view = ctx.dataset.match(
{computed_eval_key: {"$eq": uppercase_field}}
)Please ensure that this uppercase conversion is necessary and consistent with how the evaluation keys are stored. If not required, consider removing it to avoid potential case-sensitivity issues.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (3)
169-169: Correctly capturing evaluationMethod from configuration
The new variable evaluationMethod accurately retrieves the evaluation method from the configuration object.
178-180: Accurate determination of binary classification evaluations
The isBinaryClassification variable appropriately identifies binary classification evaluations by checking both the evaluation type and method. Updating showTpFpFn to include isBinaryClassification ensures that true positives, false positives, and false negatives are displayed correctly.
392-392: Consistent visibility control for TP, FP, FN metrics
The hide property in the summaryRows array is updated from !isObjectDetection to !showTpFpFn, ensuring that the TP, FP, and FN metrics are displayed for both object detection and binary classification evaluations as intended.
Also applies to: 407-407, 422-422
| def get_tp_fp_fn(self, info, results): | ||
| # Binary classification | ||
| if self.is_binary_classification(info): | ||
| neg_label, pos_label = results.classes | ||
| tp_count = np.count_nonzero( | ||
| (results.ytrue == pos_label) & (results.ypred == pos_label) | ||
| ) | ||
| fp_count = np.count_nonzero( | ||
| (results.ytrue != pos_label) & (results.ypred == pos_label) | ||
| ) | ||
| fn_count = np.count_nonzero( | ||
| (results.ytrue == pos_label) & (results.ypred != pos_label) | ||
| ) | ||
| return tp_count, fp_count, fn_count | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review calculations for false positives and false negatives in object detection
In the get_tp_fp_fn method, the calculations for false positives (fp_count) and false negatives (fn_count) in the object detection section may not align with standard definitions.
- Currently,
fp_countis calculated asnp.count_nonzero(results.ytrue == results.missing), which counts instances where the ground truth is missing but predictions exist. This may actually represent false negatives. - Similarly,
fn_countis calculated asnp.count_nonzero(results.ypred == results.missing), which counts instances where predictions are missing but ground truths exist. This may represent false positives.
Consider revising the calculations to ensure they correctly represent false positives and false negatives according to standard object detection metrics.
sample update / overlay recoloring performance optimization
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/overlays/base.ts(2 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(2 hunks)app/packages/looker/src/overlays/segmentation.ts(2 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/detection.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/heatmap.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/base.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/lookers/abstract.ts (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 (8)
app/packages/looker/src/lookers/abstract.ts (1)
26-26: LGTM!
The import statement correctly includes LabelMask for later usage.
app/packages/looker/src/overlays/base.ts (2)
70-70: LGTM!
Adding label?: BaseLabel to the Overlay interface enhances its capability to include label information.
86-86: LGTM!
Changing label to readonly provides immutability and exposes it publicly for consistent access across overlays.
app/packages/looker/src/overlays/heatmap.ts (2)
42-42: LGTM!
Changing label to readonly enhances accessibility while maintaining immutability.
210-210: LGTM!
The use of optional chaining in this.label.map?.bitmap?.close(); simplifies the cleanup process and prevents potential errors when properties may be undefined.
app/packages/looker/src/overlays/segmentation.ts (2)
33-33: LGTM! Good use of readonly modifier
The change from private to public readonly improves the API while maintaining immutability. This aligns with TypeScript best practices and the pattern used in other overlay classes.
265-265: LGTM! Clean implementation of cleanup
The simplified cleanup implementation using optional chaining is more concise while maintaining the same safety guarantees.
app/packages/looker/src/overlays/detection.ts (1)
265-265: LGTM! Consistent cleanup implementation
The simplified cleanup implementation aligns with the pattern used in other overlay classes and makes good use of TypeScript's optional chaining.
| for (const overlay of this.sampleOverlays ?? []) { | ||
| overlay.cleanup(); | ||
| } |
There was a problem hiding this comment.
Check if cleanup method exists before invoking
The cleanup method on overlay is optional in the Overlay interface. To prevent potential runtime errors, check if cleanup exists before calling it.
Apply this diff to ensure safety:
for (const overlay of this.sampleOverlays ?? []) {
- overlay.cleanup();
+ if (overlay.cleanup) {
+ overlay.cleanup();
+ }
}📝 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.
| for (const overlay of this.sampleOverlays ?? []) { | |
| overlay.cleanup(); | |
| } | |
| for (const overlay of this.sampleOverlays ?? []) { | |
| if (overlay.cleanup) { | |
| overlay.cleanup(); | |
| } | |
| } |
| const arrayBuffers: ArrayBuffer[] = []; | ||
|
|
||
| for (const overlay of this.pluckedOverlays ?? []) { | ||
| let overlayData: LabelMask = null; |
There was a problem hiding this comment.
Correct the type declaration to accommodate null assignment
The variable overlayData is declared as LabelMask but is assigned null. In TypeScript, to allow null assignment, the type should be LabelMask | null.
Apply this diff to fix the type declaration:
-let overlayData: LabelMask = null;
+let overlayData: LabelMask | null = null;📝 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.
| let overlayData: LabelMask = null; | |
| let overlayData: LabelMask | null = null; |
| try { | ||
| labelsWorker.postMessage(workerArgs, transfer); | ||
| } catch (error) { | ||
| // rarely we'll get a DataCloneError | ||
| // if one of the buffers is detached and we didn't catch it | ||
| // try again without transferring the buffers (copying them) | ||
| if (error.name === "DataCloneError") { | ||
| labelsWorker.postMessage(workerArgs); | ||
| } else { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the error in the catch block for better TypeScript safety
In TypeScript, the error in a catch clause is of type unknown. To safely access error.name, you should assert the type or check if it's an instance of Error.
Apply this diff to type the error:
} catch (error) {
+ if (error instanceof Error) {
if (error.name === "DataCloneError") {
labelsWorker.postMessage(workerArgs);
} else {
throw error;
}
+ } else {
+ throw error;
+ }
}📝 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.
| try { | |
| labelsWorker.postMessage(workerArgs, transfer); | |
| } catch (error) { | |
| // rarely we'll get a DataCloneError | |
| // if one of the buffers is detached and we didn't catch it | |
| // try again without transferring the buffers (copying them) | |
| if (error.name === "DataCloneError") { | |
| labelsWorker.postMessage(workerArgs); | |
| } else { | |
| throw error; | |
| } | |
| } | |
| try { | |
| labelsWorker.postMessage(workerArgs, transfer); | |
| } catch (error) { | |
| // rarely we'll get a DataCloneError | |
| // if one of the buffers is detached and we didn't catch it | |
| // try again without transferring the buffers (copying them) | |
| if (error instanceof Error) { | |
| if (error.name === "DataCloneError") { | |
| labelsWorker.postMessage(workerArgs); | |
| } else { | |
| throw error; | |
| } | |
| } else { | |
| throw error; | |
| } | |
| } |
| // close the copied bitmap | ||
| label[overlayField].bitmap.close(); | ||
| label[overlayField].bitmap = null; |
There was a problem hiding this comment.
Ensure bitmap exists before calling close()
Before calling label[overlayField].bitmap.close(), ensure that label[overlayField].bitmap is not undefined or null to prevent potential runtime errors.
Apply this diff to add a null check:
+if (label[overlayField].bitmap) {
label[overlayField].bitmap.close();
+}
label[overlayField].bitmap = null;📝 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.
| // close the copied bitmap | |
| label[overlayField].bitmap.close(); | |
| label[overlayField].bitmap = null; | |
| // close the copied bitmap | |
| if (label[overlayField].bitmap) { | |
| label[overlayField].bitmap.close(); | |
| } | |
| label[overlayField].bitmap = null; |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
116-138:⚠️ Potential issueReview object detection metrics calculation.
While the binary classification implementation is correct, the object detection metrics calculation needs review:
fp_countcurrently counts cases where ground truth is missingfn_countcurrently counts cases where predictions are missingThis might lead to incorrect metric reporting.
Consider revising the object detection section:
if info.config.type == "detection": - tp_count = np.count_nonzero(results.ytrue == results.ypred) - fp_count = np.count_nonzero(results.ytrue == results.missing) - fn_count = np.count_nonzero(results.ypred == results.missing) + tp_count = np.count_nonzero(results.ytrue == results.ypred) + fp_count = np.count_nonzero(results.ypred == results.missing) + fn_count = np.count_nonzero(results.ytrue == results.missing) return tp_count, fp_count, fn_count
🧹 Nitpick comments (6)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/ErrorIcon.tsx (1)
6-6: Consider making icon dimensions configurableThe width and height are hardcoded which might limit the icon's reusability. Consider using the inherited size from the SvgIcon component.
- <SvgIcon width="54" height="61" viewBox="0 0 54 61" fill="none" {...props}> + <SvgIcon viewBox="0 0 54 61" fill="none" {...props}>app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Error.tsx (2)
11-13: Add aria-label for better accessibilityThe back button should have an aria-label for better screen reader support.
- <Button onClick={onBack} startIcon={<West />} color="secondary"> + <Button + onClick={onBack} + startIcon={<West />} + color="secondary" + aria-label="Return to model evaluation" + >
25-31: Consider externalizing strings for internationalizationText content should be externalized to support multiple languages.
Consider using a translation framework like react-intl or i18next to manage these strings:
- "Analyze and improve models collaboratively with your team"
- "The Model Evaluation panel currently supports only classification, detection, and segmentation evaluations"
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
291-299: Consider adding error handling for invalid field names.The fallback logic is well-structured, but the method could benefit from additional error handling:
- Validate the
gt_fieldparameter- Handle potential exceptions when accessing dataset attributes
def get_mask_targets(self, dataset, gt_field): + if not gt_field: + return None + mask_targets = dataset.mask_targets.get(gt_field, None) if mask_targets: return mask_targets if dataset.default_mask_targets: return dataset.default_mask_targets return Noneapp/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (2)
45-45: Rename imported Error component to avoid shadowing global Error.The current import shadows the built-in global Error type, which could lead to confusion.
-import Error from "./Error"; +import ErrorView from "./Error";Update the usage accordingly:
- return <Error onBack={navigateBack} />; + return <ErrorView onBack={navigateBack} />;🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
1647-1657: Consider adding TypeScript type safety.While the implementation is functionally correct, it could benefit from better type safety:
-function getMatrix(matrices, config, maskTargets, compareMaskTargets?) { +interface MatrixConfig { + sortBy?: 'az' | 'za' | 'mc' | 'lc'; + limit?: number; +} + +interface MaskTargets { + [key: string]: string; +} + +function getMatrix( + matrices: any, + config: MatrixConfig, + maskTargets?: MaskTargets, + compareMaskTargets?: MaskTargets +) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Error.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/ErrorIcon.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(27 hunks)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts(1 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/ErrorIcon.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.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Error.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.
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.
🪛 Biome (1.9.4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/utils.ts
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Error.tsx
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
[error] 45-45: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 Ruff (0.8.2)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py
317-317: Undefined name serialized_info
(F821)
🔇 Additional comments (3)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
101-105: LGTM! Clear and focused implementation.
The method follows the single responsibility principle and has a clear, descriptive name that accurately reflects its purpose.
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (2)
94-101: LGTM! Well-structured error handling.
The error handling implementation:
- Uses React's useMemo for performance optimization
- Cleanly separates evaluation and comparison error states
- Follows React best practices
102-107: LGTM! Robust mask targets implementation.
The implementation:
- Safely handles undefined values with default empty objects
- Uses React's useMemo appropriately for performance
- Maintains clean separation of concerns
| @@ -35,7 +35,7 @@ export function getNumericDifference( | |||
| export function formatValue(value: string | number, fractionDigits = 3) { | |||
| const numericValue = | |||
| typeof value === "number" ? value : parseFloat(value as string); | |||
| if (!isNaN(numericValue)) { | |||
| if (!isNaN(numericValue) && numericValue == value) { | |||
There was a problem hiding this comment.
Replace loose equality and unsafe isNaN with strict checks
The current implementation has two potential issues:
- Using loose equality (
==) can lead to unexpected type coercion - Using global
isNaNis unsafe as it performs type coercion
Apply this diff to fix both issues:
- if (!isNaN(numericValue) && numericValue == value) {
+ if (!Number.isNaN(numericValue) && numericValue === value) {📝 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.
| if (!isNaN(numericValue) && numericValue == value) { | |
| if (!Number.isNaN(numericValue) && numericValue === value) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| import React from "react"; | ||
| import ErrorIcon from "./ErrorIcon"; | ||
|
|
||
| export default function Error(props: ErrorProps) { |
There was a problem hiding this comment.
Rename component to avoid shadowing global Error
The component name shadows the global Error constructor which could lead to confusion.
-export default function Error(props: ErrorProps) {
+export default function EvaluationError(props: ErrorProps) {📝 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.
| export default function Error(props: ErrorProps) { | |
| export default function EvaluationError(props: ErrorProps) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| evaluation_type = info.config.type | ||
| if evaluation_type not in SUPPORTED_EVALUATION_TYPES: | ||
| ctx.panel.set_data( | ||
| f"evaluation_{computed_eval_key}_error", | ||
| {"error": "unsupported", "info": serialized_info}, | ||
| ) | ||
| return | ||
| serialized_info = info.serialize() | ||
| gt_field = info.config.gt_field | ||
| mask_targets = ( | ||
| self.get_mask_targets(ctx.dataset, gt_field) | ||
| if evaluation_type == "segmentation" | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Fix undefined variable reference.
There's a bug in the error handling code where serialized_info is used before it's defined:
evaluation_type = info.config.type
+serialized_info = info.serialize()
if evaluation_type not in SUPPORTED_EVALUATION_TYPES:
ctx.panel.set_data(
f"evaluation_{computed_eval_key}_error",
{"error": "unsupported", "info": serialized_info},
)
return
-serialized_info = info.serialize()📝 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.
| evaluation_type = info.config.type | |
| if evaluation_type not in SUPPORTED_EVALUATION_TYPES: | |
| ctx.panel.set_data( | |
| f"evaluation_{computed_eval_key}_error", | |
| {"error": "unsupported", "info": serialized_info}, | |
| ) | |
| return | |
| serialized_info = info.serialize() | |
| gt_field = info.config.gt_field | |
| mask_targets = ( | |
| self.get_mask_targets(ctx.dataset, gt_field) | |
| if evaluation_type == "segmentation" | |
| else None | |
| ) | |
| evaluation_type = info.config.type | |
| serialized_info = info.serialize() | |
| if evaluation_type not in SUPPORTED_EVALUATION_TYPES: | |
| ctx.panel.set_data( | |
| f"evaluation_{computed_eval_key}_error", | |
| {"error": "unsupported", "info": serialized_info}, | |
| ) | |
| return | |
| gt_field = info.config.gt_field | |
| mask_targets = ( | |
| self.get_mask_targets(ctx.dataset, gt_field) | |
| if evaluation_type == "segmentation" | |
| else None | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
317-317: Undefined name serialized_info
(F821)
Similarity race condition patch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fiftyone/utils/torch.py (1)
576-578: Good defensive programming practice!The added null check prevents potential
AttributeErrorwhenself._no_gradisNone. This is a good fix sinceself._no_gradis initialized asNonein__init__and only set when__enter__is called.Consider also adding a debug log when
self._no_gradisNoneto help track unexpected cases:if self._no_grad is not None: self._no_grad.__exit__(*args) self._no_grad = None + else: + logger.debug("self._no_grad was None in __exit__")
add support for collection overlays
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
fiftyone/server/metadata.py (1)
Line range hint
467-490: Consolidate detection fields logicThe logic in '_get_additional_media_fields' lines 467-490 clearly handles detection fields, including ‘fol.Detections’ separately. However, the branching might become unwieldy if further label types arrive. Consider generalizing or extracting a helper function to maintain clarity.
🧹 Nitpick comments (4)
fiftyone/server/metadata.py (2)
87-87: Improve clarity in parameter namingConsider renaming 'detections_fields' in this call site to maintain alignment with the new variable naming in '_get_additional_media_fields'. This helps improve readability for future maintainers.
400-424: Bundled detection mask paths might balloon field listsLines 408-424 append detection mask paths for each detection in a loop. For large sets of detections, this can lead to a very large array of fields. Consider short-circuiting or limiting the appended paths if memory or performance become concerns.
[performance]app/packages/looker/src/worker/disk-overlay-decoder.ts (2)
28-31: Clarify overlay collection paramsThe introduction of 'overlayCollectionProcessingParams' is helpful for indexing. Consider documenting its object shape at the top of the file or using a dedicated type to clarify usage across the codebase.
105-105: Skip removing query for advanced local pathsThe query removal step is correct for typical local usage. However, if “filepath=” appears in some advanced local path or if future expansions require query parameters, reintroducing them might be necessary. Keep an eye on edge usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/looker/src/worker/disk-overlay-decoder.ts(3 hunks)fiftyone/server/metadata.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/disk-overlay-decoder.ts (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 (8)
fiftyone/server/metadata.py (4)
15-15: Consider verifying usage vs. built-in approaches
The new utilization of 'pydash.get' can certainly simplify deeply nested lookups. However, if a standard library approach (e.g., a default dictionary or a custom helper) already suffices, verify whether it’s still needed to add this dependency.
35-36: Enhance detection label field mapping
Adding “mask_path” entries for both fol.Detection and fol.Detections enables expanded functionality. It may be prudent to confirm that all detection subtypes (e.g., polylines, keypoints) do not require similar mapping.
74-78: Confirm tuple structure alignment
The return structure from '_get_additional_media_fields' now includes (opm_field, detections_fields, additional_fields). Confirm that all downstream consumers properly destructure this tuple in all usage scenarios—particularly if prior code only anticipated two return values.
442-445: Short-circuit null or empty path checks
Lines 442-445 skip processing if “path” is falsy, which is correct. As an enhancement, you could break early when multiple fields are guaranteed absent, though that may be a micro-optimization.
app/packages/looker/src/worker/disk-overlay-decoder.ts (4)
34-34: Validate existence of list field
You’re verifying 'label.detections' existence for DETECTIONS on line 34. Good. Ensure that no other list-based label type is incorrectly routed here if 'cls' has changed upstream.
83-97: Dynamic source resolution is clear
Lines 83-97 properly handle the dynamic path resolution, especially for sub-label fields. The logic is intuitive. If usage grows, consider a helper function to build these hierarchical keys.
99-99: Confirm local usage of getSampleSrc(...)
Ensure that 'getSampleSrc()' usage for overlay images is consistent with how the broader codebase resolves file paths, especially for remote or network-based assets. Minor, but worth verifying.
70-74:
Avoid redundant double-close calls
Line 72 and line 74 both close the same bitmap. After the bitmap is set to null in line 73, calling 'close()' again in line 74 will likely fail. Use a single close call or re-check the bitmap.
if (
label[overlayField] &&
label[overlayField].bitmap &&
!label[overlayField].image
) {
const height = label[overlayField].bitmap.height;
const width = label[overlayField].bitmap.width;
- label[overlayField].bitmap.close();
- label[overlayField].bitmap = null;
- label[overlayField].image = new ArrayBuffer(height * width * 4);
- label[overlayField].bitmap.close();
- label[overlayField].bitmap = null;
+ label[overlayField].bitmap.close();
+ label[overlayField].bitmap = null;
+ label[overlayField].image = new ArrayBuffer(height * width * 4);
}Likely invalid or redundant comment.
| for (let i = 0; i < label.detections.length; i++) { | ||
| const detection = label.detections[i]; | ||
| promises.push( | ||
| decodeOverlayOnDisk( | ||
| field, | ||
| detection, | ||
| coloring, | ||
| customizeColorSetting, | ||
| colorscale, | ||
| {}, | ||
| sources, | ||
| DETECTION, | ||
| maskPathDecodingPromises, | ||
| maskTargetsBuffers | ||
| maskTargetsBuffers, | ||
| { idx: i, cls: DETECTIONS } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent deep call chaining
Recursively calling 'decodeOverlayOnDisk' in the loop can cause correlated fetch operations for each detection. For large detections arrays, the concurrency might be too high. Consider chunking the promises or using concurrency control to avoid potential overhead.
[performance]
Optimize view callbacks for model evaluation panel
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (3)
110-135: Add docstring documentation for clarity.The
get_tp_fp_fnmethod would benefit from docstring documentation explaining the parameters, return values, and the logic for both binary classification and object detection cases.def get_tp_fp_fn(self, info, results): + """Calculate true positives, false positives, and false negatives. + + Args: + info: Evaluation info containing configuration + results: Evaluation results containing predictions and ground truth + + Returns: + tuple: (tp_count, fp_count, fn_count) or (None, None, None) if not applicable + """
288-296: Add docstring documentation.The
get_mask_targetsmethod would benefit from docstring documentation explaining its purpose and return value.def get_mask_targets(self, dataset, gt_field): + """Get mask targets for segmentation evaluation. + + Args: + dataset: Dataset containing mask targets + gt_field: Ground truth field name + + Returns: + dict or None: Mask targets if available, otherwise None + """
437-556: Consider breaking down complex view loading logic.The view loading logic has become quite complex with multiple nested conditions. Consider extracting the classification and detection view logic into separate methods for better maintainability.
+ def _load_classification_view(self, eval_view, view_type, info, x, y, field, gt_field, pred_field, gt_field2, pred_field2): + """Handle view loading for classification evaluations.""" + if view_type == "class": + expr = F(f"{gt_field}.label") == x + expr |= F(f"{pred_field}.label") == x + if gt_field2 is not None: + expr |= F(f"{gt_field2}.label") == x + if pred_field2 is not None: + expr |= F(f"{pred_field2}.label") == x + return eval_view.match(expr) + elif view_type == "matrix": + expr = F(f"{gt_field}.label") == y + expr &= F(f"{pred_field}.label") == x + return eval_view.match(expr) + elif view_type == "field": + if info.config.method == "binary": + expr = F(f"{eval_key}") == field.upper() + else: + expr = F(f"{eval_key}") == field + return eval_view.match(expr) + return None + def _load_detection_view(self, eval_view, view_type, x, y, missing, gt_field, pred_field, gt_field2, pred_field2, gt_root, pred_root, gt_root2, pred_root2, eval_key): + """Handle view loading for detection evaluations.""" + if view_type == "class": + view = eval_view.filter_labels(gt_field, F("label") == x, only_matches=False) + expr = F(gt_root).length() > 0 + view = view.filter_labels(pred_field, F("label") == x, only_matches=False) + expr |= F(pred_root).length() > 0 + if gt_field2 is not None: + view = view.filter_labels(gt_field2, F("label") == x, only_matches=False) + expr |= F(gt_root2).length() > 0 + if pred_field2 is not None: + view = view.filter_labels(pred_field2, F("label") == x, only_matches=False) + expr |= F(pred_root2).length() > 0 + return view.match(expr) + elif view_type == "matrix": + if y == missing: + expr = (F("label") == x) & (F(eval_key) == "fp") + return eval_view.filter_labels(pred_field, expr, only_matches=True) + elif x == missing: + expr = (F("label") == y) & (F(eval_key) == "fn") + return eval_view.filter_labels(gt_field, expr, only_matches=True) + else: + view = eval_view.filter_labels(gt_field, F("label") == y, only_matches=False) + expr = F(gt_root).length() > 0 + view = view.filter_labels(pred_field, F("label") == x, only_matches=False) + expr &= F(pred_root).length() > 0 + return view.match(expr) + elif view_type == "field": + if field == "tp": + view = eval_view.filter_labels(gt_field, F(eval_key) == field, only_matches=False) + return view.filter_labels(pred_field, F(eval_key) == field, only_matches=True) + elif field == "fn": + return eval_view.filter_labels(gt_field, F(eval_key) == field, only_matches=True) + else: + return eval_view.filter_labels(pred_field, F(eval_key) == field, only_matches=True) + return None def load_view(self, ctx): view_type = ctx.params.get("type", None) if view_type == "clear": ctx.ops.clear_view() return view_state = ctx.panel.get_state("view") or {} view_options = ctx.params.get("options", {}) eval_key = view_state.get("key") eval_key = view_options.get("key", eval_key) eval_view = ctx.dataset.load_evaluation_view(eval_key) info = ctx.dataset.get_evaluation_info(eval_key) pred_field = info.config.pred_field gt_field = info.config.gt_field eval_key2 = view_state.get("compareKey", None) pred_field2 = None gt_field2 = None if eval_key2: info2 = ctx.dataset.get_evaluation_info(eval_key2) pred_field2 = info2.config.pred_field if info2.config.gt_field != gt_field: gt_field2 = info2.config.gt_field x = view_options.get("x", None) y = view_options.get("y", None) field = view_options.get("field", None) missing = ctx.panel.get_state("missing", "(none)") if info.config.type == "classification": - if view_type == "class": - # All GT/predictions of class `x` - expr = F(f"{gt_field}.label") == x - expr |= F(f"{pred_field}.label") == x - if gt_field2 is not None: - expr |= F(f"{gt_field2}.label") == x - if pred_field2 is not None: - expr |= F(f"{pred_field2}.label") == x - view = eval_view.match(expr) - elif view_type == "matrix": - # Specific confusion matrix cell (including FP/FN) - expr = F(f"{gt_field}.label") == y - expr &= F(f"{pred_field}.label") == x - view = eval_view.match(expr) - elif view_type == "field": - if info.config.method == "binary": - # All TP/FP/FN - expr = F(f"{eval_key}") == field.upper() - view = eval_view.match(expr) - else: - # Correct/incorrect - expr = F(f"{eval_key}") == field - view = eval_view.match(expr) + view = self._load_classification_view( + eval_view, view_type, info, x, y, field, + gt_field, pred_field, gt_field2, pred_field2 + ) elif info.config.type == "detection": - _, gt_root = ctx.dataset._get_label_field_path(gt_field) - _, pred_root = ctx.dataset._get_label_field_path(pred_field) - if gt_field2 is not None: - _, gt_root2 = ctx.dataset._get_label_field_path(gt_field2) - if pred_field2 is not None: - _, pred_root2 = ctx.dataset._get_label_field_path(pred_field2) - - if view_type == "class": - # All GT/predictions of class `x` - view = eval_view.filter_labels(gt_field, F("label") == x, only_matches=False) - expr = F(gt_root).length() > 0 - view = view.filter_labels(pred_field, F("label") == x, only_matches=False) - expr |= F(pred_root).length() > 0 - if gt_field2 is not None: - view = view.filter_labels(gt_field2, F("label") == x, only_matches=False) - expr |= F(gt_root2).length() > 0 - if pred_field2 is not None: - view = view.filter_labels(pred_field2, F("label") == x, only_matches=False) - expr |= F(pred_root2).length() > 0 - view = view.match(expr) - elif view_type == "matrix": - if y == missing: - # False positives of class `x` - expr = (F("label") == x) & (F(eval_key) == "fp") - view = eval_view.filter_labels(pred_field, expr, only_matches=True) - elif x == missing: - # False negatives of class `y` - expr = (F("label") == y) & (F(eval_key) == "fn") - view = eval_view.filter_labels(gt_field, expr, only_matches=True) - else: - # All class `y` GT and class `x` predictions in same sample - view = eval_view.filter_labels(gt_field, F("label") == y, only_matches=False) - expr = F(gt_root).length() > 0 - view = view.filter_labels(pred_field, F("label") == x, only_matches=False) - expr &= F(pred_root).length() > 0 - view = view.match(expr) - elif view_type == "field": - if field == "tp": - # All true positives - view = eval_view.filter_labels(gt_field, F(eval_key) == field, only_matches=False) - view = view.filter_labels(pred_field, F(eval_key) == field, only_matches=True) - elif field == "fn": - # All false negatives - view = eval_view.filter_labels(gt_field, F(eval_key) == field, only_matches=True) - else: - # All false positives - view = eval_view.filter_labels(pred_field, F(eval_key) == field, only_matches=True) + _, gt_root = ctx.dataset._get_label_field_path(gt_field) + _, pred_root = ctx.dataset._get_label_field_path(pred_field) + gt_root2 = None + pred_root2 = None + if gt_field2 is not None: + _, gt_root2 = ctx.dataset._get_label_field_path(gt_field2) + if pred_field2 is not None: + _, pred_root2 = ctx.dataset._get_label_field_path(pred_field2) + + view = self._load_detection_view( + eval_view, view_type, x, y, missing, + gt_field, pred_field, gt_field2, pred_field2, + gt_root, pred_root, gt_root2, pred_root2, + eval_key + ) if view is not None: ctx.ops.set_view(view)app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
1655-1665: Add TypeScript type definitions for better type safety.The
getMatrixfunction would benefit from explicit TypeScript types for its parameters and return value.-function getMatrix(matrices, config, maskTargets, compareMaskTargets?) { +interface MatrixConfig { + sortBy?: 'az' | 'za' | 'mc' | 'lc'; + limit?: number; +} + +interface MatrixResult { + labels: string[]; + matrix: number[][]; + colorscale: string; +} + +function getMatrix( + matrices: Record<string, any> | undefined, + config: MatrixConfig, + maskTargets: Record<string, string>, + compareMaskTargets?: Record<string, string> +): MatrixResult | undefined {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx(28 hunks)fiftyone/operators/builtins/panels/model_evaluation/__init__.py(6 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.
🪛 Ruff (0.8.2)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py
314-314: Undefined name serialized_info
(F821)
🪛 Biome (1.9.4)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
[error] 45-45: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (3)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (2)
128-133:
Review object detection metrics calculation.
The calculation of false positives and false negatives in the object detection case may need revision:
fp_countcurrently counts cases where ground truth is missingfn_countcurrently counts cases where predictions are missing
This appears to be reversed based on standard object detection metrics definitions.
- fp_count = np.count_nonzero(results.ytrue == results.missing)
- fn_count = np.count_nonzero(results.ypred == results.missing)
+ fp_count = np.count_nonzero(results.ypred == results.missing)
+ fn_count = np.count_nonzero(results.ytrue == results.missing)Likely invalid or redundant comment.
310-323:
Fix undefined variable reference.
The serialized_info variable is used before it's defined, which will raise an UnboundLocalError.
evaluation_type = info.config.type
+serialized_info = info.serialize()
if evaluation_type not in SUPPORTED_EVALUATION_TYPES:
ctx.panel.set_data(
f"evaluation_{computed_eval_key}_error",
{"error": "unsupported", "info": serialized_info},
)
return
-serialized_info = info.serialize()Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
314-314: Undefined name serialized_info
(F821)
app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx (1)
102-107: LGTM! Clean implementation of mask targets.
The implementation properly uses useMemo for performance optimization and handles both main and comparison mask targets correctly.
| @@ -42,6 +42,7 @@ import { | |||
| } from "@mui/material"; | |||
| import React, { useEffect, useMemo, useState } from "react"; | |||
| import { useRecoilState, useSetRecoilState } from "recoil"; | |||
| import Error from "./Error"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename Error component to avoid shadowing global Error.
The imported Error component shadows the global Error object. Consider renaming it to be more specific, such as EvaluationError.
-import Error from "./Error";
+import EvaluationError from "./Error";Then update its usage:
- return <Error onBack={navigateBack} />;
+ return <EvaluationError onBack={navigateBack} />;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Transformers GPU Support
fix png decoding
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
fiftyone/utils/transformers.py (2)
584-587: Model initialization pattern could be DRYerWhile the device management is consistent, there's repetition in the model initialization pattern across different transformer classes. Consider extracting this common pattern into a helper method.
Here's a suggested implementation:
def _load_model_to_device(model_class, name_or_path): device = "cuda" if torch.cuda.is_available() else "cpu" return model_class.from_pretrained(name_or_path).to(device)Then use it in the transformer classes:
def _load_model(self, config): if config.model is not None: return config.model - device = "cuda" if torch.cuda.is_available() else "cpu" - return transformers.AutoModelForImageClassification.from_pretrained( - config.name_or_path - ).to(device) + return _load_model_to_device( + transformers.AutoModelForImageClassification, + config.name_or_path + )Also applies to: 696-699, 824-827, 878-882, 932-935
1037-1038: Ensure consistent line spacingThere are two consecutive blank lines where one would suffice. While this is a minor issue, maintaining consistent spacing improves code readability.
- - +app/packages/looker/src/worker/canvas-decoder.ts (2)
35-50: Consider explicit handling of 3-channel (RGB) PNG
The code defaults to 4 channels for color type 2 (Truecolor). While safe, it can add overhead if truly only 3 channels are essential. You might reduce memory usage by using 3 channels if you do not require alpha.- if (colorType === 0 || colorType === 4) { - channels = 1; - } else { - channels = 4; - } + if (colorType === 0 || colorType === 4) { + channels = 1; // grayscale + } else if (colorType === 2) { + channels = 3; // skip alpha if not needed + } else { + channels = 4; + }
51-52: Compatibility note for OffscreenCanvas in older browsers
OffscreenCanvas is not universally supported in all browsers—particularly older ones. If broad browser support is a requirement, consider providing a fallback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/looker/src/worker/canvas-decoder.test.ts(0 hunks)app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/painter.ts(1 hunks)fiftyone/utils/transformers.py(12 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/canvas-decoder.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/worker/painter.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/canvas-decoder.ts (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 (4)
fiftyone/utils/transformers.py (2)
454-454: Consistent device determination logic across classes
The changes standardize the device determination logic across different transformer classes using torch.cuda.is_available(). This is a more reliable approach compared to checking model parameters.
Also applies to: 499-499, 751-751, 1087-1087
760-762:
Processor device placement may be unnecessary
The processor is being moved to device with .to(self.device), but processors typically don't need to be placed on a specific device as they handle preprocessing on CPU.
Let's verify if this is necessary:
Consider removing the .to(self.device) call for processors as it may be unnecessary and could potentially cause issues.
app/packages/looker/src/worker/canvas-decoder.ts (1)
32-34: Confirm that all call sites now accept (Blob) instead of (ImageBitmapSource)
Previously, decodeWithCanvas accepted an ImageBitmapSource, but it's now switched to a Blob. Ensure references to decodeWithCanvas are updated to match the new parameter type, preventing type errors.
✅ Verification successful
All call sites correctly use Blob parameter type - no updates needed
The verification shows that decodeWithCanvas is only used in one location within disk-overlay-decoder.ts, where it's called with overlayImageBlob which is already a Blob type. The test file disk-overlay-decoder.test.ts also confirms this by testing with mockBlob parameters.
app/packages/looker/src/worker/disk-overlay-decoder.ts: UsesdecodeWithCanvas(overlayImageBlob)- Tests in
disk-overlay-decoder.test.ts: Verify usage withmockBlob
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for decodeWithCanvas usage to confirm parameter changes were addressed
rg -A 5 'decodeWithCanvas'
Length of output: 5792
app/packages/looker/src/worker/painter.ts (1)
116-116: Confirm that 2-channel or true 3-channel masks won't cause issues
The code sets numChannels based on label.mask.data.channels. If a future use case emerges (e.g., 2-channel grayscale + alpha, or pure 3-channel RGB), confirm that handling is consistent throughout the loop logic.
✅ Verification successful
Let me gather more context about how the mask channels are handled in the codebase.
Channel handling is properly implemented for different scenarios
The codebase explicitly handles different channel configurations:
- 1-channel for grayscale
- 2-channel or less for standard masks
- 3+ channels for RGB masks
- 4 channels for RGBA
Key evidence:
- In
painter.ts: Special handling for >2 channels usinggetRgbFromMaskData - In
canvas-decoder.ts: Clear channel mapping (1 for grayscale, 4 for RGB/RGBA) - In
segmentation.ts: Specific logic for masks with >2 channels - Type definition in
numpy.tsproperly defines channels as a flexiblenumbertype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to 'mask.data.channels' across the codebase to see if 2 or 3 channels are explicitly handled.
rg -A 5 '(mask\.data\.channels)'
Length of output: 1009
Script:
#!/bin/bash
# Search for mask-related type definitions and channel handling
ast-grep --pattern 'interface $_ {
$$$
channels
$$$
}'
# Search for any explicit channel number checks or assignments
rg -A 3 '\b[23]\s*channels|\bchannels\s*[=><]'
Length of output: 4720
| const getPngcolorType = async (blob: Blob): Promise<number | undefined> => { | ||
| // https://www.w3.org/TR/2003/REC-PNG-20031110/#11IHDR | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add size validation before accessing PNG header data
The function reads 26 bytes from the blob without checking its size. If the blob is smaller than 26 bytes, accessing header[25] will return undefined instead of throwing, but this could lead to incorrect color type detection. Add a size check before processing:
if (blob.size < 26) {
return undefined;
}🔗 Analysis chain
Ensure robust error handling for small or invalid blobs
The function reads the first 26 bytes. If the provided blob is smaller than 26 bytes, it may cause an exception or return unexpected data from the slice. Consider adding a safety check to ensure the blob meets the minimum size.
Let me check the full implementation of getPngcolorType to see how it handles the blob size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code references to `getPngcolorType` calls to see if all call sites handle errors or short blobs.
rg -A 5 'getPngcolorType'
Length of output: 1266
Script:
#!/bin/bash
# Get the full implementation of getPngcolorType function
ast-grep --pattern 'const getPngcolorType = async (blob: Blob): Promise<number | undefined> => {
$$$
}'
Length of output: 2118
| // putImageData results in an UInt8ClampedArray, | ||
| // if image is grayscale, it'll be packed as: | ||
| // [I, I, I, I, I...], where I is the intensity value | ||
| // or else it'll be packed as: | ||
| // [R, G, B, A, R, G, B, A...] | ||
| // for non-grayscale masks, we can check every nth byte, | ||
| // where n = numChannels, to check for whether or not the pixel is part of the mask | ||
| for (let i = 0; i < targets.length; i += numChannels) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Efficient iteration for grayscale vs. multi-channel
Incrementing by numChannels ensures correct skipping for non-grayscale masks. If a partial or unexpected channel count occurs (like 2 or 3), the loop might skip relevant data. Validate you won’t encounter partial channel data from decodeWithCanvas.
| // overlay image is a Uint32Array, where each pixel is represented by 4 bytes (RGBA) | ||
| // so we need to divide by 4 to get the correct index to assign 32 bit color | ||
| const overlayIndex = i / 4; | ||
| const overlayIndex = i / numChannels; |
There was a problem hiding this comment.
Check overlay index bounds
Dividing i by numChannels calculates overlayIndex. If i + numChannels surpasses targets.length, an off-by-one or partial read could occur. Verify that the overlay array matches the target array length for any channel count.
* reset buffers to initial state, handle single frame * assert grid tagging with locators
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/packages/looker/src/lookers/video.ts (1)
365-365: Consider extracting buffer reset logicWhile the buffer reset implementation is correct, consider extracting the common buffer reset logic into a private method to avoid duplication with the
detachmethod.export class VideoLooker extends AbstractLooker<VideoState, VideoSample> { + private resetBuffers(): void { + this.state.buffers = this.initialBuffers(this.state.config); + } + detach() { this.pause(); if (LOOKER_WITH_READER === this) { clearReader(); LOOKER_WITH_READER = null; - this.state.buffers = this.initialBuffers(this.state.config); + this.resetBuffers(); } super.detach(); } postProcess(): VideoState { // ... existing code ... if (LOOKER_WITH_READER === this) { if (this.state.config.thumbnail && !this.state.hovering) { clearReader(); - this.state.buffers = this.initialBuffers(this.state.config); + this.resetBuffers(); LOOKER_WITH_READER = null; } } // ... existing code ... } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/looker/src/elements/video.ts(1 hunks)app/packages/looker/src/lookers/video.ts(2 hunks)e2e-pw/src/oss/specs/grid-tagging.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/lookers/video.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
e2e-pw/src/oss/specs/grid-tagging.spec.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/elements/video.ts (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)
app/packages/looker/src/lookers/video.ts (1)
55-55: LGTM: Improved state cleanup on detach
The addition of buffer reset logic ensures proper cleanup of the video buffer state when detaching the looker instance, preventing potential memory leaks and stale states.
app/packages/looker/src/elements/video.ts (1)
79-79: Validate the presence of multiple frames before showing the loader bar.
Displaying the loader only when “start !== end” helps avoid showing it for a single-frame video. This logic seems correct and efficiently prevents unnecessary rendering overhead.
|
|
||
| if (shown === this.shown) { | ||
| return this.element; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider re-checking the (start, end) boundary conditions.
Right now, the function immediately exits if “shown” has not changed. This could skip a re-render when “start” or “end” changes but “shown” remains the same, possibly causing the loader bar to remain stale. It may be prudent to also detect changes in these range values and trigger a re-render if they’re modified.
Possible approach:
-if (shown === this.shown) {
+const rangeChanged = start !== this.start || end !== this.end;
+if (shown === this.shown && !rangeChanged) {
return this.element;
}
...
+this.start = start;
+this.end = end;Committable suggestion skipped: line range outside the PR's diff.
| for (let i = 31; i <= 54; i++) { | ||
| const locator = grid.locator.getByText(`/tmp/${i}.png`); | ||
| await expect(locator).toBeVisible(); | ||
| await expect( | ||
| locator.locator("..").getByTestId("tag-tags-grid-test") | ||
| ).toBeVisible(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve DOM traversal reliability
The current implementation uses fragile DOM traversal with ".." and lacks clear error messages.
Consider this improvement:
- for (let i = 31; i <= 54; i++) {
+ for (let i = GRID_TEST_START_INDEX; i <= GRID_TEST_END_INDEX; i++) {
- const locator = grid.locator.getByText(`/tmp/${i}.png`);
+ const filePath = getExpectedFilePath(i);
+ const locator = grid.locator.getByText(filePath);
await expect(locator).toBeVisible();
- await expect(
- locator.locator("..").getByTestId("tag-tags-grid-test")
- ).toBeVisible();
+ // Use a more reliable selector that doesn't depend on DOM structure
+ const tagLocator = grid.locator
+ .getByTestId(`grid-item-${i}`)
+ .getByTestId("tag-tags-grid-test");
+
+ await expect(tagLocator).toBeVisible({
+ timeout: 5000,
+ message: `Tag for item ${filePath} should be visible`
+ });
}Also consider adding a helper method in the GridPom class:
class GridPom {
async verifyItemTag(index: number, tagName: string) {
const filePath = `/tmp/${index}.png`;
const gridItem = this.locator.getByText(filePath);
await expect(gridItem).toBeVisible();
const tag = this.locator
.getByTestId(`grid-item-${index}`)
.getByTestId(`tag-tags-${tagName}`);
await expect(tag).toBeVisible();
}
}| for (let i = 31; i <= 54; i++) { | ||
| const locator = grid.locator.getByText(`/tmp/${i}.png`); | ||
| await expect(locator).toBeVisible(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test robustness
The current implementation has several potential issues:
- Magic numbers (31-54) make the test's intent unclear
- No custom error messages in expect statements
- Potential flakiness if grid layout changes
Consider this improvement:
+ // Define constants at the top of the file
+ const GRID_TEST_START_INDEX = 31;
+ const GRID_TEST_END_INDEX = 54;
+ const getExpectedFilePath = (i: number) => `/tmp/${i}.png`;
- for (let i = 31; i <= 54; i++) {
+ // Verify initial grid items visibility
+ for (let i = GRID_TEST_START_INDEX; i <= GRID_TEST_END_INDEX; i++) {
- const locator = grid.locator.getByText(`/tmp/${i}.png`);
+ const filePath = getExpectedFilePath(i);
+ const locator = grid.locator.getByText(filePath);
- await expect(locator).toBeVisible();
+ await expect(locator).toBeVisible({
+ timeout: 5000,
+ message: `Grid item ${filePath} should be visible`
+ });
}📝 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.
| for (let i = 31; i <= 54; i++) { | |
| const locator = grid.locator.getByText(`/tmp/${i}.png`); | |
| await expect(locator).toBeVisible(); | |
| } | |
| // Define constants at the top of the file | |
| const GRID_TEST_START_INDEX = 31; | |
| const GRID_TEST_END_INDEX = 54; | |
| const getExpectedFilePath = (i: number) => `/tmp/${i}.png`; | |
| // Verify initial grid items visibility | |
| for (let i = GRID_TEST_START_INDEX; i <= GRID_TEST_END_INDEX; i++) { | |
| const filePath = getExpectedFilePath(i); | |
| const locator = grid.locator.getByText(filePath); | |
| await expect(locator).toBeVisible({ | |
| timeout: 5000, | |
| message: `Grid item ${filePath} should be visible` | |
| }); | |
| } |
* update font size when pulling from looker cache * add fontSize to attach
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/looker/src/lookers/abstract.ts (1)
524-559: Consider adding explicit error handling for buffer collectionWhile the implementation handles both modern and legacy browsers well, consider adding explicit error handling for edge cases.
const arrayBuffers: ArrayBuffer[] = []; +try { for (const overlay of this.pluckedOverlays ?? []) { let overlayData: LabelMask = null; // ... existing code ... } +} catch (error) { + console.warn('Failed to collect array buffers:', error); + // Continue with empty array buffers +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/core/src/components/Grid/Grid.tsx(1 hunks)app/packages/core/src/components/ImageContainerHeader.tsx(1 hunks)app/packages/looker/src/lookers/abstract.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/core/src/components/ImageContainerHeader.tsx
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/components/Grid/Grid.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.
app/packages/looker/src/lookers/abstract.ts (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 (6)
app/packages/core/src/components/Grid/Grid.tsx (2)
75-78: LGTM: Font size parameter correctly passed to attach method
The implementation properly forwards the font size to the looker instance.
Line range hint 482-483: LGTM: State updates properly handle font size
The implementation correctly updates both the disabled state and font size options in a single state update.
Also applies to: 499-499
app/packages/looker/src/lookers/abstract.ts (4)
472-476: LGTM: Method signature properly updated
The attach method signature is correctly updated to include the optional fontSize parameter.
744-748: Check if cleanup method exists before calling
The cleanup method on overlay is optional in the Overlay interface.
787-798: Type the error in catch block
The error in catch clause should be typed for better TypeScript safety.
757-759: LGTM: Good documentation and memory management
The implementation properly documents the rationale for cleanup and helps prevent memory leaks from dangling ImageBitmaps.
Merge
release/v1.2.0todevelopSummary by CodeRabbit
New Features
Errorcomponent to display evaluation-related error messages.ErrorIconcomponent for visual representation of error states.Bug Fixes
Documentation
Style
Chores
fiftyone-brainin setup configuration.