add support for detection.mask_path #5120
Conversation
WalkthroughThe changes introduced in this pull request enhance the handling of mask data within the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (5)fiftyone/core/labels.py (5)
The Line range hint The class documentation clearly explains the new
The code correctly uses the new 🧰 Tools🪛 Ruff654-654: No explicit (B028) Line range hint While the implementation looks solid, ensure comprehensive test coverage for the new mask functionality, including:
Run the following script to check test coverage: ✅ Verification successfulLet me gather more information about the test coverage by checking the actual test implementations and any related mask functionality in the codebase. Test coverage for mask functionality appears comprehensive The codebase shows thorough test coverage for mask-related functionality including:
The test suite adequately covers:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check test coverage for mask-related functionality
# Look for test files
echo "Searching for test files..."
fd -e py test_ --exec rg -l "mask_path|has_mask|import_mask|export_mask" {}
# Look for specific test cases
echo "Searching for mask-related test cases..."
rg -A 5 "def test.*mask"
Length of output: 1088 Script: #!/bin/bash
# Check the implementation of mask-related tests
echo "Checking mask test implementations..."
rg -B 2 -A 10 "test.*mask" tests/unittests/label_tests.py tests/unittests/dataset_tests.py
# Look for mask-related functionality in core code
echo -e "\nChecking core mask functionality..."
rg -l "mask" fiftyone/core/
# Check for mask-related assertions and test utilities
echo -e "\nChecking test assertions and utilities..."
rg "assert.*mask" tests/
Length of output: 8182
Add error handling for mask operations The mask import/export methods should handle potential I/O errors gracefully. Apply this diff to improve error handling: def import_mask(self, update=False):
if self.mask_path is not None:
- self.mask = _read_mask(self.mask_path)
+ try:
+ self.mask = _read_mask(self.mask_path)
+ except (IOError, OSError) as e:
+ raise IOError(f"Failed to read mask from {self.mask_path}: {e}")
if update:
self.mask_path = None
def export_mask(self, outpath, update=False):
if self.mask_path is not None:
- etau.copy_file(self.mask_path, outpath)
+ try:
+ etau.copy_file(self.mask_path, outpath)
+ except (IOError, OSError) as e:
+ raise IOError(f"Failed to copy mask from {self.mask_path} to {outpath}: {e}")
else:
- _write_mask(self.mask, outpath)
+ try:
+ _write_mask(self.mask, outpath)
+ except (IOError, OSError) as e:
+ raise IOError(f"Failed to write mask to {outpath}: {e}")
if update:
self.mask = None
self.mask_path = outpath
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
🧹 Outside diff range and nitpick comments (1)
app/packages/looker/src/worker/index.ts (1)
114-125: Consider parallel processing for better performanceWhile the current sequential processing is safe, consider using
Promise.allwith proper error handling for better performance when processing multiple detections:- for (const detection of label.detections) { - await imputeOverlayFromPath( - field, - detection, - coloring, - customizeColorSetting, - colorscale, - buffers, - {}, - DETECTION - ); - } + await Promise.all( + label.detections.map((detection) => + imputeOverlayFromPath( + field, + detection, + coloring, + customizeColorSetting, + colorscale, + buffers, + {}, + DETECTION + ).catch(error => { + console.error('Failed to process detection:', error); + // Handle or rethrow based on requirements + throw error; + }) + ) + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/looker/src/worker/index.ts(2 hunks)fiftyone/core/labels.py(2 hunks)fiftyone/utils/coco.py(2 hunks)fiftyone/utils/cvat.py(1 hunks)fiftyone/utils/eta.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/worker/index.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)
app/packages/looker/src/worker/index.ts (2)
8-8: LGTM: Import addition is correct and well-placed
The addition of the DETECTION constant import aligns with the PR's objective of enhancing detection mask support.
114-125: LGTM: Improved async handling for detection masks
The change from forEach to for...of with await ensures proper sequential processing of detection masks, preventing potential race conditions and memory leaks.
fiftyone/utils/eta.py (1)
599-599: LGTM! Proper encapsulation of mask access.
The change from direct mask access to using get_mask() method aligns with the PR objective of supporting on-disk masks.
Let's verify the implementation of the get_mask() method:
✅ Verification successful
Verified: get_mask() properly handles both in-memory and on-disk masks
The implementation correctly handles both cases:
- Returns
self.maskif the mask is stored in memory - Falls back to loading from
self.mask_pathif mask is stored on disk - Returns
Noneif neither is available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of get_mask() method in Detection class
# Expected: Method that handles both in-memory and on-disk masks
ast-grep --pattern 'class Detection {
$$$
get_mask($$$) {
$$$
}
$$$
}'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for Detection class and get_mask method
rg "class Detection" -A 20
# Also search for get_mask method definition separately
rg "def get_mask"
# Search for any imports of Detection class to find its location
rg "from.*Detection.*import"
Length of output: 10373
Script:
#!/bin/bash
# Let's check the implementation of get_mask in Detection class
rg "def get_mask" -A 10 fiftyone/core/labels.py
Length of output: 656
fiftyone/utils/coco.py (2)
1307-1307: Good use of encapsulation with has_mask() method
Replacing direct mask attribute access with the has_mask() method improves encapsulation and makes the code more maintainable.
2119-2119: Consistent use of has_mask() method
Good consistency in using the has_mask() method here as well, maintaining the encapsulation pattern throughout the codebase.
fiftyone/utils/cvat.py (1)
6403-6403: Verify the implementation of has_mask() method
The change from det.mask to det.has_mask() looks good as it aligns with the support for detection.mask_path. This change allows for checking both in-memory masks and on-disk masks.
Run the following script to verify the implementation:
fiftyone/core/labels.py (4)
421-424: Good implementation of the has_mask property
The has_mask property accurately checks for the presence of a mask, whether it's in-memory (mask) or stored on disk (mask_path).
426-439: get_mask method correctly retrieves the mask
The get_mask method properly returns the mask from the mask attribute or reads it from mask_path when mask is not available.
455-472: export_mask method handles mask exporting appropriately
The export_mask method effectively exports the mask to the specified outpath, managing both cases where the mask is in-memory or referenced by mask_path.
527-528: Enhanced code clarity by using get_mask() in to_segmentation
Utilizing self.get_mask() in the to_segmentation method ensures that the mask is correctly obtained from either mask or mask_path, improving code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (1)
10-14: Consider using dynamic temp directories instead of hardcoded pathsUsing hardcoded paths in
/tmpmight cause issues in different environments or parallel test runs. Consider using Node'sos.tmpdir()with unique subdirectories for test isolation.-const badDetectionMaskSampleImage = "/tmp/detection-bad-mask-img.png"; -const goodDetectionMaskSampleImage = "/tmp/detection-good-mask-img.png"; -const goodDetectionMaskPathSampleImage = "/tmp/detection-mask-path-img.png"; -const goodDetectionMaskOnDisk = "/tmp/detection-mask-on-disk.png"; +const testDir = path.join(os.tmpdir(), `detection-mask-${Date.now()}`); +const badDetectionMaskSampleImage = path.join(testDir, "detection-bad-mask-img.png"); +const goodDetectionMaskSampleImage = path.join(testDir, "detection-good-mask-img.png"); +const goodDetectionMaskPathSampleImage = path.join(testDir, "detection-mask-path-img.png"); +const goodDetectionMaskOnDisk = path.join(testDir, "detection-mask-on-disk.png");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts-snapshots/grid-detections-chromium-darwin.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
e2e-pw/src/oss/specs/overlays/detection-mask.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.
🔇 Additional comments (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (1)
Line range hint 54-90: Well-structured test data covering multiple scenarios
The test data setup effectively covers three important scenarios:
- Invalid mask (empty array)
- Valid in-memory mask
- Valid on-disk mask path
This provides good coverage for the new detection.mask_path feature.
sashankaryal
force-pushed
the
feat/on-disk-detection-mask-path
branch
from
db07f00 to
a79cf96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
e2e-pw/src/oss/specs/plugins/histograms.spec.ts (1)
Line range hint
16-43: Enhance test data to include mask_path samples.The test data setup creates Detection objects but doesn't include mask_path values. Consider enhancing the test coverage by including samples with mask paths.
Here's a suggested modification to the test data setup:
samples = [] for i in range(0, 10): sample = fo.Sample( filepath=f"{i}.png", - detections=fo.Detections(detections=[fo.Detection(label=f"label-{i}")]), + detections=fo.Detections(detections=[ + fo.Detection( + label=f"label-{i}", + mask_path=f"masks/{i}_mask.png" + ) + ]), classification=fo.Classification(label=f"label-{i}"),e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (1)
26-40: Add error handling for image creationThe Promise.all for image creation should include error handling to ensure test stability and proper cleanup on failure.
await Promise.all( [ badDetectionMaskSampleImage, goodDetectionMaskSampleImage, goodDetectionMaskPathSampleImage, ].map((img, index) => { const fillColor = colors[index]; - mediaFactory.createBlankImage({ - outputPath: img, - width: 25, - height: 25, - fillColor: fillColor, - }); + return mediaFactory.createBlankImage({ + outputPath: img, + width: 25, + height: 25, + fillColor: fillColor, + }).catch(error => { + console.error(`Failed to create test image ${img}:`, error); + throw error; + }); }) );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts-snapshots/grid-detections-chromium-darwin.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (7)
app/packages/looker/src/worker/index.ts(2 hunks)e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts(4 hunks)e2e-pw/src/oss/specs/plugins/histograms.spec.ts(1 hunks)fiftyone/core/labels.py(2 hunks)fiftyone/utils/coco.py(2 hunks)fiftyone/utils/cvat.py(1 hunks)fiftyone/utils/eta.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- fiftyone/utils/coco.py
- fiftyone/utils/cvat.py
- fiftyone/utils/eta.py
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/worker/index.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/overlays/detection-mask.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.
e2e-pw/src/oss/specs/plugins/histograms.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.
🔇 Additional comments (11)
e2e-pw/src/oss/specs/plugins/histograms.spec.ts (1)
62-62: LGTM! Field addition is consistent with PR objectives.
The addition of "detections.detections.mask_path" to the verification list aligns with the PR's goal of adding mask path support.
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (1)
73-74: Validate bounding box coordinates and mask dimensions
The bounding box coordinates [0.0, 0.0, 0.5, 0.5] and mask dimensions (15x15) are hardcoded. Consider:
- Adding validation for bounding box coordinates (should be normalized [0-1])
- Making mask dimensions proportional to the image size (25x25)
app/packages/looker/src/worker/index.ts (2)
8-8: LGTM: Clean import addition
The addition of the DETECTION constant import is well-placed and aligns with the PR's objective of supporting detection mask paths.
114-125: LGTM: Improved async handling for detection processing
The changes improve the handling of detections by:
- Using
for...ofloop to properly support async/await - Using the type-safe
DETECTIONconstant - Ensuring proper sequential processing of detection masks
This enhancement prevents potential race conditions in mask processing and aligns with the PR's objective.
fiftyone/core/labels.py (7)
412-413: LGTM! Class attribute follows established patterns.
The _MEDIA_FIELD class attribute correctly identifies the media path field and follows the same pattern used by other classes in the codebase.
404-405: LGTM! Field declaration and documentation are appropriate.
The mask_path field is correctly declared as a StringField and well-documented in the class docstring.
Also applies to: 417-417
421-424: LGTM! Property implementation is clean and consistent.
The has_mask property correctly checks both in-memory and on-disk mask representations.
426-438: LGTM! Method handles mask retrieval appropriately.
The get_mask method implements a clear priority between in-memory and on-disk masks, with proper documentation.
449-454: LGTM! Method correctly handles mask import.
The implementation properly handles importing the mask from disk and updating the path attribute based on the update parameter.
455-471: LGTM! Method handles mask export comprehensively.
The export_mask method correctly handles both file copying and mask writing scenarios, with proper state management based on the update parameter.
527-528: LGTM! Method properly uses the new mask retrieval approach.
The to_segmentation method has been correctly updated to use the new get_mask method for consistent mask handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
fiftyone/core/labels.py (3)
421-424: Enhance the docstring for thehas_maskpropertyConsider expanding the docstring to clarify that it checks both in-memory and on-disk masks.
@property def has_mask(self): - """Whether this instance has a mask.""" + """Whether this instance has a mask, either in memory or on disk.""" return self.mask is not None or self.mask_path is not None
426-438: Enhance the docstring for theget_maskmethodConsider expanding the docstring to specify the return type and clarify the priority order.
def get_mask(self): - """Returns the detection mask for this instance. + """Returns the detection mask for this instance, prioritizing in-memory mask over on-disk mask. Returns: - a numpy array, or ``None`` + a numpy array of shape (height, width) containing the mask data, or ``None`` if no mask exists """
440-470: Add mask format specification in docstringsThe methods would benefit from explicit documentation of the mask format.
def import_mask(self, update=False): """Imports this instance's mask from disk to its :attr:`mask` - attribute. + attribute. The mask should be a 2D binary or 0/1 integer numpy array. Args: update (False): whether to clear this instance's :attr:`mask_path` attribute after importing """ def export_mask(self, outpath, update=False): """Exports this instance's mask to the given path. + + The mask is written as a 2D binary image file. Args: outpath: the path to write the mask update (False): whether to clear this instance's :attr:`mask` attribute and set its :attr:`mask_path` attribute when exporting in-database segmentations """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/core/labels.py(2 hunks)
🔇 Additional comments (2)
fiftyone/core/labels.py (2)
404-405: LGTM: Well-structured media field implementation
The addition of mask_path attribute and _MEDIA_FIELD class variable follows the established pattern for media fields in the codebase.
Also applies to: 412-413
526-527: LGTM: Clean integration with new mask handling
The update to use get_mask() properly integrates with the new mask handling system.
sashankaryal
force-pushed
the
feat/on-disk-detection-mask-path
branch
2 times, most recently
from
7e2e35e to
fb8b322
Compare
sashankaryal
force-pushed
the
feat/on-disk-detection-mask-path
branch
from
fb8b322 to
9fe5978
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts (2)
8-8: Consider adding JSDoc comment for the colors arrayAdd documentation explaining the purpose of each color in the array and its relationship to the test cases.
+/** Colors used for creating test images: + * - Red (#ff0000): Bad detection mask + * - Green (#00ff00): Good detection mask + * - Blue (#0000ff): Good detection mask with path + */ const colors = ["#ff0000", "#00ff00", "#0000ff"];
Line range hint
54-65: Consider parameterizing the test dataThe hardcoded values for bounding boxes and mask dimensions could be defined as constants at the top of the file for better maintainability.
+const TEST_CONFIG = { + emptyMask: { + bbox: [0.0, 0.0, 0.0, 0.0], + maskDims: [0, 0] + }, + goodMask: { + bbox: [0.0, 0.0, 0.5, 0.5], + maskDims: [15, 15] + } +}; # Then use in the code: - bounding_box=[0.0, 0.0, 0.0, 0.0], - mask=np.empty((0, 0)), + bounding_box=TEST_CONFIG.emptyMask.bbox, + mask=np.empty(TEST_CONFIG.emptyMask.maskDims),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts-snapshots/grid-detections-chromium-darwin.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (7)
app/packages/looker/src/worker/index.ts(2 hunks)e2e-pw/src/oss/specs/overlays/detection-mask.spec.ts(4 hunks)e2e-pw/src/oss/specs/plugins/histograms.spec.ts(1 hunks)fiftyone/core/labels.py(2 hunks)fiftyone/utils/coco.py(2 hunks)fiftyone/utils/cvat.py(1 hunks)fiftyone/utils/eta.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/packages/looker/src/worker/index.ts
- e2e-pw/src/oss/specs/plugins/histograms.spec.ts
- fiftyone/utils/cvat.py
- fiftyone/utils/eta.py
🧰 Additional context used
📓 Path-based instructions (1)
e2e-pw/src/oss/specs/overlays/detection-mask.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.
🔇 Additional comments (6)
fiftyone/core/labels.py (6)
404-405: LGTM! Well-documented mask_path attribute
The mask_path attribute and _MEDIA_FIELD constant are properly implemented and documented.
Also applies to: 412-413, 417-417
421-424: LGTM! Clear and correct has_mask property
The has_mask property correctly checks for both in-memory and on-disk masks.
426-438: LGTM! Well-implemented get_mask method
The get_mask method provides a clean interface for retrieving masks from either memory or disk.
440-452: Skip comment about incorrect docstring
A previous review has already identified the incorrect 'outpath' reference in the docstring.
454-470: LGTM! Comprehensive export_mask implementation
The export_mask method effectively handles all mask export scenarios with proper memory management options.
526-527: LGTM! Proper integration with get_mask
The to_segmentation method correctly uses the new get_mask method for consistent mask handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
fiftyone/core/labels.py (2)
Line range hint
397-418: Add return type hints to docstringsThe class docstring should specify return types for properties and methods that return values.
Line range hint
1150-1250: Consider extracting common mask handling logicThe
DetectionandSegmentationclasses have identical mask handling methods (has_mask,get_mask,import_mask,export_mask). Consider extracting these into a mixin class to reduce code duplication.Create a new mixin class like this:
class _HasMask(_HasMedia): """Mixin for Label classes that have mask functionality.""" @property def has_mask(self): """Whether this instance has a mask.""" return self.mask is not None or self.mask_path is not None def get_mask(self): """Returns the mask for this instance.""" if self.mask is not None: return self.mask if self.mask_path is not None: return _read_mask(self.mask_path) return None def import_mask(self, update=False): """Imports this instance's mask from disk.""" if self.mask_path is not None: try: self.mask = _read_mask(self.mask_path) if update: self.mask_path = None except (IOError, OSError) as e: raise IOError(f"Failed to read mask from {self.mask_path}: {e}") def export_mask(self, outpath, update=False): """Exports this instance's mask to disk.""" try: if self.mask_path is not None: etau.copy_file(self.mask_path, outpath) else: _write_mask(self.mask, outpath) if update: self.mask = None self.mask_path = outpath except (IOError, OSError) as e: raise IOError(f"Failed to write mask to {outpath}: {e}")Then update the class definitions:
-class Detection(_HasAttributesDict, _HasID, _HasMedia, Label): +class Detection(_HasAttributesDict, _HasID, _HasMask, Label): -class Segmentation(_HasID, _HasMedia, Label): +class Segmentation(_HasID, _HasMask, Label):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/core/labels.py(4 hunks)
🔇 Additional comments (1)
fiftyone/core/labels.py (1)
43-48: Well-designed mixin class!
The _HasMedia mixin provides a clean abstraction for labels with media fields, following good OOP principles.
| @@ -1044,7 +1100,6 @@ def import_mask(self, update=False): | |||
| attribute. | |||
|
|
|||
| Args: | |||
| outpath: the path to write the map | |||
| // putImageData results in an UInt8ClampedArray (for both grayscale or RGB masks), | ||
| // where each pixel is represented by 4 bytes (RGBA) | ||
| // it's packed like: [R, G, B, A, R, G, B, A, ...] | ||
| // use first channel info to determine if the pixel is in the mask | ||
| // skip second (G), third (B) and fourth (A) channels |
There was a problem hiding this comment.
Ok, we are always loading a 32 bit image when mask_path is used
What changes are proposed in this pull request?
Continuation of #4693. This PR adds support for
detection.mask_path. Works for both grayscale and multi-channel masks.Until now, this was the only label type which didn't have support for on-disk masks.
How is this patch tested? If it is not, please explain why.
Golden path e2e. Also exported and re-imported the following dataset with and verified to be working.
To test locally, run the following script:
You should get a dataset with samples that look like:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
mask_path,has_mask,get_mask,import_mask, andexport_mask.Improvements
has_mask().Bug Fixes