Open Images: add segmentation mask support #388
Conversation
|
|
||
| from datumaro.cli.util import make_file_name |
There was a problem hiding this comment.
I don't think importing from cli is good here. Probably, this function is better be moved to the core utils (and it is done in the versioning PR).
There was a problem hiding this comment.
Yeah, fair enough. I'll probably steal your move patch (unless you want to submit it as a separate PR).
|
|
||
| if box is not None: | ||
| if item.has_image and item.image.size is not None: | ||
| image_meta[item.id] = (height, width) = item.image.size |
There was a problem hiding this comment.
I discourage using multiple assignment pattern.
There was a problem hiding this comment.
Basically, for the same reason we don't write lines like do_one(); do_another(); x = foo(); y = bar();. This make the code clearer and reduces load on the reader.
There was a problem hiding this comment.
It's not quite the same thing, because it reduces duplication. But eh, I can change it if you want.
| if all(mask_description[f] for f in [ | ||
| 'BoxXMin' ,'BoxXMax', 'BoxYMin', 'BoxYMax', | ||
| ]): | ||
| box = attributes['box'] = {} |
There was a problem hiding this comment.
Is there any reason to include it as an attribute? Can these values be distinct from what is in the mask? If possible, let's stick to simple type attributes.
If you want to have a bbox attached to a mask, the current approach is to have it as another annotation in the list with the same group as the corresponding mask and no label. It complicates processing a bit, though, check COCO converter or https://github.com/openvinotoolkit/datumaro/blob/develop/datumaro/util/annotation_util.py#L15. I'd vote for exclusion of this information completely, because it can be computed without great overhead, and masks have such method.
There was a problem hiding this comment.
Is there any reason to include it as an attribute? Can these values be distinct from what is in the mask?
To quote the Open Images website:
"Note that this is not the bounding box of the mask, but the starting box from which the mask was annotated. These coordinates can be used to relate the mask data with the boxes data."
If you want to have a bbox attached to a mask, the current approach is to have it as another annotation in the list with the same group as the corresponding mask and no label.
Okay, that seems quite doable. I can assign a distinct group to every box annotation and then find the matching box annotation for every mask annotation when loading them.
I could, of course, just not load this information at all, but according to the dataset description, it's not something that just can be computed, so it seems useful to have.
There was a problem hiding this comment.
Okay, that seems quite doable. I can assign a distinct group to every box annotation and then find the matching box annotation for every mask annotation when loading them.
Implemented this.
It's useful in more than just CLI. Patch shamelessly stolen from Maxim Zhiltsov's WIP versioning PR. :-)
| box_id = mask_description['BoxID'] | ||
| if _RE_INVALID_PATH_COMPONENT.fullmatch(box_id): | ||
| raise UnsupportedBoxIdError(item_id=item.id, box_id=box_id) | ||
| attributes['box_id'] = box_id |
There was a problem hiding this comment.
Maybe these ids should be mask ids?
There was a problem hiding this comment.
They're called box IDs in the dataset; I don't think we need to rename them. 🤷
tests/test_open_images_format.py
Outdated
| Mask(label=0, image=np.eye(8)), | ||
| Mask(label=1, image=np.ones((8, 8)), group=1, attributes={ | ||
| 'box_id': '00000000', | ||
| 'box': {'x': 2, 'y': 3, 'w': 6, 'h': 1}, |
There was a problem hiding this comment.
| 'box': {'x': 2, 'y': 3, 'w': 6, 'h': 1}, |
Instead of using a `box` attribute, group masks with boxes using group IDs.
Summary
Yet another step to implementing #274.
How to test
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.