[CVAT integration] Use pixelwise masks, not polygons, for instance segmentation #4483 #4937
Conversation
… for instance segmentation voxel51#4483 - We can upload mask [] Missing test [] Missing download mask
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4937 +/- ##
=========================================
Coverage 99.17% 99.18%
=========================================
Files 49 49
Lines 17785 17991 +206
=========================================
+ Hits 17639 17845 +206
Misses 146 146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Amazing, thanks @NicDionne! @ehofesmann will give the PR a review and assist with getting this merged 💪 |
There was a problem hiding this comment.
This is awesome! Thank you so much for taking this on @NicDionne !!
To your questions:
- We don't have a great way to mock the CVAT API at the moment, so most testing is done against a running CVAT instance. In the end, it would be great to add a new test for this to our test suite here.
- In order to be able to load the CVAT mask back into FiftyOne, you'd need to add a new check here for
if shape_type == "mask", which can then check forinstanceorinstancestypes and call your newCVATShape.to_instance_detection()method. Let me know if you run into any issues with this!
|
It seems I'm having trouble committing files from cvat_test.py. Pylint is throwing multiple E0401 and E1101 errors, even though everything seems to be working fine. Here is the code for the unit test I ran: |
WalkthroughThe changes introduce a new class Changes
Possibly related issues
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: 3
🧹 Outside diff range and nitpick comments (4)
fiftyone/utils/cvat.py (4)
1607-1615: Possible Improvement inmask_to_cvat_rleMethodThe implementation of
mask_to_cvat_rlecan be optimized for readability and efficiency. Currently, the method iterates over the grouped elements of the flattened binary mask and constructs the RLE counts.Consider using
np.packbitsor other NumPy vectorized operations to optimize the RLE encoding process.While the current implementation is correct, utilizing NumPy's vectorized functions can improve performance, especially for large masks.
1592-1616: Remove Unused Commented CodeLines 1602-1604 contain commented-out code that seems to be an alternative implementation of
rle_to_binary_image_mask. Unless this code is necessary for future reference, it would improve code cleanliness to remove it.Remove the commented-out code to enhance readability:
- # mask = np.zeros(mask_width * mask_height, dtype=np.uint8) - # mask[np.add.accumulate(rle)[::2]] = 1 - # return mask.reshape(mask_width, mask_height)
6439-6443: Swap Width and Height Variables for ConsistencyIn the calculation of
mask_heightandmask_widthfromdet.mask.shape, there is a potential mix-up between width and height:
- Line 6441:
mask_height, mask_width = det.mask.shapeassignsmask_heighttodet.mask.shape[0](which is typically the number of rows, i.e., the height).This could lead to confusion and inconsistencies later in the code.
Consider assigning
mask_height, mask_widthas:- mask_height, mask_width = det.mask.shape + mask_width, mask_height = det.mask.shapeEnsure that width corresponds to the number of columns and height corresponds to the number of rows.
1590-1617: Add Docstrings toHasCVATBinMaskMethodsThe methods
rle_to_binary_image_maskandmask_to_cvat_rlelack docstrings explaining their purpose, parameters, and return values.Add descriptive docstrings to improve code readability and maintainability:
class HasCVATBinMask: @staticmethod def rle_to_binary_image_mask(rle, mask_width, mask_height) -> np.ndarray: """ Converts a run-length encoded (RLE) mask to a binary image mask. Args: rle: The run-length encoded mask as a list of integers. mask_width: The width of the mask. mask_height: The height of the mask. Returns: A binary mask as a NumPy ndarray. """ ... @staticmethod def mask_to_cvat_rle(binary_mask: np.ndarray) -> np.array: """ Converts a binary image mask to a CVAT-compatible RLE format. Args: binary_mask: The binary mask as a NumPy ndarray. Returns: A list of integers representing the RLE. """ ...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/utils/cvat.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
fiftyone/utils/cvat.py (2)
5923-5925: Ensure Consistent Handling of 'mask' Shape TypesIn the
_parse_annotationmethod, the addition ofelif shape_type == "mask":introduces handling for mask shapes.
- Line 5925: The method
cvat_shape.to_instance_detection()is called to convert the mask shape to a detection.The implementation correctly extends the method to handle mask shapes. Ensure that any invoking code correctly processes the returned detection with a mask.
6436-6458: Handle Cases Whendet.maskis MissingIn the
_create_detection_shapesmethod, whenlabel_typeis"instance"or"instances", the code expectsdet.maskto be present.
- Line 6437: Checks if
det.maskisNoneand continues if so.Confirm that
det.maskis always provided for instances of this label type. Ifdet.maskcan beNone, consider logging a warning or handling the case appropriately.Ensure that the dataset or upstream code provides the necessary mask data for these instances.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/utils/cvat.py (1)
7104-7134: LGTM! New method for converting CVAT shapes to FiftyOne instance detections.The
to_instance_detectionmethod is a valuable addition to theCVATShapeclass, enabling conversion of CVAT shapes with masks to FiftyOne Detection objects. The implementation correctly handles the conversion process and makes good use of the newHasCVATBinMaskclass.Consider adding a brief comment explaining why we add 1 to
mask_wandmask_h. For example:# Add 1 to account for CVAT's inclusive coordinate system mask_w, mask_h = ( round(xbr - xtl) + 1, round(ybr - ytl) + 1, )This would improve code clarity for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/utils/cvat.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
fiftyone/utils/cvat.py (1)
1590-1613: LGTM! New utility class for handling CVAT binary masks.The
HasCVATBinMaskclass is a well-implemented utility for converting between run-length encoded (RLE) masks and binary image masks. The use of numpy for array operations is efficient, and the methods are clearly defined with appropriate type hints.
|
Hi @brimoor, It looks like the build failed, but strangely it stopped at FFmpeg. I'm not really sure what I might have touched that could cause this. Do you have any ideas? Thanks, |
This pull request is currently a draft. The following items are still pending:
@ehofesmann
fiftyone/fiftyone/utils/cvat.py
Line 5908 in 77f664c
What changes are proposed in this pull request?
Propose following the issue #4483, to make it possible to export fiftyone mask to CVAT as pixel mask.
How is this patch tested? If it is not, please explain why.
For now, simple upload test with rectangle. I seek guidance regarding how it should be properly tested. For now this pull request is only a draft.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
Yes,
The data should now be uploaded in pixel format instead of polygons. This change is good because CVAT does not support polygons with holes, which could create bottlenecks in certain use cases.
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit