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

IOU with segm masks and MAP for instance segment. #822

Merged
merged 166 commits into from May 24, 2022

Conversation

gianscarpe
Copy link
Contributor

@gianscarpe gianscarpe commented Feb 1, 2022

What does this PR do?

Fixes #821

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@Borda
Copy link
Member

Borda commented Feb 4, 2022

@gianscarpe mind resolve the conflict... 🐰

@gianscarpe
Copy link
Contributor Author

gianscarpe commented Feb 5, 2022

Any help with this error? It happens with ddp and I cannot figure out why @Borda

multiprocessing.pool.RemoteTraceback:                     
Traceback (most recent call last):                                                             
  File "/usr/lib/python3.8/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/usr/lib/python3.8/multiprocessing/pool.py", line 51, in starmapstar
    return list(itertools.starmap(args[0], args[1]))
  File "/home/gianlucascarpellini/dev/unsupervised-sensor-network/third_parties/metrics/tests/helpers/testers.py", line 219, in _class_test
    result = metric.compute()
  File "/home/gianlucascarpellini/dev/unsupervised-sensor-network/third_parties/metrics/torchmetrics/metric.py", line 374, in wrapped_func
    with self.sync_context(
  File "/usr/lib/python3.8/contextlib.py", line 113, in __enter__
    return next(self.gen)                                                                      
  File "/home/gianlucascarpellini/dev/unsupervised-sensor-network/third_parties/metrics/torchmetrics/metric.py", line 345, in sync_context
    self.sync(                                                                                 
  File "/home/gianlucascarpellini/dev/unsupervised-sensor-network/third_parties/metrics/torchmetrics/metric.py", line 297, in sync                                                            
    self._sync_dist(dist_sync_fn, process_group=process_group)                                                                                                                                
  File "/home/gianlucascarpellini/dev/unsupervised-sensor-network/third_parties/metrics/torchmetrics/metric.py", line 245, in _sync_dist                                                          if isinstance(output_dict[attr][0], Tensor):
IndexError: list index out of range

We can create an empty tensor for "detection_masks" and "groundtruth_masks" when the user does not provide them, in order to keep things working smoothly

@Borda Borda changed the title implementaed IOU with segmentation masks and MAP for instance segment… IOU with segm masks and MAP for instance segment. Feb 7, 2022
@Borda Borda added the enhancement New feature or request label Feb 7, 2022
@Borda Borda added this to the v0.8 milestone Feb 7, 2022
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #822 (7006e01) into master (85d798e) will decrease coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff          @@
##           master   #822   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         181    181           
  Lines        7937   7993   +56     
=====================================
+ Hits         7526   7568   +42     
- Misses        411    425   +14     

torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
torchmetrics/detection/map.py Outdated Show resolved Hide resolved
Copy link

@Red-Eyed Red-Eyed left a comment

Choose a reason for hiding this comment

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

Hello guys,
Let me add my 5 cents:

I just started to use this code, and it seems too complicated, maybe it would be better to create separate MAP for boxes and MAP for segments with common parent?

For example, I want to use segments, but I have to fill in boxes

If someone want to use both, he/she can create 2 metrics for boxes and for segments.

I maybe can contribute, If this idea seems fine

@SkafteNicki
Copy link
Member

Not an expert on the detection topic so just wanted to ask how many different common formats there exist?

@Red-Eyed
Copy link

Red-Eyed commented Feb 10, 2022

Not an expert on the detection topic so just wanted to ask how many different common formats there exist?

There are segmentation and bounding boxes (not rotated)

  1. Segmentation can be as a list of points [(x, y), (x, y)], or matrix (n,w,h), where n is number of planes, each plain represents single object binary mask

  2. boxes can usually represented in 3 ways: (x_center, y_center, width, height), (x_left, y_top, width, height), (x_left, y_top, x_right, y_bottom)

@Borda
Copy link
Member

Borda commented Feb 24, 2022

@gianscarpe @Red-Eyed @SkafteNicki how is it going here?

@mergify mergify bot added the has conflicts label Mar 1, 2022
@gianscarpe
Copy link
Contributor Author

@Borda I'll fix my PR and submit next week :)

@gianscarpe
Copy link
Contributor Author

Hello guys, Let me add my 5 cents:

I just started to use this code, and it seems too complicated, maybe it would be better to create separate MAP for boxes and MAP for segments with common parent?

For example, I want to use segments, but I have to fill in boxes

If someone want to use both, he/she can create 2 metrics for boxes and for segments.

I maybe can contribute, If this idea seems fine

The idea seems interesting. Let's see what @justusschock think about it! For the moment I have kept things as in the original pycoco implementation, where only one kind of iou_type

@mergify mergify bot added the ready label May 17, 2022
@gianscarpe
Copy link
Contributor Author

Hi @Borda @SkafteNicki could you check the failing test?

        areas = compute_area(gt, iou_type=self.iou_type).to(self.device)
    
>       ignore_area = torch.logical_or(areas < area_range[0], areas > area_range[1])
E       RuntimeError: value cannot be converted to type int without overflow

torchmetrics\detection\mean_ap.py:583: RuntimeError

I'm trying to fix this but it happens only on windows. This should be a boolean value, so why overflow error? Any ideas?

@SkafteNicki
Copy link
Member

Hi @gianscarpe,
I tried changing:

area = torch.tensor(mask_utils.area(input).astype("int"))

to

area = torch.tensor(mask_utils.area(input).astype("float"))

and hope that fixed the problem :]

@Borda
Copy link
Member

Borda commented May 19, 2022

if all is fine now, I ll move the data to S3

@SkafteNicki
Copy link
Member

@Borda go ahead and do that, test are passing on windows now :]

@Borda Borda enabled auto-merge (squash) May 19, 2022 16:40
@Borda Borda requested a review from SkafteNicki May 19, 2022 16:40
Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
@Borda
Copy link
Member

Borda commented May 19, 2022

@Borda go ahead and do that, test are passing on windows now :]

shall be all set now :)

@mergify mergify bot added has conflicts and removed ready labels May 19, 2022
@gianscarpe
Copy link
Contributor Author

Update: Seems like it's not installing pycocotools from detection requirements

@mergify mergify bot removed the has conflicts label May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IOU type for mean average precision
6 participants