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 masks to boundaries #7704

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Add masks to boundaries #7704

wants to merge 26 commits into from

Conversation

bhack
Copy link

@bhack bhack commented Jun 27, 2023

Fixes: #7537

How do you would impl the test against:
https://github.com/bowenc0221/boundary-iou-api/blob/master/boundary_iou/utils/boundary_utils.py#L12-L30

I suppose we don't want to add python OpenCV as a test dependecy.

@pytorch-bot
Copy link

pytorch-bot bot commented Jun 27, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7704

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link

Hi @bhack!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bhack , I'll take a deeper look later.

How do you would impl the test against

Yeah, we can't have openCV on the tests suite. Maybe we can create a custom tests where we draw simple masks e.g. circles or squares, fill them in, and then assert in the test that the output of masks_to_boundaries corresponds to the contour shape?

@@ -382,7 +382,39 @@ def _box_diou_iou(boxes1: Tensor, boxes2: Tensor, eps: float = 1e-7) -> Tuple[Te
# distance between boxes' centers squared.
return iou - (centers_distance_squared / diagonal_distance_squared), iou

def masks_to_boundaries(masks: torch.Tensor, dilation_ratio: float = 0.02) -> torch.Tensor:
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's OK to have the implementation in this file even though this isn't related to boxed. However, I don't think we should expose it here. I think we should just expose it in from the torchvision.ops namespace (otherwise the implementation will always have to stay in this file for BC, and that may lock us).

We probably just need to rename this to _masks_to_boundaries and the expose it in torchvision.ops.__init__.py like

from .boxes import import _masks_to_boundaries as masks_to_boundaries

Any other suggestion @pmeier @vfdev-5 @oke-aditya ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it's OK to have the implementation in this file even though this isn't related to boxed.

No strong opinion, but could we maybe also have a new _masks.py module or move it into the misc.py one?

👍 for only exposing it in the torchvision.ops namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh there is demand for mask_utils. Several of them, #4415 . Candidate utils like convert_masks_format, paste_masks_in_images, etc. Maybe it's time to create new files mask_utils.py and make future extensions possible?

Copy link
Member

Choose a reason for hiding this comment

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

we can always create an ops.mask* namespace at any time. We should only do that when we know for sure we need it, i.e. when we start having 2+ mask utils. Alls ops are exposed in the ops. namespace anyway so there's no need to rush and create a file which will only have one single util in it ATM.

I'm OK with creating _mask.py as well (and we can rename it into mask.py later if we want to).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with creating _mask.py as well (and we can rename it into mask.py later if we want to).

This sounds best solution! We can avoid the bloat inside this file as well as keep them private 😄

Comment on lines 403 to 416
n, h, w = masks.shape
img_diag = math.sqrt(h ** 2 + w ** 2)
dilation = int(round(dilation_ratio * img_diag))
selem_size = dilation * 2 + 1
selem = torch.ones((n, 1, selem_size, selem_size), device=masks.device)

# Compute the boundaries for each mask
masks = masks.float().unsqueeze(1)
eroded_masks = F.conv2d(masks, selem, padding=dilation, groups=n)
eroded_masks = (eroded_masks == selem.view(n, -1).sum(1, keepdim=True)).byte() # Make the output binary

contours = masks.byte() - eroded_masks

return contours.squeeze(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think this code works as expected. Here is my test example and it fails in multiple places:

import torch
import numpy as np
from PIL import ImageDraw, Image

mask = torch.zeros(4, 32, 32, dtype=torch.bool)
mask[0, 1:10, 1:10] = True
mask[0, 12:20, 12:20] = True
mask[0, 15:18, 20:32] = True

mask[1, 15:23, 15:23] = True
mask[1, 22:33, 22:33] = True

mask[2, 1:5, 22:30] = True
mask[2, 5:14, 25:27] = True


pil_img = Image.new("L", (32, 32))

draw = ImageDraw.Draw(pil_img)
draw.ellipse([2, 7, 26, 26], fill=1, outline=1, width=1)

mask[3, ...] = torch.from_numpy(np.asarray(pil_img))


import math
from torch.nn import functional as F

dilation_ratio = 0.05
masks = mask.clone()

n, h, w = masks.shape
img_diag = math.sqrt(h ** 2 + w ** 2)
dilation = int(round(dilation_ratio * img_diag))
selem_size = dilation * 2 + 1
selem = torch.ones((n, 1, selem_size, selem_size), device=masks.device)


# Compute the boundaries for each mask
masks = masks.float().unsqueeze(1)
eroded_masks = F.conv2d(masks, selem, padding=dilation, groups=n)
eroded_masks = (eroded_masks == selem.view(n, -1).sum(1, keepdim=True)).byte()  # Make the output binary

contours = masks.byte() - eroded_masks
contours. = contours.squeeze(1)

Error:

---> 17 eroded_masks = (eroded_masks == selem.view(n, -1).sum(1, keepdim=True)).byte()  # Make the output binary

RuntimeError: The size of tensor a (32) must match the size of tensor b (4) at non-singleton dimension 2

Masks:
image

Error is related to masks = masks.float().unsqueeze(1) where we may need to unsqueeze(0) instead.
But if fixed like that, the next line does not make much sense IMO:

eroded_masks = (eroded_masks == selem.view(n, -1).sum(1, keepdim=True)).byte()

as eroded_masks shape wont match the size of conv weights...

Sorry, if I'm missing something...

Copy link
Author

Choose a reason for hiding this comment

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

What do you think about:

import torch
import numpy as np
from PIL import ImageDraw, Image
import math
from torch.nn import functional as F
import matplotlib.pyplot as plt

mask = torch.zeros(4, 32, 32, dtype=torch.bool)
mask[0, 1:10, 1:10] = True
mask[0, 12:20, 12:20] = True
mask[0, 15:18, 20:32] = True

mask[1, 15:23, 15:23] = True
mask[1, 22:33, 22:33] = True

mask[2, 1:5, 22:30] = True
mask[2, 5:14, 25:27] = True

pil_img = Image.new("L", (32, 32))
draw = ImageDraw.Draw(pil_img)
draw.ellipse([2, 7, 26, 26], fill=1, outline=1, width=1)
mask[3, ...] = torch.from_numpy(np.asarray(pil_img))

dilation_ratio = 0.05
masks = mask.clone()

n, h, w = masks.shape
img_diag = math.sqrt(h ** 2 + w ** 2)
dilation = int(round(dilation_ratio * img_diag))
selem_size = dilation * 2 + 1
selem = torch.ones((1, 1, selem_size, selem_size), device=masks.device)

# Compute the boundaries for each mask
masks = masks.float().unsqueeze(1)
eroded_masks = torch.zeros_like(masks)

#for i in range(n):
#    eroded_masks[i] = F.conv2d(masks[i].unsqueeze(0), selem, padding=dilation)
eroded_masks = F.conv2d(masks, selem, padding=dilation)

eroded_masks = (eroded_masks == selem.view(-1).sum()).byte()  # Make the output binary
contours = masks.byte() - eroded_masks
contours = contours.squeeze(1)

# Visualize the results
fig, ax = plt.subplots(n, 3, figsize=(10, 10))

for i in range(n):
    ax[i, 0].imshow(mask[i], cmap='gray')
    ax[i, 1].imshow(eroded_masks[i].squeeze(), cmap='gray')
    ax[i, 2].imshow(contours[i], cmap='gray')

plt.show()

immagine

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bhack why do we need dilation_ratio ? I think we can do the following without extra parametrization:

masks = masks.float().unsqueeze(1)
w_size = 3
w = torch.ones((1, 1, w_size, w_size), device=masks.device) / (w_size ** 2)
eroded_masks = F.conv2d(masks, w, padding=1)
contours = (masks - eroded_masks) > 0
contours = contours.squeeze(1)

what do you think ?

Copy link
Author

Choose a reason for hiding this comment

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

It is in the paper official implementation

https://github.com/bowenc0221/boundary-iou-api/blob/master/boundary_iou/utils/boundary_utils.py#L12

But also in the more classical F score (Davis dataset/challenge official eval kit).

https://github.com/davisvideochallenge/davis2017-evaluation/blob/master/davis2017/metrics.py#L57

As this is often a preprocessing step used in the boundary overlapping metrics (BoundaryIOU/Boundary F-Score) the dilate will give the control over the tolerance of the exact boundaries overlapping of the boundaries.

In both the papers they talked about bipartite graph matching but then they have always approximated with morphological ops.

Of you see the F/Davis case impl there is also an option where the tolerance/dilate Is defined by the input resolution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the links. According to https://github.com/davisvideochallenge/davis2017-evaluation/blob/master/davis2017/metrics.py#L57 code, mask to boundary is done without using any parameters, see _seg2bmap:
https://github.com/davisvideochallenge/davis2017-evaluation/blob/ac7c43fca936f9722837b7fbd337d284ba37004b/davis2017/metrics.py#L122
Anyway, I see why they have dilation_ratio arg.

However, previously I missed the issue description and the context for this PR:

A mask to boundary API is useful for implementing many segmentation metrics used in many dataset and challenges (Davis F score, BoundaryIOU, etc..).
It could be also used more generally for visualization tasks.

In this case, I'm not very sure about torchvision's interest in following line by line what does https://github.com/bowenc0221/boundary-iou-api as 1) IMO we wont be able to reproduce cv2.erode behaviour and 2) as such helper function can be used within a metric implementation, it should be carefully tested vs ref implementation in a lot of corner cases etc (and this is not the role of torchvision, IMO).

In general, a method to produce mask to edges (sort of edge detector) could make sense like mask to bboxes.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the links. According to https://github.com/davisvideochallenge/davis2017-evaluation/blob/master/davis2017/metrics.py#L57 code, mask to boundary is done without using any parameters, see _seg2bmap:
https://github.com/davisvideochallenge/davis2017-evaluation/blob/ac7c43fca936f9722837b7fbd337d284ba37004b/davis2017/metrics.py#L122

Yes but cause in F they are dilating in an extra post-processing step in the metric instead of the BoundariesIOU approach (see dilate disk param)
https://github.com/davisvideochallenge/davis2017-evaluation/blob/master/davis2017/metrics.py#L77

In this case, I'm not very sure about torchvision's interest in following line by line what does https://github.com/bowenc0221/boundary-iou-api as 1) IMO we wont be able to reproduce cv2.erode behaviour and 2) as such helper function can be used within a metric implementation, it should be carefully tested vs ref implementation in a lot of corner cases etc (and this is not the role of torchvision, IMO).

I've tested another early implementation with some inputs but the Boundary IOU paper reference impl doesn't have a test suite.

In general, a method to produce mask to edges (sort of edge detector) could make sense like mask to bboxes.

Let me know as I am mainly interested to achieve the metric and eventually to contribute also an intermediate function here in the case it could be compatible and useful for other contexts/domain.

Copy link
Author

@bhack bhack Jun 29, 2023

Choose a reason for hiding this comment

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

I see you are also a member of the MONAI project so you have already something similar but it still rely on a non-Pytorch implementation:
https://github.com/Project-MONAI/MetricsReloaded/blob/main/MetricsReloaded/metrics/pairwise_measures.py#L963

@bhack
Copy link
Author

bhack commented Jul 1, 2023

import torch
import numpy as np
from PIL import ImageDraw, Image
import math
from torch.nn import functional as F
import matplotlib.pyplot as plt

# Create masks
mask = torch.zeros(4, 32, 32, dtype=torch.bool)
mask[0, 1:10, 1:10] = True
mask[0, 12:20, 12:20] = True
mask[0, 15:18, 20:32] = True
mask[1, 15:23, 15:23] = True
mask[1, 22:33, 22:33] = True
mask[2, 1:5, 22:30] = True
mask[2, 5:14, 25:27] = True
pil_img = Image.new("L", (32, 32))
draw = ImageDraw.Draw(pil_img)
draw.ellipse([2, 7, 26, 26], fill=1, outline=1, width=1)
mask[3, ...] = torch.from_numpy(np.asarray(pil_img))

# Define dilation_ratio
dilation_ratio = 0.02

# Clone masks
masks = mask.clone()

# Get the dimensions
n, h, w = masks.shape

# Compute img_diag, dilation, selem_size and selem
img_diag = math.sqrt(h ** 2 + w ** 2)
dilation = int(round(dilation_ratio * img_diag))
selem_size = dilation * 2 + 1
selem = torch.ones((n, 1, selem_size, selem_size), device=masks.device)

# Compute the boundaries for each mask
masks = masks.float().unsqueeze(1)
eroded_masks = F.conv2d(masks, selem, padding=dilation)
eroded_masks = (eroded_masks == selem.view(n, -1).sum(-1).view(n, 1, 1, 1)).byte()  # Make the output binary

contours = masks.byte() - eroded_masks

# Squeeze the contours tensor
contours = contours.squeeze(1)

# Visualize the results
fig, ax = plt.subplots(n, 3, figsize=(10, 10))
for i in range(n):
    ax[i, 0].imshow(mask[i], cmap='gray')
    ax[i, 1].imshow(eroded_masks[i, 0].cpu(), cmap='gray')
    ax[i, 2].imshow(contours[i, 0].cpu(), cmap='gray')

plt.show()

immagine

test/test_ops.py Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@ The below operators perform pre-processing as well as post-processing required i

batched_nms
masks_to_boxes
masks_to_boudnaries
Copy link
Author

Choose a reason for hiding this comment

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

Fixed. So what we want to do?

@bhack
Copy link
Author

bhack commented Nov 13, 2023

Any news on this? Are you still interested?

@bhack bhack marked this pull request as ready for review December 30, 2023 12:51
@bhack
Copy link
Author

bhack commented Feb 15, 2024

Gently ping

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, but the implementation still has some problems. I left comments in the code.

torchvision/ops/boxes.py Outdated Show resolved Hide resolved
torchvision/ops/boxes.py Outdated Show resolved Hide resolved
torchvision/ops/boxes.py Outdated Show resolved Hide resolved
Refactor test and add debug image util
Refactor implementation
@bhack bhack requested a review from vfdev-5 February 17, 2024 01:08
@bhack
Copy link
Author

bhack commented Mar 5, 2024

@NicolasHug Gently ping.

@bhack
Copy link
Author

bhack commented Apr 29, 2024

Let me know if we want to close this as we are at the 10th month.

@bhack
Copy link
Author

bhack commented Sep 15, 2024

Ping again, we are over 1 year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mask to boundary API
6 participants