Segmentation annotations default flow modifications #4990
Conversation
WalkthroughThe changes primarily focus on enhancing the Changes
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.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/utils/annotations.py (3 hunks)
🔇 Additional comments (1)
fiftyone/utils/annotations.py (1)
849-867: Great improvements to type hints and documentation!The addition of type hints and comprehensive docstring significantly improves code readability and maintainability. The docstring clearly explains the function's purpose, parameters, and return values.
| # If this is a new field, users must define mask targets | ||
| if not existing_field: | ||
| raise ValueError( | ||
| f"Must specify mask_targets argument or in schema for new segmentations field '{label_field}'" | ||
| ) | ||
|
|
||
| # Attempt to find mask targets, otherwise bail and use a default set | ||
| if label_field in samples.mask_targets: | ||
| mask_targets = samples.mask_targets[label_field] | ||
| elif samples.default_mask_targets != {}: | ||
| mask_targets = samples.default_mask_targets | ||
| else: | ||
| mask_targets = {i: str(i) for i in range(1, 256)} | ||
| mask_targets[0] = "background" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for mask target values.
The logic for handling mask targets is well-structured and aligns with the PR objectives. However, consider adding validation to ensure that:
- The mask target values are non-negative integers
- The value 0 is either undefined or maps to "background"
- There are no duplicate labels in the values
Here's a suggested implementation:
if mask_targets is None:
# If this is a new field, users must define mask targets
if not existing_field:
raise ValueError(
f"Must specify mask_targets argument or in schema for new segmentations field '{label_field}'"
)
# Attempt to find mask targets, otherwise bail and use a default set
if label_field in samples.mask_targets:
mask_targets = samples.mask_targets[label_field]
elif samples.default_mask_targets != {}:
mask_targets = samples.default_mask_targets
else:
mask_targets = {i: str(i) for i in range(1, 256)}
mask_targets[0] = "background"
+ # Validate mask targets
+ if not all(isinstance(k, int) and k >= 0 for k in mask_targets.keys()):
+ raise ValueError("Mask target keys must be non-negative integers")
+
+ if 0 in mask_targets and mask_targets[0] != "background":
+ raise ValueError("Mask target 0 must map to 'background' if defined")
+
+ if len(set(mask_targets.values())) != len(mask_targets):
+ raise ValueError("Mask target values must be unique")
classes = [c for v, c in mask_targets.items() if v != 0]📝 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 this is a new field, users must define mask targets | |
| if not existing_field: | |
| raise ValueError( | |
| f"Must specify mask_targets argument or in schema for new segmentations field '{label_field}'" | |
| ) | |
| # Attempt to find mask targets, otherwise bail and use a default set | |
| if label_field in samples.mask_targets: | |
| mask_targets = samples.mask_targets[label_field] | |
| elif samples.default_mask_targets != {}: | |
| mask_targets = samples.default_mask_targets | |
| else: | |
| mask_targets = {i: str(i) for i in range(1, 256)} | |
| mask_targets[0] = "background" | |
| # If this is a new field, users must define mask targets | |
| if not existing_field: | |
| raise ValueError( | |
| f"Must specify mask_targets argument or in schema for new segmentations field '{label_field}'" | |
| ) | |
| # Attempt to find mask targets, otherwise bail and use a default set | |
| if label_field in samples.mask_targets: | |
| mask_targets = samples.mask_targets[label_field] | |
| elif samples.default_mask_targets != {}: | |
| mask_targets = samples.default_mask_targets | |
| else: | |
| mask_targets = {i: str(i) for i in range(1, 256)} | |
| mask_targets[0] = "background" | |
| # Validate mask targets | |
| if not all(isinstance(k, int) and k >= 0 for k in mask_targets.keys()): | |
| raise ValueError("Mask target keys must be non-negative integers") | |
| if 0 in mask_targets and mask_targets[0] != "background": | |
| raise ValueError("Mask target 0 must map to 'background' if defined") | |
| if len(set(mask_targets.values())) != len(mask_targets): | |
| raise ValueError("Mask target values must be unique") |
What changes are proposed in this pull request?
Require users to specify a
mask_targetsargument for new segmentations annotations, otherwise fall back onto the dataset's defaults specified indataset.mask_targetsordataset.default_mask_targets.How is this patch tested? If it is not, please explain why.
Manual testing of annotation flow with a test CVAT instance.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
When requesting semantic segmentations annotations, users must specify
mask_targetsfor fields that do not yet exist. If no targets are specified, the targets will be pulled fromdataset.mask_targetsordataset.default_mask_targetsif either exist.What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation