Add `VIAMaskParser` for parsing polygons as mask annotations. #593

rsomani95 · 2020-12-16T11:21:39Z

In order to do this, I had to remove VIABBoxParser's ability to parse bounding boxes from both rect and polygon shape attributes and assume that rect annotations are for bounding boxes and polygon annotations for masks. If not, then the polygon annotations were added as an additional bbox and this didn't allow it to be parsed due to length mismatch betwen number of boxes and masks.

I've left some of the older code commented out rather than delete it, will be happy to clean that up after some feedback.

Move some code from `VIABaseParser` to `VIABBoxParser` and add the assumption that bounding boxes are to be parsed from the `rect` attribute only

rsomani95 · 2020-12-16T11:23:15Z

Qualitative output check:

lgvaz

An option that we have is to keep the bbox parser able to handle both "polygon" and "rect" and overwrite the function bboxes to only parse rect on the mask parser. (I'm just throwing this out there, I'm not sure this would be the best strategy)

How common it's to have bboxes as polygon? I think this is a specific case because @jerbly originally had rotated bboxes on his dataset.

@jerbly what do you think?

lgvaz · 2020-12-16T13:39:27Z

icevision/parsers/via_parser.py

+
+                    # create empty image and fill it with the polygon
+                    # mask_img = PIL.Image.new("L", get_image_size(self.filepath(o)), 0)
+                    PIL.ImageDraw.Draw(mask_img).polygon(xy_coords, outline=1, fill=1)


Instead of returning a image mask, can we directly return a Polygon class?

The current Polygon class requires points to be vertices, like the COCO format. We'll need to convert x,y coordinate pairs to vertices to do that. I'm not sure how to do this, but I can look around for solutions

rsomani95 · 2020-12-16T14:16:03Z

An option that we have is to keep the bbox parser able to handle both "polygon" and "rect" and overwrite the function bboxes to only parse rect on the mask parser. (I'm just throwing this out there, I'm not sure this would be the best strategy)

I think it would be good to have that option. We can have the following scenarios:

(As you described) Using only VIABBoxParser -- parse from either rect or polygon (not both..?)
Using VIAMaskParser with only polygon annotations -- derive bboxes and masks both from the same polygon annotation (could save a user a lot of time when annotating)
Using VIAMaskParser with both rect and polygon annotations -- this is what's been implemented in this PR as of now

rsomani95 · 2020-12-16T17:33:27Z

@jerbly I'm unable to load the test via_bbox.json file. I'm using via-2.0.10. On trying to load the project, I get a Cannot import project from a corrupt file at the bottom of the browser.

jerbly · 2020-12-16T18:52:41Z

An option that we have is to keep the bbox parser able to handle both "polygon" and "rect" and overwrite the function bboxes to only parse rect on the mask parser. (I'm just throwing this out there, I'm not sure this would be the best strategy)

As a user you're already selecting mask=True so why not just push the shape parsing down to the specific class and continue to support polys converted to bboxes. This would also allow you to convert rects to masks!

jerbly · 2020-12-16T18:53:58Z

@jerbly I'm unable to load the test via_bbox.json file. I'm using via-2.0.10. On trying to load the project, I get a Cannot import project from a corrupt file at the bottom of the browser.

via_bbox.json is an annotation export file, not a project file. Try importing the annotation file.

icevision/parsers/via_parser.py

`polygon` annotations. The choice is left to the user and defaults are set to maintain backward compatibility. Masks from `rect` annotations, tests yet to be implemented

rsomani95 · 2020-12-17T05:50:54Z

Made some heavy-ish changes based on the feedback. The parser is now quite flexible, bbox and mask annotations can be derived from both rect and/or polygon annotations. The choice is left entirely to the user and defaults have been set to maintain previous behavior.

I've also separated the logic to create BBox or MaskArray from the annotations into a separate function so we can tweak that more easily in the future if needed. Masks from rect annotations are yet to be implemented.

Curious to hear what you guys think :)

EDIT: I need to write unit tests to cover the added functionality -- that's a WIP

codecov · 2020-12-17T05:52:37Z

Codecov Report

Merging #593 (6d966f1) into master (6cbd6d1) will decrease coverage by 0.74%.
The diff coverage is 65.67%.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   88.15%   87.41%   -0.75%     
==========================================
  Files         113      113              
  Lines        2694     2749      +55     
==========================================
+ Hits         2375     2403      +28     
- Misses        319      346      +27

