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.

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

Add the option of using panoptic segmentation masks #4829

Merged

Conversation

kemaleren
Copy link
Contributor

@kemaleren kemaleren commented Sep 21, 2024

Panoptic segmentations contain both instance objects and semantic segmentation. Instead of PR#4500, which implemented disk-backed detections as a new type of label, this PR extends the Segmentation class. It supports all the same functionality and more, so this PR replaces that one.

What changes are proposed in this pull request?

This pull request modifies the Segmentation class to allow it to represent both semantic and panoptic segmentations. The mask attribute for panoptic segmentations must be a 2-channel array, where the first channel contains a semantic segmentation and the second channel contains an object index. It would have been cleaner to maintain two separate arrays -- one for semantic segmentation and one for objects -- but this implementation maintains backwards compatibility with the way semantic segmentations work, which is to keep everything in a single mask array or single image indicated by mask_path.

For instance, the following segmentation contains three objects, each with a different class (10, 20, and 30), as well as a region of "stuff" with another class (99):

class_mask = np.array([
    [10, 99, 20, 20],
    [10, 10, 99, 20],
    [30, 30, 99, 99],
    [30, 99, 99, 99],
])

instance_mask = np.array([
    [1, 0, 2, 2],
    [1, 1, 0, 2],
    [3, 3, 0, 0],
    [3, 0, 0, 0],
])

panoptic_mask = np.stack(
    [class_mask, instance_mask], axis=-1
)
segmentation = fo.Segmentation(mask=panoptic_mask, is_panoptic=True)

When saving to or loading from an image, such as a PNG, the third channel is left empty for panoptic segmentations. Therefore the is_panoptic boolean is necessary to indicate whether the third channel is empty actually, or whether it represents all 0 values in the blue channel of an RGB segmentation mask.

This PR also adds the tifffile package to allow saving and loading two-channel TIFF images, which allows a higher bit depth for saving saving segmentation class indices or object labels.

Conversions to/from Detection, Detections, Polyline, and Polylines are implemented. Two more mask_type values are now recognized, in addition to the "stuff" and "thing" types, because there are a number of ways that panoptic segmentations can be converted to other labels:

  • "stuff" - object instance labels are ignored. This option is identical to "stuff" on a normal semantic segmentation.
  • "thing" - object instances are first converted to individual detections/polylines. Then any remaining connected regions are also converted to individual detections/polylines, just like converting "thing" semantic classes.
  • "panoptic" - object instances are first converted to individual detections/polylines. Then any remaining regions are converted to one detection/polyline, just like converting "stuff" semantic classes. This option is called "panoptic" because it most closely aligns with the original intention of treating objects as "thing" and non-object regions as "stuff".
  • "object" - i.e. objects only. Object instances are first converted to individual detections/polylines. Then any remaining regions are ignored.

I'm opening this PR so we can start a discussion about this approach and make sure it aligns with the FiftyOne team's expectations. Before final approval, the following should be implemented and tested:

  • User documentation
  • Evaluations need to be updated to work with panoptic segmentations. Panoptic Quality (PQ) should be implemented.
  • Other code that imports or transforms segmentations needs to be updated to support the panoptic option.

How is this patch tested? If it is not, please explain why.

There are unit tests for all possible conversions and saving/loading the semantic segmentations.

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

The Segmentation class now supports panoptic segmentations, which indicate not only the segmentation class of a pixel but also which object to which it belongs. Panoptic Segmentation objects can be converted to detections and polylines, as well as to semantic segmentations, and they can be saved as TIF files.

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

Summary by CodeRabbit

  • New Features

    • Introduced panoptic segmentation functionality, allowing for the handling of instance masks alongside class masks.
    • Added methods to facilitate conversion between semantic and panoptic segmentations.
  • Bug Fixes

    • Updated mask processing methods to correctly handle the new panoptic attribute.
  • Chores

    • Added tifffile as a required dependency for handling TIFF files.
  • Tests

    • Implemented unit tests for validating segmentation conversions, enhancing the testing framework for segmentation functionalities.

* store instances and classes in two-channel mask arrays

* conversion to/from detections, polylines, and other segmentations

* option to store segmentation masks as TIFFs

* new mask_types options: "panoptic" and "object"

* unit tests
Copy link
Contributor

coderabbitai bot commented Sep 21, 2024

Walkthrough

The changes introduce new functionalities for handling panoptic segmentation in the fiftyone/core/labels.py file. Key modifications include the addition of a panoptic parameter in several methods, the introduction of new methods for converting between semantic and panoptic segmentations, and updates to existing methods to support the new functionality. Additionally, a new dependency on the tifffile library is added in setup.py, and comprehensive unit tests are implemented to validate these changes.

Changes

File Change Summary
fiftyone/core/labels.py Added panoptic segmentation handling, updated method signatures, and introduced new conversion methods.
setup.py Added tifffile as a new dependency in the install_requires list.
tests/unittests/label_tests.py Added unit tests for segmentation mask creation and conversion, including helper functions.

Possibly related issues

Possibly related PRs

Suggested reviewers

  • brimoor

Poem

In the land of code where rabbits play,
New masks and methods brighten the day.
With panoptic dreams and tests so bright,
We hop through functions, oh what a sight!
Let's celebrate changes, both big and small,
For in FiftyOne's world, we cherish them all! 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@kemaleren kemaleren mentioned this pull request Sep 21, 2024
7 tasks
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Outside diff range and nitpick comments (5)
tests/unittests/label_tests.py (1)

613-614: Complete TODOs by adding assertions for mask verification

There are TODO comments indicating that mask comparisons are pending. Adding assertions will ensure that the tests validate the expected outcomes.

Would you like assistance in implementing assertions to compare the masks? For example:

# After generating sseg1 and sseg2
self.assertTrue(np.array_equal(sseg1.mask, expected_mask))
self.assertTrue(np.array_equal(sseg2.mask, expected_mask))

Let me know if you'd like me to draft these assertions.

Also applies to: 631-632

fiftyone/core/labels.py (4)

1658-1660: Simplify Nested if Statements for Clarity

The nested if statements can be combined into a single condition to improve readability.

Apply this diff to combine the conditions:

-elif mask.ndim == 3:
-    if mask.shape[-1] in (1, 2, 3):
-        shape_is_valid = True
+elif mask.ndim == 3 and mask.shape[-1] in (1, 2, 3):
+    shape_is_valid = True

1791-1794: Use Ternary Operator to Simplify Conditional Assignment

The if-else block assigning is_rgb can be simplified using a ternary operator.

Apply this diff to simplify the code:

-if target is not None:
-    is_rgb = fof.is_rgb_target(target)
-else:
-    is_rgb = False
+is_rgb = fof.is_rgb_target(target) if target is not None else False
Tools
Ruff

1791-1794: Use ternary operator is_rgb = fof.is_rgb_target(target) if target is not None else False instead of if-else-block

Replace if-else-block with is_rgb = fof.is_rgb_target(target) if target is not None else False

(SIM108)


1901-1904: Use Ternary Operator to Simplify Conditional Assignment

The if-else block assigning label can be simplified using a ternary operator.

Apply this diff to simplify the code:

-if mask_targets is not None:
-    label = mask_targets.get(target, None)
-else:
-    label = str(target)
+label = mask_targets.get(target, None) if mask_targets is not None else str(target)
Tools
Ruff

1901-1904: Use ternary operator label = mask_targets.get(target, None) if mask_targets is not None else str(target) instead of if-else-block

Replace if-else-block with label = mask_targets.get(target, None) if mask_targets is not None else str(target)

(SIM108)


1069-1070: Clarify Docstring Reference for mask_path Files

In the docstring, the sentence starting with "They must use the '.tif' or '.tiff' extension." could be clearer regarding what "They" refers to.

Consider rephrasing as:

"Files specified in mask_path must use the '.tif' or '.tiff' extension to ensure proper handling of TIFF or BigTIFF formats."

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 12aec6e and 2344fd8.

Files selected for processing (3)
  • fiftyone/core/labels.py (29 hunks)
  • setup.py (1 hunks)
  • tests/unittests/label_tests.py (3 hunks)
Additional context used
Ruff
fiftyone/core/labels.py

1657-1658: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)


1791-1794: Use ternary operator is_rgb = fof.is_rgb_target(target) if target is not None else False instead of if-else-block

Replace if-else-block with is_rgb = fof.is_rgb_target(target) if target is not None else False

(SIM108)


1901-1904: Use ternary operator label = mask_targets.get(target, None) if mask_targets is not None else str(target) instead of if-else-block

Replace if-else-block with label = mask_targets.get(target, None) if mask_targets is not None else str(target)

(SIM108)

tests/unittests/label_tests.py

591-594: Combine if branches using logical or operator

Combine if branches

(SIM114)


605-605: Local variable sseg1 is assigned to but never used

Remove assignment to unused variable sseg1

(F841)


610-610: Local variable pseg1 is assigned to but never used

Remove assignment to unused variable pseg1

(F841)


622-622: Local variable sseg2 is assigned to but never used

Remove assignment to unused variable sseg2

(F841)


627-627: Local variable pseg2 is assigned to but never used

Remove assignment to unused variable pseg2

(F841)


632-632: Local variable single_seg is assigned to but never used

Remove assignment to unused variable single_seg

(F841)


641-641: Local variable class_mask is assigned to but never used

Remove assignment to unused variable class_mask

(F841)

Additional comments not posted (6)
setup.py (1)

72-72: Verify the necessity of the tifffile dependency.

The addition of the tifffile dependency to the INSTALL_REQUIRES list is approved, assuming it is required for the changes introduced in this pull request.

To confirm the necessity of this dependency, please verify that the code changes in other files actually utilize the tifffile package. If not, consider removing this dependency to keep the installation lightweight.

tests/unittests/label_tests.py (1)

576-722: Well-structured unit tests enhance code reliability

The added test methods comprehensively validate the conversion processes for segmentation masks, covering various scenarios and data types.

Tools
Ruff

591-594: Combine if branches using logical or operator

Combine if branches

(SIM114)


605-605: Local variable sseg1 is assigned to but never used

Remove assignment to unused variable sseg1

(F841)


610-610: Local variable pseg1 is assigned to but never used

Remove assignment to unused variable pseg1

(F841)


622-622: Local variable sseg2 is assigned to but never used

Remove assignment to unused variable sseg2

(F841)


627-627: Local variable pseg2 is assigned to but never used

Remove assignment to unused variable pseg2

(F841)


632-632: Local variable single_seg is assigned to but never used

Remove assignment to unused variable single_seg

(F841)


641-641: Local variable class_mask is assigned to but never used

Remove assignment to unused variable class_mask

(F841)

fiftyone/core/labels.py (4)

485-488: Verify Correct Handling of Panoptic Masks

The logic for creating panoptic segmentation masks by stacking the class mask and instance mask appears correct. Please ensure downstream processes can handle the new mask structure and that the dtype and dimensions are consistent with expectations.


Line range hint 1185-1235: Assess Impact of Changing Default mask_types Parameter

The default value of mask_types in to_detections has changed from "stuff" to None, altering the default behavior based on self.is_panoptic. Verify that this change does not introduce backward compatibility issues and that existing code relying on the previous default behaves as intended.


1341-1345: Ensure Proper Conversion in to_panoptic Method

In the to_panoptic method, if self.is_panoptic is False, the mask is converted to panoptic format. However, if self.is_panoptic is already True, the method returns the mask unchanged. Verify that this behavior is appropriate and that redundant conversions are avoided.


452-454: Ensure Backward Compatibility with Added panoptic Parameter

The panoptic parameter has been added to to_segmentation method in the Detection class. Please verify that this change maintains backward compatibility and does not break any existing overrides or calls that do not expect this parameter.

Run the following script to check for method overrides and usages:

Verification successful

Backward Compatibility Maintained with Added panoptic Parameter

The addition of the panoptic parameter to the to_segmentation method maintains backward compatibility:

  • The parameter has a default value of False, ensuring existing calls without this parameter continue to work as before.
  • Unit tests in tests/unittests/label_tests.py cover both panoptic=True and panoptic=False cases, indicating proper testing of the new functionality.

To ensure a smooth integration:

  • Conduct comprehensive testing to verify that the new parameter doesn't introduce unexpected behavior in various scenarios.
  • Update the method's docstring to document the new panoptic parameter and its usage, if not already done.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all overrides and calls to `to_segmentation` that may be affected.

# Test for method overrides
rg --type py --word-regexp '^\\s*def to_segmentation\\('

# Test for method calls
rg --type py --word-regexp 'to_segmentation\\('

Length of output: 300


Script:

#!/bin/bash
# Find method overrides and calls to to_segmentation

# Find method overrides using ast-grep
ast-grep --lang python --pattern 'def to_segmentation($$$)'

# Find method calls using ast-grep
ast-grep --lang python --pattern '$_.to_segmentation($$$)'

# Use rg as a fallback to find method overrides
rg --type py -n '^\s*def to_segmentation\(' -A 5

# Use rg as a fallback to find method calls
rg --type py -n '\.to_segmentation\(' -A 2

Length of output: 11029

Comment on lines +24 to +43
def _make_panoptic(dtype=np.uint8):
max_value = np.iinfo(dtype).max

instance_mask = np.zeros((8, 8), dtype=dtype)
instance_mask[1:2, 1:2] = 1
instance_mask[2:3, 2:3] = 2
instance_mask[3:4, 3:4] = 3
class_mask = (instance_mask > 0).astype(dtype)

class_mask[4:5, 4:5] = 1
class_mask[5:6, 5:6] = max_value
class_mask[6:7, 6:7] = 1

panoptic_mask = np.stack([class_mask, instance_mask], axis=-1).astype(
dtype
)
seg = fo.Segmentation(mask=panoptic_mask, is_panoptic=True)

return seg

Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor similar functions to reduce code duplication

The functions _make_panoptic, _make_1d_segmentation, and _make_3d_segmentation share similar structures. Refactoring them into a single function with parameters could enhance maintainability.

Consider combining them as follows:

def _make_segmentation(seg_type='panoptic', dtype=np.uint8):
    max_value = np.iinfo(dtype).max
    if seg_type == 'panoptic':
        # Implement panoptic segmentation mask creation
        # ...
        return fo.Segmentation(mask=panoptic_mask, is_panoptic=True)
    elif seg_type == '1d':
        # Implement 1D segmentation mask creation
        # ...
        return fo.Segmentation(mask=mask, is_panoptic=False)
    elif seg_type == '3d':
        # Implement 3D segmentation mask creation
        # ...
        return fo.Segmentation(mask=mask, is_panoptic=False)

dets = seg.to_detections(mask_types=mask_types)
self.assertEqual(len(dets.detections), n_expected)

sseg1 = dets.to_segmentation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused variables to clean up the code

The variables sseg1, pseg1, sseg2, pseg2, and single_seg are assigned but never used, which may cause warnings and reduce code readability.

Apply the following diff to remove the unused variables:

                # Remove unused variable sseg1
-               sseg1 = dets.to_segmentation(
-                   panoptic=False,
-                   frame_size=frame_size,
-                   mask_targets=mask_targets,
-               )
                # Remove unused variable pseg1
-               pseg1 = dets.to_segmentation(
-                   panoptic=True, frame_size=frame_size, mask_targets=mask_targets
-               )

                # Remove unused variable sseg2
-               sseg2 = poly.to_segmentation(
-                   panoptic=False,
-                   frame_size=frame_size,
-                   mask_targets=mask_targets,
-               )
                # Remove unused variable pseg2
-               pseg2 = poly.to_segmentation(
-                   panoptic=True, frame_size=frame_size, mask_targets=mask_targets
-               )

                # Remove unused variable single_seg
-               single_seg = dets.detections[0].to_segmentation(
-                   panoptic=True, frame_size=frame_size
-               )
-               single_seg = poly.polylines[0].to_segmentation(
-                   panoptic=True, frame_size=frame_size
-               )

If these variables are placeholders for future assertions, consider adding comments to clarify their intended use.

Also applies to: 610-610, 622-622, 627-627, 632-632

Tools
Ruff

605-605: Local variable sseg1 is assigned to but never used

Remove assignment to unused variable sseg1

(F841)

Comment on lines 646 to 647
assert np.all(pseg.mask[..., 0] == seg.mask)
assert np.all(pseg.mask[..., 1] == instance_mask)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use self.assertTrue instead of bare assert statements in tests

Direct assert statements do not provide detailed messages on failure within unittest.TestCase methods. It's better to use self.assertTrue, self.assertEqual, etc., for clearer test outcomes.

Replace assert statements with unittest assertions:

-           assert np.all(pseg.mask[..., 0] == seg.mask)
-           assert np.all(pseg.mask[..., 1] == instance_mask)
+           self.assertTrue(np.all(pseg.mask[..., 0] == seg.mask))
+           self.assertTrue(np.all(pseg.mask[..., 1] == instance_mask))

-           assert np.all(seg2.mask == seg.mask)
+           self.assertTrue(np.all(seg2.mask == seg.mask))

-           assert np.all(seg3.mask[..., 2] == seg.mask)
-           assert np.all(seg3.mask[..., 1] == 0)
-           assert np.all(seg3.mask[..., 0] == 0)
+           self.assertTrue(np.all(seg3.mask[..., 2] == seg.mask))
+           self.assertTrue(np.all(seg3.mask[..., 1] == 0))
+           self.assertTrue(np.all(seg3.mask[..., 0] == 0))

-           assert np.all(pseg.mask[..., 0] == class_mask)
-           assert np.all(pseg.mask[..., 1] == instance_mask)
+           self.assertTrue(np.all(pseg.mask[..., 0] == class_mask))
+           self.assertTrue(np.all(pseg.mask[..., 1] == instance_mask))

-           assert np.all(seg2.mask == class_mask)
+           self.assertTrue(np.all(seg2.mask == class_mask))

-           assert np.all(seg3.mask == seg.mask)
+           self.assertTrue(np.all(seg3.mask == seg.mask))

Also applies to: 650-651, 658-662, 677-679, 681-685

Comment on lines 686 to 693
def test_segmentation_io(self):
def _test_io(dims, tif, dtype):
with TemporaryDirectory() as temp_dir:
if tif:
mask_path = Path(temp_dir) / "mask.tif"
else:
mask_path = Path(temp_dir) / "mask.tif"

Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure correct file extensions when saving masks

In the _test_io function, when tif is False, the mask_path still uses a .tif extension. This might lead to confusion or improper file handling.

Update the file extension based on the tif parameter:

                     if tif:
                         mask_path = Path(temp_dir) / "mask.tif"
                     else:
-                        mask_path = Path(temp_dir) / "mask.tif"
+                        mask_path = Path(temp_dir) / "mask.png"

This ensures that non-TIFF masks are saved with the appropriate extension.

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
def test_segmentation_io(self):
def _test_io(dims, tif, dtype):
with TemporaryDirectory() as temp_dir:
if tif:
mask_path = Path(temp_dir) / "mask.tif"
else:
mask_path = Path(temp_dir) / "mask.tif"
def test_segmentation_io(self):
def _test_io(dims, tif, dtype):
with TemporaryDirectory() as temp_dir:
if tif:
mask_path = Path(temp_dir) / "mask.tif"
else:
mask_path = Path(temp_dir) / "mask.png"

Comment on lines 591 to 594
if mask_types is None:
n_expected = 5
elif mask_types == "panoptic":
n_expected = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify conditional statements by combining if branches

The if statements for mask_types can be combined using logical or to make the code more concise.

Apply this change:

             if mask_types is None or mask_types == "panoptic":
                 n_expected = 5
-            elif mask_types == "panoptic":
-                n_expected = 5
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 mask_types is None:
n_expected = 5
elif mask_types == "panoptic":
n_expected = 5
if mask_types is None or mask_types == "panoptic":
n_expected = 5
Tools
Ruff

591-594: Combine if branches using logical or operator

Combine if branches

(SIM114)

Comment on lines 1294 to 1313
def to_semantic(self, to_rgb=False):
"""Convert panoptic segmentation to semantic.

Optionally converts the class integer values back to 3D pixel
values.

Args:
to_rgb (False): convert integer classes to RGB hex values

Returns:
a :class:`Segmentation`

"""
mask = self.get_mask()
if self.is_panoptic:
mask = mask[..., 0]
if to_rgb:
mask = _int_array_to_rgb(mask)

return Segmentation(mask=mask, is_panoptic=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle Non-Panoptic Masks in to_semantic Method

In the to_semantic method, if self.is_panoptic is False, the method returns the original mask without modification. Is this the intended behavior? If so, consider adding a note to the docstring to clarify this case.

You could update the docstring to include:

"If self.is_panoptic is False, the original mask is returned as is."

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 2344fd8 and 93739f3.

Files selected for processing (2)
  • fiftyone/core/labels.py (29 hunks)
  • tests/unittests/label_tests.py (3 hunks)
Additional context used
Ruff
fiftyone/core/labels.py

1660-1661: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)


1794-1797: Use ternary operator is_rgb = fof.is_rgb_target(target) if target is not None else False instead of if-else-block

Replace if-else-block with is_rgb = fof.is_rgb_target(target) if target is not None else False

(SIM108)


1904-1907: Use ternary operator label = mask_targets.get(target, None) if mask_targets is not None else str(target) instead of if-else-block

