Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implementation for cityscapes in proto datasets #5441
base: main
Are you sure you want to change the base?
Implementation for cityscapes in proto datasets #5441
Changes from 5 commits
1ccf7b0
e003b71
440fbdb
f35b87b
0a3558d
de25fb5
4fdeb31
41d6c47
19dbebf
5c8850b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in
extra=dict(classname_to_details=self.categories_to_details),
and is exposed to the user as in the previously implementationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is: what does the user do with this information? I reckon
color
is useful to parse a segmentation mask. Thecategory
looks like what we calledsuper_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theSegmentationMask
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, we should aim for functions that have only a single responsibility. Thus, we should have one
Filter
that picks the right folder and one that removes all non-image files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had that in mind but then thought about performance overhead we bring by that granularity, I'll measure the diff in times later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the granularity given that the performance overhead from this hardly will make any difference. cc @NicolasHug for an opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree there should be no perf difference.
But is the granularity needed here? Unless the 2 new proposed filters are being used somewhere else in the code, there's probably no need to split this 4-liner into 2 separate functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go for granularity, we don't need any extra function. The current
is equivalent to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the archive again, I think we don't even need the second
Filter
here. The only files that are not annotations are located in the parent folder of the split folders. Thus, if we drop every file that is not contained in the split folder we are after, we should only get png images and json files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's work also without filtering on file extension. Archive contains files like :
/cityscapes/leftImg8bit_trainvaltest.zip/README
/cityscapes/leftImg8bit_trainvaltest.zip/license.txt
for which computed split is
cityscapes
and thus filtered as does not matchtrain
,val
etcSuch implicit filtering works but less evident and either we have to leave a comment about that or keep filtering by the extension. I'd keep filtering by extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder for self: we need an
EncodedSegmentationMask
feature now that we also have a separateSegementationMask
.