add support for collection overlays #5289
Conversation
WalkthroughThe pull request introduces modifications to two files: Changes
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
|
sashankaryal
changed the title
![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/looker/src/worker/disk-overlay-decoder.ts(2 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 (10)
fiftyone/server/metadata.py (7)
15-15: Ensure 'pydash' is included in project dependencies
The pydash library is being imported. Please verify that it is listed in your project's dependencies (e.g., in setup.py or requirements.txt) to avoid import errors.
35-36: Addition of 'mask_path' for Detection classes is correct
The 'mask_path' field is appropriately added for fol.Detection and fol.Detections in _ADDITIONAL_MEDIA_FIELDS, enabling proper handling of mask paths for detection labels.
74-78: Unpacking additional fields from _get_additional_media_fields
The unpacking of opm_field, detections_fields, and additional_fields from _get_additional_media_fields(collection) aligns with the updated return values of the function.
400-400: Update function signature to include detections_fields
The addition of the detections_fields parameter to _create_media_urls ensures that detection masks are processed correctly.
408-424: Properly handle detection masks in _create_media_urls
The added code correctly iterates over detection fields to include individual detection mask paths in media_fields. This allows for accurate retrieval of mask paths for each detection object.
442-442: Use pydash.get for nested field access
Using get(sample, field) from pydash facilitates safe retrieval of nested fields, which is beneficial for handling dynamic or deeply nested sample structures.
Line range hint 464-487: Correctly collect detections_fields in _get_additional_media_fields
The function now separates detections_fields from other additional fields, allowing specialized processing of detection masks. This enhances the modularity and readability of the code.
app/packages/looker/src/worker/disk-overlay-decoder.ts (3)
28-31: Add optional parameters maskTargetsBuffers and overlayCollectionProcessingParams
Including maskTargetsBuffers and overlayCollectionProcessingParams as optional parameters in decodeOverlayOnDisk increases the function's flexibility to handle overlay collections effectively.
34-50: Process detections within DETECTIONS labels correctly
The updated logic correctly handles the DETECTIONS class by iterating over each detection and recursively calling decodeOverlayOnDisk with appropriate parameters. This ensures that each detection's overlay is processed individually.
85-93: Safely construct source for overlay images
The conditional construction of source using overlayCollectionProcessingParams correctly accesses the specific overlay path for each detection. Ensure that the key exists in sources to prevent undefined values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/server/metadata.py (2)
408-424: Consider optimizing the detections processing logicThe current implementation could be improved in several ways:
- Nested get calls could be simplified
- The loop could be optimized for better performance
- Error handling could be enhanced
Consider this optimization:
if detections_fields is not None: for field in detections_fields: - detections = get(sample, field) - - if not detections: - continue - - detections_list = get(detections, "detections") - - if not detections_list or len(detections_list) == 0: - continue - - len_detections = len(detections_list) - - for i in range(len_detections): - media_fields.append(f"{field}.detections[{i}].mask_path") + detections_list = get(sample, f"{field}.detections", []) + media_fields.extend( + f"{field}.detections[{i}].mask_path" + for i in range(len(detections_list)) + )This optimization:
- Reduces nested get calls to a single call with default value
- Uses list comprehension for better performance
- Handles missing or malformed data gracefully through the default value
Line range hint
467-490: Consider adding type hints and updating docstringThe function implementation looks good, but could benefit from better documentation:
- Add return type hints:
def _get_additional_media_fields( collection: SampleCollection, -) -> t.List[str]: +) -> t.Tuple[t.Optional[str], t.Optional[t.List[str]], t.List[str]]:
- Add docstring explaining the return values:
""" Get additional media fields from the collection. Args: collection: A SampleCollection instance Returns: A tuple containing: - opm_field: Optional field name for orthographic projection metadata - detections_fields: Optional list of detection field names - additional: List of additional media field paths """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/server/metadata.py(8 hunks)
🔇 Additional comments (2)
fiftyone/server/metadata.py (2)
15-15: LGTM! Good choice using pydash.get for safe nested dictionary access
The addition of Detection and Detections types to _ADDITIONAL_MEDIA_FIELDS aligns well with the PR objective to support collection overlays. Using pydash.get is a safer approach for accessing nested dictionary values.
Also applies to: 35-36
74-78: LGTM! Clean implementation of detection fields support
The changes maintain backward compatibility while elegantly adding support for detection fields through proper tuple unpacking and parameter passing.
Also applies to: 87-87
| // example: for detections, we need to access the source from the parent label | ||
| // like: if field is "prediction_masks", we're trying to get "predictiion_masks.detections[INDEX].mask" | ||
| source = | ||
| sources[ |
There was a problem hiding this comment.
any concern that the key we're constructing here doesn't exist on sources?
There was a problem hiding this comment.
in which case it'd return undefined. In fact, it won't exist in OSS
a = {}
assert typeof a['foo.bar'] === 'undefined';There was a problem hiding this comment.
my eyes told me the ]. deref was outside the key so thought we might be derefing an undefined. my mistake 🙏
We don't have a precedent for
sourcesin labels accounting for collection label types likeDetections. This PR adds support for that.Summary by CodeRabbit
New Features
Bug Fixes
Refactor