add 16 bit png support #5413
add 16 bit png support #5413
Conversation
|
Warning Rate limit exceeded@sashankaryal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces significant modifications to image decoding and processing in the FiftyOne project. The changes primarily focus on simplifying PNG image handling, introducing a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant MaskDecoder
participant CustomPngDecoder
participant CanvasDecoder
Caller->>MaskDecoder: decodeMaskOnDisk(blob, cls)
MaskDecoder->>MaskDecoder: Check image type
alt JPEG/JPG
MaskDecoder->>CanvasDecoder: decodeWithCanvas
else PNG
MaskDecoder->>MaskDecoder: Get PNG header
alt 16-bit PNG
MaskDecoder->>CustomPngDecoder: customDecode16BitPng
else Other PNG
MaskDecoder->>CanvasDecoder: decodeWithCanvas
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (7)
fiftyone/utils/eval/base.py (1)
Line range hint
219-283: Documentation could be more explicit about default behaviorThe implementation looks good, but consider making the docstring more explicit about what "only include a row/column for missing labels if there are any" means in practice.
Add this clarification to the docstring:
include_missing (None): whether to include a row/column for missing ground truth/predictions in the confusion matrix. The supported values are: - None (default): only include a row/column for missing - labels if there are any + labels if there are any unmatched ground truth or predicted + labels in the evaluation results - True: do include a row/column for missing labels - False: do not include a row/column for missing labelsapp/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)
76-79: Consider a more robust options merging strategy.The current spread operator usage (
...executorOptions, ...options) might accidentally override important options. Consider implementing a deep merge or explicitly defining which options can be overridden.- const resolvedOptions = { - ...executorOptions, - ...options, - }; + const resolvedOptions = { + ...executorOptions, + ...(options ?? {}), + // Explicitly list options that should be overrideable + target: options?.target ?? executorOptions?.target, + // Add other specific options here + };app/packages/looker/src/worker/custom-16-bit-png-decoder.ts (1)
16-17: Document channel calculation fallbackThe fallback calculation for channels should be documented to explain why and when it's used.
const numChannels = - decodedPng.channels ?? decodedPng.data.length / (width * height); + // If channels is not specified, calculate from data length + // For 16-bit PNGs: 1 channel = grayscale, 2 = gray+alpha, 3 = RGB, 4 = RGBA + decodedPng.channels ?? decodedPng.data.length / (width * height);app/packages/operators/src/usePanelEvent.ts (1)
Line range hint
37-52: Improve error handling in eventCallbackThe error handling could be more robust:
- The error message concatenation might result in undefined when no operator is provided
- The error logging could include more context
const eventCallback = (result, opts) => { decrement(panelId); - const msg = - result.errorMessage || result.error || "Failed to execute operation"; - const computedMsg = `${msg} (operation: ${operator})`; + const msg = result.errorMessage || result.error || "Failed to execute operation"; + const computedMsg = `${msg} (operation: ${operator || "unknown"})`; if (result?.error) { notify({ msg: computedMsg, variant: "error" }); - console.error(result?.error); + console.error("Operation failed:", { + error: result.error, + operator, + params: actualParams + }); } options?.callback?.(result, opts); };app/packages/core/src/components/Modal/ModalLooker.tsx (1)
68-71: Enhance key prop usage for consistencyConsider:
- Making the key more specific to avoid potential conflicts
- Adding the same key to VideoLookerReact for consistent behavior
- const modalMediaField = useRecoilValue(fos.selectedMediaField(true)); + const modalMediaField = useRecoilValue(fos.selectedMediaField(true)); + const componentKey = `modal-${modalMediaField}`; if (shouldRenderImavid) { - return <ImaVidLookerReact sample={sample} key={modalMediaField} />; + return <ImaVidLookerReact sample={sample} key={componentKey} />; } if (video) { - return <VideoLookerReact sample={sample} />; + return <VideoLookerReact sample={sample} key={componentKey} />; }app/packages/state/src/hooks/useCreateLooker.ts (1)
227-230: Improve key composition for better maintainability.The composite key creation can be moved to a utility function for better maintainability and reuse. This would also help ensure consistent key generation across the codebase.
Consider applying this change:
-const imavidPartitionKey = `${thisSampleId}-${mediaField}`; +const createImavidPartitionKey = (sampleId: string, mediaField: string) => `${sampleId}-${mediaField}`; +const imavidPartitionKey = createImavidPartitionKey(thisSampleId, mediaField);app/packages/operators/src/built-in-operators.ts (1)
Line range hint
1048-1074: LGTM! The callback handling has been simplified.The removal of the
on_cancelcallback and the refactoring of the success/error handling logic makes the code cleaner and more maintainable.Consider adding TypeScript type annotations for the
paramsobject to improve type safety:- const { params, operator_uri, on_success, on_error } = ctx.params; + const { + params, + operator_uri, + on_success, + on_error + }: { + params: Record<string, unknown>; + operator_uri: string; + on_success?: string; + on_error?: string; + } = ctx.params;
🛑 Comments failed to post (1)
app/packages/core/src/components/Modal/ImaVidLooker.tsx (1)
198-198:
⚠️ Potential issuePotential memory leak: Event listener cleanup needed.
Removing
{ once: true }means the event listener will persist. This could lead to memory leaks if the event listeners accumulate over time. Either:
- Re-add
{ once: true }, or- Ensure proper cleanup in component unmount
Apply one of these changes:
# Option 1: Re-add once: true -fetchMoreListener as EventListener +fetchMoreListener as EventListener, +{ once: true } # Option 2: Add cleanup +useEffect(() => { + return () => { + window.removeEventListener("fetchMore", fetchMoreListener as EventListener); + }; +}, []);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
Line range hint
109-115: Improve error handling during mask decodingThe
try...catchblock currently logs the error but does not provide context. Enhance the error message for better debugging and consider rethrowing the error if appropriate.Apply this diff to improve error handling:
try { overlayMask = await decodeMaskOnDisk(overlayImageBlob, cls); } catch (e) { - console.error(e); + console.error(`Error decoding overlay mask for class ${cls}:`, e); return; }app/packages/looker/src/worker/mask-decoder.test.ts (4)
6-16: Use more dynamic mock implementations for better test coverageThe current mock for
decodeWithCanvasreturns fixed values. Consider making the mock return values based on input parameters to simulate different scenarios and enhance test reliability.Example modification:
vi.mock("./canvas-decoder", () => { return { decodeWithCanvas: vi.fn().mockImplementation(async (blob, cls, channels) => ({ // Return dynamic fake decoded info based on input buffer: new ArrayBuffer(channels * 4), channels, arrayType: "Uint8Array", shape: [2, 2], })), }; });
51-66: Consider using standard testing utilities for Blob mockingManually mocking Blob methods increases code complexity and maintenance burden. Utilize standard testing libraries like
blob-polyfillor utilities from the testing framework to create Blob objects, improving readability and reliability.
90-91: Remove unused import ofdecodeWithCanvasThe import of
decodeWithCanvasis redundant in the test file as it's being mocked and not directly used. This can help clean up the code.Apply this diff to remove the unused import:
-import { decodeWithCanvas } from "./canvas-decoder";
122-139: Expand test cases to cover additional PNG formatsCurrently, tests cover 16-bit grayscale and 8-bit truecolor PNGs. To ensure comprehensive coverage, add test cases for other color types, such as indexed-color (colorType 3) and grayscale with alpha (colorType 4), to verify the decoder's behavior across various formats.
app/packages/looker/src/worker/canvas-decoder.ts (2)
41-45: Add TypeScript validation and documentation for numOriginalChannels parameter.The new parameter lacks type constraints and documentation. Consider adding:
- Type validation to ensure valid channel counts (1-4)
- JSDoc documentation explaining the parameter's purpose and valid values
+/** + * Decodes an image blob using canvas operations + * @param blob - The image blob to decode + * @param cls - The class type of the image + * @param numOriginalChannels - Number of channels in the original image (1-4) + */ export const decodeWithCanvas = async ( blob: Blob, cls: string, - numOriginalChannels: number + numOriginalChannels: 1 | 2 | 3 | 4 ) => {
Line range hint
48-57: Ensure proper cleanup of resources.The ImageBitmap is properly closed, but consider wrapping the canvas operations in a try-finally block to ensure cleanup in case of errors.
- const imageBitmap = await createImageBitmap(blob); - const { width, height } = imageBitmap; - - const { canvas: offScreenCanvas, ctx: offScreenCanvasCtx } = canvasAndCtx!; - - offScreenCanvas.width = width; - offScreenCanvas.height = height; - - offScreenCanvasCtx.drawImage(imageBitmap, 0, 0); - - imageBitmap.close(); + let imageBitmap; + try { + imageBitmap = await createImageBitmap(blob); + const { width, height } = imageBitmap; + + const { canvas: offScreenCanvas, ctx: offScreenCanvasCtx } = canvasAndCtx!; + + offScreenCanvas.width = width; + offScreenCanvas.height = height; + + offScreenCanvasCtx.drawImage(imageBitmap, 0, 0); + } finally { + imageBitmap?.close(); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/mask-decoder.test.ts(1 hunks)app/packages/looker/src/worker/mask-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/looker/src/worker/mask-decoder.test.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/mask-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/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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: e2e / test-e2e
🔇 Additional comments (2)
app/packages/looker/src/worker/mask-decoder.ts (1)
17-31: Verify the byte offsets forbitDepthandcolorTypeThe offsets used to access
bitDepthandcolorTypein the header may be incorrect due to zero-based indexing in arrays. Confirm that the indices24and25correctly correspond to thebitDepthandcolorTypebytes in the PNG header.To ensure accurate parsing, double-check the offsets against the PNG specification.
app/packages/looker/src/worker/canvas-decoder.ts (1)
Line range hint
59-83: Optimize buffer reallocation for performance.The current implementation might perform unnecessary buffer reallocations. Consider:
- Pre-allocating the buffer with the correct size
- Adding early returns for optimization
Also, verify that this change properly handles 16-bit PNGs as intended by the PR.
✅ Verification successful
Current implementation is already well-optimized
The code already implements efficient buffer handling:
- Buffers are pre-allocated with correct sizes
- Reallocation only occurs when format conversion is necessary (grayscale or heatmap)
- Proper support for different PNG bit depths (1, 2, 4, 8) is implemented in the indexed-png-decoder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests or examples using 16-bit PNGs rg -l "\.png" --glob "**/*test*" | while read -r file; do echo "=== $file ===" rg "16.*bit|uint16" "$file" doneLength of output: 3702
Script:
#!/bin/bash # Find recastBufferToMonoChannel implementation and PNG-related tests echo "=== recastBufferToMonoChannel implementation ===" ast-grep --pattern 'function recastBufferToMonoChannel' echo -e "\n=== PNG-related tests in looker package ===" fd -e test.ts -e spec.ts . app/packages/looker | while read -r file; do echo "=== $file ===" rg -A 5 "\.png|ImageData|canvas" "$file" done echo -e "\n=== Buffer allocation patterns ===" rg -A 3 "new (Uint8Array|Uint16Array|Float32Array|Float64Array)" app/packages/lookerLength of output: 13348
sashankaryal
force-pushed
the
fix/16-bit-png
branch
from
5487025 to
a1ac532
Compare
good suggestion. done |
sashankaryal
force-pushed
the
fix/16-bit-png
branch
from
a1ac532 to
968d719
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/packages/looker/src/worker/canvas-decoder.ts (1)
Line range hint
41-69: Consider adding TypeDoc comments for the updated function signatureThe function's signature has changed significantly, but the documentation hasn't been updated to reflect the new parameter and its requirements.
+/** + * Decodes an image blob using canvas. + * @param blob - The image blob to decode + * @param cls - The class type of the image + * @param numOriginalChannels - Number of channels in the original image (1-4) + * @returns Promise<OverlayMask> - The decoded image data + * @throws {Error} When numOriginalChannels is invalid + */ export const decodeWithCanvas = async (app/packages/looker/src/worker/mask-decoder.ts (2)
38-44: Enhance error handling for PNG header parsingWhile the code handles invalid PNG signatures, it could benefit from more detailed error logging.
if (blob.type === "image/png") { const headerInfo = await getPngHeader(blob); if (!headerInfo) { - console.warn("blob type is image/png but not a PNG file"); + console.warn( + "Invalid PNG: blob has image/png MIME type but invalid PNG signature", + { blobSize: blob.size } + ); return decodeWithCanvas(blob, cls, channels); }
54-59: Consider using an enum for PNG color typesThe color type values and their meanings should be defined as an enum for better maintainability and type safety.
+export enum PngColorType { + Grayscale = 0, + Truecolor = 2, + IndexedColor = 3, + GrayscaleAlpha = 4, + RGBA = 6, +} // according to PNG specs: // 0: Grayscale => 1 channel
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (5)
app/packages/looker/src/worker/canvas-decoder.ts(1 hunks)app/packages/looker/src/worker/custom-16-bit-png-decoder.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(2 hunks)app/packages/looker/src/worker/mask-decoder.test.ts(1 hunks)app/packages/looker/src/worker/mask-decoder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/packages/looker/src/worker/custom-16-bit-png-decoder.ts
- app/packages/looker/src/worker/mask-decoder.test.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/worker/mask-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/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 (1)
app/packages/looker/src/worker/canvas-decoder.ts (1)
41-46: Add input validation for numOriginalChannels parameterThe function should validate the input parameter to ensure robust error handling.
export const decodeWithCanvas = async ( blob: Blob, cls: string, numOriginalChannels: number ) => { + if (numOriginalChannels < 1 || numOriginalChannels > 4) { + throw new Error(`Invalid number of channels: ${numOriginalChannels}. Expected 1-4.`); + } let channels: number = numOriginalChannels;
sashankaryal
force-pushed
the
fix/16-bit-png
branch
from
22333bc to
dff2887
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/packages/looker/src/worker/canvas-decoder.ts (1)
Line range hint
48-52: Add error handling for canvas operations.The canvas operations (createImageBitmap, drawImage, getImageData) could fail and should be wrapped in try-catch blocks.
- const imageBitmap = await createImageBitmap(blob); - const { width, height } = imageBitmap; - - const { canvas: offScreenCanvas, ctx: offScreenCanvasCtx } = canvasAndCtx!; + try { + const imageBitmap = await createImageBitmap(blob); + const { width, height } = imageBitmap; + + const { canvas: offScreenCanvas, ctx: offScreenCanvasCtx } = canvasAndCtx!;
🧹 Nitpick comments (19)
app/packages/state/src/hooks/useCreateLooker.ts (1)
228-230: Consider adding error handling for store operations.While the store operations are correctly implemented, consider adding error handling for edge cases where store operations might fail.
if (!ImaVidFramesControllerStore.has(imavidPartitionKey)) { + try { ImaVidFramesControllerStore.set( imavidPartitionKey, new ImaVidFramesController({ environment, firstFrameNumber, page, targetFrameRate: dynamicGroupsTargetFrameRateValue, totalFrameCountPromise, key: imavidKey, }) ); + } catch (error) { + console.error(`Failed to create controller for ${imavidPartitionKey}:`, error); + throw new Error(`Failed to initialize frame controller: ${error.message}`); + } }app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
Line range hint
10-23: Documentation could be enhanced with more detailed prop descriptions.Consider adding more detailed JSDoc comments for each prop, especially for
executionParamsandonOptionSelected, to better explain their purpose and expected values.app/packages/operators/src/usePanelEvent.ts (2)
Line range hint
26-33: Error handling could be more specific.The generic error message could be enhanced to provide more context about why the operator was not provided. Consider adding error codes or more specific error messages.
- notify({ - msg: "No operator provided for panel event.", - variant: "error", - }); - return console.error("No operator provided for panel event."); + const errorMsg = "No operator provided for panel event. Please ensure an operator is specified in the panel configuration."; + notify({ + msg: errorMsg, + variant: "error", + code: "OPERATOR_MISSING" + }); + return console.error(errorMsg);
Line range hint
8-13: Consider strengthening type safety for HandlerOptions.The
anytype forcurrentPanelStatecould be replaced with a more specific type to improve type safety.- currentPanelState?: any; // most current panel state + currentPanelState?: Record<string, unknown>; // most current panel stateapp/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)
74-84: Consider dependency array optimization.The useCallback dependency array could be optimized by destructuring executionParams earlier.
+ const params = executionParams ?? {}; const onExecute = useCallback( (options?: OperatorExecutorOptions) => { const resolvedOptions = { ...executorOptions, ...options, }; - return operator.execute(executionParams ?? {}, resolvedOptions); + return operator.execute(params, resolvedOptions); }, - [executorOptions, operator, executionParams] + [executorOptions, operator, params] );app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (2)
Line range hint
22-107: Consider breaking down the component for better maintainability.The component is handling too many responsibilities. Consider extracting the event handlers and style logic into separate custom hooks or components.
Example structure:
// useOperatorEventHandlers.ts export function useOperatorEventHandlers(props) { const triggerEvent = usePanelEvent(); const panelId = usePanelId(); return { handleOnSuccess: useCallback(...), handleOnError: useCallback(...), handleOnOptionSelected: useCallback(...) }; } // OperatorExecutionButtonView.tsx export default function OperatorExecutionButtonView(props: ViewPropsType) { const eventHandlers = useOperatorEventHandlers(props); // ... rest of the component }
Line range hint
111-196: Move style-related functions to a separate file.The style utility functions (
getButtonProps,getIconProps,getCommonProps,getColor) should be moved to a separate file to improve code organization and reusability.Create a new file
buttonStyles.ts:export function getButtonProps(props: ViewPropsType): ButtonProps { // ... existing implementation } export function getIconProps(props: ViewPropsType): ButtonProps { // ... existing implementation } // ... other style-related functionsapp/packages/core/src/plugins/SchemaIO/index.tsx (1)
23-30: LGTM! Consider adding type annotations.The changes improve value handling by ensuring consistent coercion before state updates. The implementation correctly handles state updates and propagates the coerced value to callbacks.
Consider adding TypeScript type annotations for better type safety:
- const computedValue = coerceValue(value, schema); + const computedValue: unknown = coerceValue(value, schema);app/packages/core/src/plugins/SchemaIO/components/DynamicIO.tsx (1)
39-44: LGTM! Consider improving readability.The changes improve schema handling by providing a more robust fallback mechanism.
Consider improving readability by using a separate variable for the schema resolution:
+ const resolvedSchema = schema ?? subSchema ?? computedSchema; onChange( path, value, - schema ?? subSchema ?? computedSchema, + resolvedSchema, computedAncestors );fiftyone/utils/transformers.py (1)
589-591: LGTM! Consistent device handling across model types.The device handling is implemented consistently across different model types and operations. However, there's an opportunity to reduce code duplication.
Consider extracting the common device handling pattern into a helper method to reduce code duplication. For example:
def _load_model_to_device(self, model_cls, config): """Helper to load a model and move it to the correct device.""" device = torch.device(config.device) return model_cls.from_pretrained(config.name_or_path).to(device)This could then be used across different model loading methods:
def _load_model(self, config): if config.model is not None: return config.model return self._load_model_to_device( transformers.AutoModelForImageClassification, config )Also applies to: 649-649, 701-704, 708-708, 756-757, 766-766, 777-779, 790-790, 832-832, 886-886, 938-938, 1090-1090
fiftyone/core/view.py (2)
442-451: Improve exception handling in first() method.While the implementation is efficient, the exception handling should be improved to maintain the exception chain.
Apply this change to properly chain exceptions:
try: return next(iter(self)) except StopIteration: - raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
451-451: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
489-553: LGTM with minor improvements needed.The
one()method is well-implemented with comprehensive documentation and efficient querying. However, the exception handling should be improved.Apply these changes to properly chain exceptions:
try: d = next(matches) except StopIteration: - raise ValueError("No samples match the given expression") + raise ValueError("No samples match the given expression") from None🧰 Tools
🪛 Ruff (0.8.2)
542-542: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/unittests/dataset_tests.py (2)
971-1018: Well structured test for first/last/head/tail dataset operations.The test provides good coverage of basic functionality but could benefit from additional assertions:
- Verify that head() and tail() return exactly 3 samples by default
- Add test cases for custom head/tail sizes
- Consider testing edge cases like head/tail sizes larger than dataset size
def test_first_last_head_tail(self): dataset = fo.Dataset() dataset.add_samples( [ fo.Sample(filepath="image%d.jpg" % i, index=i) for i in range(1, 52) ] ) # Test default behavior self.assertEqual(len(dataset.head()), 3) self.assertEqual(len(dataset.tail()), 3) # Test custom sizes self.assertEqual(len(dataset.head(5)), 5) self.assertEqual(len(dataset.tail(5)), 5) # Test oversized requests self.assertEqual(len(dataset.head(100)), len(dataset)) self.assertEqual(len(dataset.tail(100)), len(dataset)) # Existing assertions...
1019-1048: Good error handling test for empty dataset operations.The test thoroughly verifies error handling for empty datasets. Consider adding:
- Test cases for head/tail with non-default sizes on empty dataset
- Assertions to verify the specific error message content
def test_first_last_head_tail_empty(self): dataset = fo.Dataset() with self.assertRaises(ValueError) as cm: _ = dataset.first() self.assertIn("empty dataset", str(cm.exception)) # Test custom sizes self.assertListEqual(dataset.head(5), []) self.assertListEqual(dataset.tail(5), []) # Existing assertions...fiftyone/core/dataset.py (2)
1236-1243: Improve error handling in empty dataset case.Use
raise ... from Noneto properly chain exceptions when the dataset is empty.try: d = next(cursor) except StopIteration: - raise ValueError("%s is empty" % self.__class__.__name__) + raise ValueError("%s is empty" % self.__class__.__name__) from None🧰 Tools
🪛 Ruff (0.8.2)
1241-1241: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Line range hint
1337-1350: Improve error handling in match cases.The implementation efficiently checks for multiple matches using limit=2. Consider improving exception chaining:
try: d = next(matches) except StopIteration: - raise ValueError("No samples match the given expression") + raise ValueError("No samples match the given expression") from Nonefiftyone/core/collections.py (1)
10575-10590: Simplify boolean expressionThe boolean expression can be simplified by returning the condition directly.
- if ( - isinstance(self, fov.DatasetView) - and self._dataset.media_type == fom.GROUP - and len(self._stages) == 1 - and isinstance(self._stages[0], fos.SelectGroupSlices) - and self._pipeline() == [] - ): - return True - - return False + return ( + isinstance(self, fov.DatasetView) + and self._dataset.media_type == fom.GROUP + and len(self._stages) == 1 + and isinstance(self._stages[0], fos.SelectGroupSlices) + and self._pipeline() == [] + )🧰 Tools
🪛 Ruff (0.8.2)
10580-10589: Return the condition directly
Inline condition
(SIM103)
app/packages/looker/src/worker/canvas-decoder.ts (1)
Line range hint
54-57: Ensure proper cleanup of imageBitmap.The imageBitmap.close() call should be in a finally block to ensure cleanup even if subsequent operations fail.
- offScreenCanvas.width = width; - offScreenCanvas.height = height; - - offScreenCanvasCtx.drawImage(imageBitmap, 0, 0); - - imageBitmap.close(); + try { + offScreenCanvas.width = width; + offScreenCanvas.height = height; + offScreenCanvasCtx.drawImage(imageBitmap, 0, 0); + } finally { + imageBitmap.close(); + }app/packages/looker/src/worker/mask-decoder.ts (1)
54-64: Document channel mapping in JSDoc format.Add detailed JSDoc comments explaining the channel mapping for each PNG color type.
+ /** + * Channel mapping according to PNG specification: + * - Type 0: Grayscale (1 channel) + * - Type 2: RGB (3 channels, using 4 for compatibility) + * - Type 3: Indexed (palette-based, using 4 channels) + * - Type 4: Grayscale+Alpha (using 1 channel) + * - Type 6: RGBA (4 channels) + */ if (colorType === 0 || colorType === 4) { channels = 1; } else {
🛑 Comments failed to post (2)
app/packages/state/src/hooks/useCreateLooker.ts (1)
244-245:
⚠️ Potential issueAdd null check for controller retrieval.
The current implementation assumes the controller always exists. Add a null check to handle cases where the controller might be missing.
- frameStoreController: - ImaVidFramesControllerStore.get(imavidPartitionKey), + frameStoreController: (() => { + const controller = ImaVidFramesControllerStore.get(imavidPartitionKey); + if (!controller) { + throw new Error(`Frame controller not found for key: ${imavidPartitionKey}`); + } + return controller; + })(),📝 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.frameStoreController: (() => { const controller = ImaVidFramesControllerStore.get(imavidPartitionKey); if (!controller) { throw new Error(`Frame controller not found for key: ${imavidPartitionKey}`); } return controller; })(),fiftyone/utils/open_clip.py (1)
61-61:
⚠️ Potential issueBreaking change: Renamed preprocess attribute.
The attribute has been renamed from
preprocessto_preprocess_aux. While the new name is more descriptive, the change from public to private attribute (with underscore prefix) might break external code.Please ensure this breaking change is documented in the changelog and consider whether it should remain public for backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/packages/looker/src/worker/mask-decoder.ts (3)
10-34: 🛠️ Refactor suggestionAdd comprehensive error handling to PNG header parsing
The function should handle potential errors during blob reading and array buffer conversion.
const getMaybePngHeader = async ( blob: Blob ): Promise<{ bitDepth: number; colorType: number } | undefined> => { + try { + if (blob.size < 29) { + console.warn("PNG file too small to contain valid header"); + return undefined; + } + const header = new Uint8Array(await blob.slice(0, 29).arrayBuffer()); // check PNG signature for (let i = 0; i < PNG_SIGNATURE.length; i++) { if (header[i] !== PNG_SIGNATURE[i]) { - // not a PNG return undefined; } } const bitDepth = header[24]; const colorType = header[25]; + + // Validate bit depth and color type + if (![1, 2, 4, 8, 16].includes(bitDepth)) { + console.warn(`Invalid bit depth: ${bitDepth}`); + return undefined; + } + return { bitDepth, colorType }; + } catch (error) { + console.error("Error reading PNG header:", error); + return undefined; + } };
49-53: 🛠️ Refactor suggestionImprove error handling for 16-bit PNG decoding
The customDecode16BitPng call needs proper error handling.
if (colorType !== undefined) { if (bitDepth === 16) { // browser doesn't natively parse 16 bit pngs, we'll use a 3pp library - return customDecode16BitPng(blob); + try { + return await customDecode16BitPng(blob); + } catch (error) { + console.error("Failed to decode 16-bit PNG:", error); + return decodeWithCanvas(blob, cls, channels); + } }
61-65: 🛠️ Refactor suggestionAdd validation for supported color types
The code should explicitly validate supported color types.
+ const validColorTypes = [0, 2, 3, 4, 6]; + if (!validColorTypes.includes(colorType)) { + console.warn(`Unsupported PNG color type: ${colorType}`); + return decodeWithCanvas(blob, cls, channels); + } + if (colorType === 0 || colorType === 4) { channels = 1; } else { channels = 4; }
🧹 Nitpick comments (2)
app/packages/looker/src/worker/mask-decoder.ts (2)
1-4: Consider adding type safety to PNG_SIGNATURE constantThe PNG signature constant could benefit from explicit typing for better maintainability and type safety.
-const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; +const PNG_SIGNATURE: readonly number[] = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a] as const;
55-66: Document channel selection logic with JSDocThe channel selection logic should be documented for better maintainability.
+ /** + * Channel selection based on PNG color type: + * - Type 0 (Grayscale): 1 channel + * - Type 2 (RGB): 4 channels (with alpha) + * - Type 3 (Indexed): 4 channels + * - Type 4 (Grayscale+Alpha): 1 channel + * - Type 6 (RGBA): 4 channels + */ if (colorType === 0 || colorType === 4) { channels = 1; } else { channels = 4; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/looker/src/worker/mask-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/mask-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.
addresses #5363
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance
Technical Improvements