Replace if-else-block with label = mask_targets.get(target, None) if mask_targets is not None else str(target)

(SIM108)

tests/unittests/label_tests.py

670-670: Local variable class_mask is assigned to but never used

Remove assignment to unused variable class_mask

(F841)

Additional comments not posted (11)
fiftyone/core/labels.py (11)

1689-1705: LGTM!

The function correctly handles reading masks from different file formats, converts the mask to an optimal data type, and extracts the relevant channels for panoptic segmentations.


1707-1720: LGTM!

The function correctly handles writing masks to different file formats, supports higher bit depths and compression for TIFF images, and ensures compatibility with image formats expecting three channels.


Line range hint 1793-1821: LGTM!

The function correctly parses the segmentation target, validates the compatibility of RGB targets with panoptic segmentations, initializes a new mask when needed, and converts RGB targets to the expected format.

Tools
Ruff

1794-1797: Use ternary operator is_rgb = fof.is_rgb_target(target) if target is not None else False instead of if-else-block

Replace if-else-block with is_rgb = fof.is_rgb_target(target) if target is not None else False

(SIM108)


Line range hint 1823-1853: LGTM!

The function correctly parses the segmentation mask targets, validates the compatibility of RGB targets with panoptic segmentations, initializes a new mask when needed, and creates a dictionary mapping labels to target values for efficient rendering.


Line range hint 452-488: LGTM!

The to_segmentation method correctly converts a detection to a segmentation by validating the presence of an instance mask, parsing the segmentation target, rendering the instance mask onto the segmentation mask, and creating a two-channel mask for panoptic segmentations.


Line range hint 570-623: LGTM!

The to_segmentation method correctly converts detections to a segmentation by parsing the mask targets, initializing the mask, iterating over the detections to render their instance masks, skipping invalid or unwanted detections, and creating a two-channel mask for panoptic segmentations.

Tools
Ruff

606-606: No explicit stacklevel keyword argument found

(B028)


Line range hint 704-737: LGTM!

The to_segmentation method correctly converts a polyline to a segmentation by parsing the segmentation target, initializing the mask, rendering the polyline onto the segmentation mask, and creating a two-channel mask for panoptic segmentations.


Line range hint 942-993: LGTM!

The to_segmentation method correctly converts polylines to a segmentation by parsing the mask targets, initializing the mask, iterating over the polylines to render them onto the segmentation mask, skipping unwanted polylines, and creating a two-channel mask for panoptic segmentations.


Line range hint 1053-1347: LGTM!

The Segmentation class provides a flexible and efficient representation of segmentations, supporting both semantic and panoptic types. It offers methods for importing, exporting, and transforming the segmentation mask, as well as converting the segmentation to detections and polylines for interoperability with other label types.


Line range hint 1185-1231: LGTM!

The to_detections method correctly converts a segmentation to detections, supporting both semantic and panoptic segmentations. It provides flexibility by allowing the specification of mask targets and mask types to control the conversion process and uses the _segmentation_to_detections function for a consistent and modular conversion.


Line range hint 1233-1284: LGTM!

The to_polylines method correctly converts a segmentation to polylines, supporting both semantic and panoptic segmentations. It provides flexibility by allowing the specification of mask targets, mask types, and tolerance to control the conversion process and uses the _segmentation_to_polylines function for a consistent and modular conversion.

seg = _make_1d_segmentation()
pseg = seg.to_panoptic()

class_mask = seg.mask
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused variable class_mask to tidy up code

The variable class_mask assigned at line 670 is not used elsewhere in the code. Removing it will improve code readability and eliminate potential warnings.

Apply this diff to remove the unused variable:

-class_mask = seg.mask
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
class_mask = seg.mask
Tools
Ruff

670-670: Local variable class_mask is assigned to but never used

Remove assignment to unused variable class_mask

(F841)

@brimoor brimoor changed the base branch from develop to panoptic-segmentation September 21, 2024 15:56
@benjaminpkane benjaminpkane self-requested a review October 3, 2024 14:58
Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

Hi @kemaleren. Proposal looks great! I will merge this and work through the App details. We can follow up offline about the other work you outlined

@benjaminpkane benjaminpkane merged commit 8c8427c into voxel51:panoptic-segmentation Oct 4, 2024
8 of 13 checks passed
@benjaminpkane benjaminpkane mentioned this pull request Oct 14, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants