-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Conversation
merge from fb
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.
Thanks a lot for the PR, it will definitely be very helpful to the community!
I have left a few comments, let me know what you think.
The biggest comment is that I'd rather avoid having the new config flag, and instead I'd go with a new name for the dataset, like voc_2007_train_cocostyle
.
Thoughts?
Hi @fmassa Instead of having two entries in "voc_2012_train": ("voc/VOC2012", 'train'),
"voc_2012_train_cocostyle": ("voc/VOC2012/JPEGImages", "voc/VOC2012/Annotations/pascal_test2012.json"), If we use dictionary to replace tuple, it will be more readable "voc_2012_train": {
"data_dir": "voc/VOC2012",
"img_dir": "voc/VOC2012/JPEGImages",
"ann_file": "voc/VOC2012/Annotations/pascal_train2012.json",
"split": "train"
}, In addition, I think using code to append *_cocostyle entries to DATASETS is clearer. coco_style_datasets = {
key + "_cocostyle": value for key, value in DATASETS.items() if "coco" not in key
} }
DATASETS={**DATASETS, **coco_style_datasets} What do you think about all the updates? |
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.
This is looking great, thanks!
I have a few more comments, let me know what you think, I'm open to suggestions!
Thanks for the review and all the comments.
|
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.
Thanks a lot!
This reverts commit 0f61b00.
* add force json option * fix the same issue as facebookresearch#185 * bug fix * cityscapes config * update paths catalog * discard config change * organize code for more-datasets * use better representation for coco-style datasets * rename coco-style config * remove import * chmod 644 * make the config more verbose * update readme * rename * chmod
I would like to make a proposal for the issues about supporting other datasets. e.g. #168 and #207.
#168 doesn't have a clear solution so far, and #207 doesn't support evaluating segmentation results.
Inspired by Detectron, I propose that adding a new config option, "DATASETS.FORCE_USE_JSON_ANNOTATION".
If the option is True, maskrcnn-benchmark will use COCO API to load the specified dataset and evaluate the result with the COCO styled AP. (i.e. AP, AP50, AP75, APs, APm, APl for bbox and segm)
In addition to the newly added option, I also
You could try the below command options to evaluate the changes.
Thanks !