Flag	Coverage Δ
unittests	`87.41% <65.67%> (-0.75%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
icevision/parsers/via_parser.py	`74.76% <65.67%> (-25.24%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cbd6d1...b0df677. Read the comment docs.

jerbly · 2020-12-19T03:11:46Z

I'm considering adding keypoint parsing for VIA too so this gets pretty messy. There comes a point where the factory function is more cumbersome than just instantiating the concrete class yourself. So I think either remove the factory function or do something like this...

Instead of all the boolean flags in the factory function we should use two enums. One for ParserType which IMO should be moved up into the parser.py for all parsers. This can be used to fix the factory function coco too. (Which hasn't been extended to include keypoints @lgvaz ). A second enum VIAShape specifically describes the shapes. The user can then provide a set of shapes to exclude from the input.

Something like this:

from enum import Enum
VIAShape = Enum('VIAShape', [('POLYGON', 'polygon'), ('RECT', 'rect'), ('POINT', 'point'),
                             ('POLYLINE', 'polyline'), ('ELLIPSE', 'ellipse'), ('CIRCLE', 'circle')])
ParserType = Enum('ParserType', 'BBOX MASK KEYPOINT')

def via(
    annotations_file: Union[str, Path],
    img_dir: Union[str, Path],
    class_map: ClassMap,
    label_field: str = "label",
    parser_type: ParserType = ParserType.BBOX,
    exclude_shapes: set = None
) -> Parser:
    if parser_type is ParserType.BBOX:
        parser_cls = VIABBoxParser
    elif parser_type is ParserType.MASK:
        parser_cls = VIAMaskParser
    else:
        raise NotImplementedError()
    return parser_cls(annotations_file, img_dir, class_map, label_field, exclude_shapes)

As a user, if I wanted a mask parser that ignored rect shape attributes in the via file I would call this:

p = via('file', 'dir', class_map, parser_type = ParserType.MASK, exclude_shapes = {VIAShape.RECT})

Then in the parser classes you can get the union set of not-supported-shapes and excluded-shapes to determine which shapes to parse. Oh, and by using the string for the enum value in VIAShape you can do things like this:

shape = VIAShape(shape_attr["name"])
if shape not in exclude_shapes:
...

lgvaz · 2020-12-22T10:02:35Z

I'm up for removing the factory function and just instantiating the class, let's go with that?

jerbly · 2020-12-22T17:25:57Z

I'm up for removing the factory function and just instantiating the class, let's go with that?

Yes. I think that's much cleaner. Also it means we don't need BBoxFrom and MaskFrom since the contract is now just in the concrete class. I think it means a lot of code can be deleted! (MY favourite type of coding).

lgvaz · 2020-12-22T20:51:43Z

a lot of code can be deleted! (MY favourite type of coding).

Specially when it's untested code, then I just see the coverage percentages going up, really nice! hahaaah

Let's do that then. @rsomani95 what do you think?

rsomani95 · 2020-12-24T06:26:42Z

Concur, I think the Enum approach + instantiating from class is much cleaner

lgvaz · 2021-01-05T15:14:37Z

Hi hi, I hope you both had a great new year!

I'm returning back to this, @rsomani95 should I go and make the changes or you would like to do it?

I think the Enum approach + instantiating from class is much cleaner

Just to be clear, I think if we're instantiating directly from the class we don't need Enums anymore

rsomani95 · 2021-01-05T17:08:26Z

Hi @lgvaz, thanks. hope you had a good one too!
I’m a bit caught up with some tasks at work so feel free to make the changes if you’d like :)

jerbly · 2021-01-05T18:53:25Z

@lgvaz - I think there's potentially still value in the VIAShape enum to tidy up the parsing and to allow the user to optionally specify shapes to exclude. But, perhaps that's an extension from initial mask support for another PR some day?

lgvaz · 2021-01-06T00:31:54Z

@jerbly I'm a bit confused about exclude_shapes, in the example you provided we have:

...parser_type = ParserType.MASK, exclude_shapes = {VIAShape.RECT})

If I understood correctly this would not parse bboxes, but to train the segmentation models we need them. Maybe what you have in mind is that if rect gets remove we calculate the bboxes from the masks?

jerbly · 2021-01-06T01:09:49Z

@lgvaz - no, that's not what I intended. The VIA spec allows you to mark up multiple shapes like rectangles, circles, ellipses etc. The exclude_shapes set was intended to allow you to take a dataset but only parse a subset of the shapes. This is independent of the parsing type be that bboxes, mask or keypoints. Think of shapes as the input and bbox/mask/keypoint as the output.

I can have a go at a PR for this if that makes more sense? I'm just a little short on time this week.

lgvaz · 2021-01-07T11:48:12Z

@jerbly - Aaah okay, gotcha!!

Let's do that as a subsequent PR as you suggested, I'll work and merge this one this week and then you can open a new PR next week, what do you think?

jerbly · 2021-01-07T13:48:40Z

@lgvaz - sure, I can if you think there's value in it?

lgvaz · 2021-01-08T11:19:05Z

@jerbly - I don't have a definitive answer right now, let me explore VIA a little bit more this weekend while I work on this PR, then I can provide you with a better answer without BSing 😅

lgvaz · 2021-01-12T14:17:52Z

I'm falling behind on this, but just want to let you know that I didn't forget, I'll get back to this after mmdetection support has been tackled

rsomani95 · 2021-04-22T16:43:56Z

@lgvaz @jerbly closing this as it's quite outdated with the new record API :)

Add VIAMaskParser for parsing polygons as mask annotations.

69cb47f

Move some code from `VIABaseParser` to `VIABBoxParser` and add the assumption that bounding boxes are to be parsed from the `rect` attribute only

uncomment accidentally commented line

c13f5b0

lgvaz reviewed Dec 16, 2020

View reviewed changes

jerbly reviewed Dec 16, 2020

View reviewed changes

icevision/parsers/via_parser.py Outdated Show resolved Hide resolved

rsomani95 added 2 commits December 17, 2020 08:47

fix backward compatibility

3932668

Add ability to parse both bboxes and masks from both rect and/or

b0df677

`polygon` annotations. The choice is left to the user and defaults are set to maintain backward compatibility. Masks from `rect` annotations, tests yet to be implemented

lgvaz mentioned this pull request Jan 12, 2021

Default parsers api refactor #606

Closed

rsomani95 closed this Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `VIAMaskParser` for parsing polygons as mask annotations. #593

Add `VIAMaskParser` for parsing polygons as mask annotations. #593

rsomani95 commented Dec 16, 2020

rsomani95 commented Dec 16, 2020

lgvaz left a comment

lgvaz Dec 16, 2020

rsomani95 Dec 16, 2020

rsomani95 commented Dec 16, 2020

rsomani95 commented Dec 16, 2020

jerbly commented Dec 16, 2020

jerbly commented Dec 16, 2020

rsomani95 commented Dec 17, 2020 •

edited

codecov bot commented Dec 17, 2020 •

edited

jerbly commented Dec 19, 2020

lgvaz commented Dec 22, 2020

jerbly commented Dec 22, 2020

lgvaz commented Dec 22, 2020

rsomani95 commented Dec 24, 2020

lgvaz commented Jan 5, 2021

rsomani95 commented Jan 5, 2021

jerbly commented Jan 5, 2021

lgvaz commented Jan 6, 2021

jerbly commented Jan 6, 2021

lgvaz commented Jan 7, 2021

jerbly commented Jan 7, 2021

lgvaz commented Jan 8, 2021

lgvaz commented Jan 12, 2021

rsomani95 commented Apr 22, 2021

Add VIAMaskParser for parsing polygons as mask annotations. #593

Add VIAMaskParser for parsing polygons as mask annotations. #593

Conversation

rsomani95 commented Dec 16, 2020

rsomani95 commented Dec 16, 2020

lgvaz left a comment

Choose a reason for hiding this comment

lgvaz Dec 16, 2020

Choose a reason for hiding this comment

rsomani95 Dec 16, 2020

Choose a reason for hiding this comment

rsomani95 commented Dec 16, 2020

rsomani95 commented Dec 16, 2020

jerbly commented Dec 16, 2020

jerbly commented Dec 16, 2020

rsomani95 commented Dec 17, 2020 • edited

codecov bot commented Dec 17, 2020 • edited

Codecov Report

jerbly commented Dec 19, 2020

lgvaz commented Dec 22, 2020

jerbly commented Dec 22, 2020

lgvaz commented Dec 22, 2020

rsomani95 commented Dec 24, 2020

lgvaz commented Jan 5, 2021

rsomani95 commented Jan 5, 2021

jerbly commented Jan 5, 2021

lgvaz commented Jan 6, 2021

jerbly commented Jan 6, 2021

lgvaz commented Jan 7, 2021

jerbly commented Jan 7, 2021

lgvaz commented Jan 8, 2021

lgvaz commented Jan 12, 2021

rsomani95 commented Apr 22, 2021

Add `VIAMaskParser` for parsing polygons as mask annotations. #593

Add `VIAMaskParser` for parsing polygons as mask annotations. #593

rsomani95 commented Dec 17, 2020 •

edited

codecov bot commented Dec 17, 2020 •

edited