Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Segmentation annotations default flow modifications #4990

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Segmentation annotations default flow modifications #4990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6faa20
Fix mask target flow
geke-mir Oct 25, 2024
050c702
Remove stray breakpoint
geke-mir Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions fiftyone/utils/annotations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ def annotate(
define their per-field attributes (in addition to any per-class
attributes)
mask_targets (None): a dict mapping pixel values to semantic label
strings. Only applicable when annotating semantic segmentations
strings. Only applicable when annotating semantic segmentations.
Must specify if the field does not exist prior to annotation.
allow_additions (True): whether to allow new labels to be added. Only
applicable when editing existing label fields
allow_deletions (True): whether to allow labels to be deleted. Only
Expand Down Expand Up @@ -468,7 +469,11 @@ def _build_label_schema(

if _label_type == "segmentation":
_mask_targets, _classes = _get_mask_targets(
samples, mask_targets, _label_field, _label_info
samples,
mask_targets,
_label_field,
_label_info,
_existing_field,
)
else:
_mask_targets = None
Expand Down Expand Up @@ -841,13 +846,44 @@ def _parse_classes_dict(
return {"classes": classes, "attributes": attributes}


def _get_mask_targets(samples, mask_targets, label_field, label_info):
def _get_mask_targets(
samples,
mask_targets: dict,
label_field: str,
label_info: dict,
existing_field: bool,
) -> tuple[dict[int, str], list[str]]:
"""Returns mask targets for a semantic segmentation field

Args:
samples: The sample collection that is being annotated
mask_targets: A dictionary mapping pixel values to semantic labels, 0 is reserved for the background
label_field: The name of the field where the semantic segmentation masks will be stored
label_info: The label schema information for the field
existing_field: Whether the field already exists in the dataset

Returns:
A tuple containing the mask targets dictionary {1: "label1", 2: "label2", ...} and a list of class names ["label1", "label2", ...]
"""
# We have defined mask targets for this field
if "mask_targets" in label_info:
mask_targets = label_info["mask_targets"]

if mask_targets is None:
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"
Comment on lines +873 to +886
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. The mask target values are non-negative integers
  2. The value 0 is either undefined or maps to "background"
  3. 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.

Suggested change
# 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")


classes = [c for v, c in mask_targets.items() if v != 0]

Expand Down
Loading