Implementation for cityscapes in proto datasets #5441
Conversation
💊 CI failures summary and remediationsAs of commit 5c8850b (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
vfdev-5
force-pushed
the
fix-5353-add-cityscapes
branch
2 times, most recently
from
b94bb3d to
7b40f9c
Compare
vfdev-5
force-pushed
the
fix-5353-add-cityscapes
branch
5 times, most recently
from
b3f4c0a to
71e0d26
Compare
vfdev-5
force-pushed
the
fix-5353-add-cityscapes
branch
from
71e0d26 to
e003b71
Compare
vfdev-5
force-pushed
the
fix-5353-add-cityscapes
branch
from
064e02e to
440fbdb
Compare
|
|
||
| categories_to_details: FrozenMapping = FrozenMapping( | ||
| { | ||
| "unlabeled": CityscapesClass("unlabeled", 0, 255, "void", 0, False, True, (0, 0, 0)), |
There was a problem hiding this comment.
It seems, all of this information is exposed to the user, but never used internally. Is this just static information or would it make sense to include it in the samples?
There was a problem hiding this comment.
It is used in extra=dict(classname_to_details=self.categories_to_details), and is exposed to the user as in the previously implementation
There was a problem hiding this comment.
My point is: what does the user do with this information? I reckon color is useful to parse a segmentation mask. The category looks like what we called super_category in COCO. So maybe we can just provide two dictionaries in the extra information that map the category to its color and super category respectively?
There was a problem hiding this comment.
CityscapesClass provides more info that just color, category name, super category. It may happen that users could use other fields for visualization purposes and thus new dataset would miss that info.
There was a problem hiding this comment.
In the stable API this is exposed through the undocumented torchvision.datasets.Cityscapes(...).classes attribute. Given that the new API is BC breaking anyway, IMO we should aim to reduce the baggage from the old implementation. I indeed see use cases for returning a color to category mapping (maybe we should have this on the SegmentationMask feature) as well as the category to super category mapping. For the rest I would remove them for now and only put them back if someone requests it. cc @NicolasHug for an opinion.
There was a problem hiding this comment.
It is dataset meta-data, probably coming from https://github.com/mcordts/cityscapesScripts and is used for visualization (this can be seen by searching through the GitHub and exploring repository by repository, https://github.com/search?p=1&q=CityscapesClass&type=Code). Even if this wasn't explicitly documented previously, I do not think that we have to remove that in the new API.
There was a problem hiding this comment.
I would remove them for now and only put them back if someone requests it.
I'm happy with that.
|
@pmeier thanks for the review |
|
|
||
| categories_to_details: FrozenMapping = FrozenMapping( | ||
| { | ||
| "unlabeled": CityscapesClass("unlabeled", 0, 255, "void", 0, False, True, (0, 0, 0)), |
There was a problem hiding this comment.
My point is: what does the user do with this information? I reckon color is useful to parse a segmentation mask. The category looks like what we called super_category in COCO. So maybe we can just provide two dictionaries in the extra information that map the category to its color and super category respectively?
| def _filter_split_images(self, data: Tuple[str, Any], *, req_split: str) -> bool: | ||
| path = Path(data[0]) | ||
| split = path.parent.parts[-2] | ||
| return split == req_split and ".png" == path.suffix |
There was a problem hiding this comment.
I would prefer the granularity given that the performance overhead from this hardly will make any difference. cc @NicolasHug for an opinion.
| if target_type == "polygon": | ||
| enc_data = json.loads(data.read()) | ||
| else: | ||
| enc_data = EncodedImage.from_file(data) |
There was a problem hiding this comment.
Reminder for self: we need an EncodedSegmentationMask feature now that we also have a separate SegementationMask.
Fixes #5353
Related to #5336
Description: