add support for pascal voc dataset and evaluate #131

lufficc · 2018-11-08T06:56:56Z

facebook-github-bot · 2018-11-08T06:57:01Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-11-08T07:09:34Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

fmassa

Hi,

Thanks a lot for the PR!

I made some comments.

In general, I think now it would be a good opportunity to clean up a bit more the inference.py file, in order to make it more generic and less specific to a particular dataset implementation.

I'll try to come up with a plan in the next week, but let me know if you have proposals for it.

fmassa · 2018-11-09T11:28:51Z

maskrcnn_benchmark/data/datasets/voc_eval.py

+from collections import defaultdict
+import itertools
+import numpy as np
+import numpy as xp


do you need this duplicated import?

It is a duplicated import, I've optimized it.

fmassa · 2018-11-09T12:00:06Z

maskrcnn_benchmark/data/datasets/voc.py

+        labels = ann['labels']
+
+        boxes = torch.as_tensor(boxes).reshape(-1, 4)  # guard against no boxes
+        target = BoxList(boxes, img.size, mode="xyxy").convert("xyxy")


You don't need the .convert("xyxy") here, as the mode is already specified as xyxy.

I've optimized it.

fmassa · 2018-11-09T12:03:43Z

maskrcnn_benchmark/data/datasets/voc.py

+
+from maskrcnn_benchmark.structures.bounding_box import BoxList
+
+VOC_BBOX_LABEL_NAMES = ('aeroplane',


If you add a __background__ in the first position, you don't need the mapping json_category_id_to_contiguous_id nor contiguous_category_id_to_json_id I think

Great advise. I've optimized it.

fmassa · 2018-11-09T12:04:59Z

maskrcnn_benchmark/data/datasets/voc.py

+            v: k for k, v in self.json_category_id_to_contiguous_id.items()
+        }
+        self.anns = {}
+        for img_id in self.ids:


Just to know, how much time it takes to load this for all images in the trainval set?
One other possibility would be to perform the file loading in __getitem__

Infact it costs very little time(<1s).To keep consistent, I add a print to tell the cost time like coco api does.

fmassa · 2018-11-09T12:07:01Z

maskrcnn_benchmark/data/datasets/voc_eval.py

+import six
+
+
+def bbox_iou(bbox_a, bbox_b):


we already have a bbox_iou function in structures/boxlist_ops.py, can't we use that instead?

To keep consistent, I removed bbox_iou and used structures/boxlist_ops.py's boxlist_iou.

fmassa · 2018-11-10T23:25:41Z

maskrcnn_benchmark/engine/inference.py

 from tqdm import tqdm
-
+from ..data.datasets.voc import VOC_BBOX_LABEL_NAMES


Ideally we would keep the inference engine agnostic to particular dataset implementations. But this will require some refactorings.

I removed it.

fmassa · 2018-11-10T23:26:59Z

maskrcnn_benchmark/engine/inference.py


+from ..config import cfg


I'd rather not import the config in this file, but keep it agnostic to it. You can probably pass the name of the dataset as an argument to the inference function

fmassa · 2018-11-10T23:27:38Z

maskrcnn_benchmark/data/datasets/voc_eval.py

+    return area_i / (area_a[:, None] + area_b - area_i)
+
+
+def eval_detection_voc(


If those functions come from an already-existing repo, it would be good to reference them here.

It is a modification version from chainercv repository.
(See https://github.com/chainer/chainercv/blob/master/chainercv/evaluations/eval_detection_voc.py)
I add this in top of voc_eval.py file.

fmassa · 2018-11-10T23:29:31Z

maskrcnn_benchmark/engine/inference.py

-
+            torch.save(results, os.path.join(output_folder, "coco_results.pth"))
+        return results, coco_results
+    if 'coco' in cfg.DATASETS.TEST[0]:


This doesn't work if we pass multiple datasets during evaluation. I think it might be better to pass a dataset name as an argument to the inference function

I fixed it by add a name property to dataset.

lufficc · 2018-11-13T14:26:24Z

Thanks very much for your review. I've submmited a new pr.

lufficc · 2018-11-13T14:33:57Z

Sorry, I cannot make a new pr, this pr has already contained the new changes. My mistakes.

fmassa

This is looking much better, thanks!

I've made some more comments, let me know what you think.

In particular, I think that there are two points that we should pay attention to:

the difficult instances
if we should split the inference into dataset-specific files. For example all the prepare_for_coco_detection functions could be moved to their own file, and leave entirely dataset-agnostic functions in inference.py, like compute_predictions and inference.

Thoughts?

fmassa · 2018-11-13T14:48:38Z

maskrcnn_benchmark/engine/inference.py

@@ -366,7 +363,9 @@ def inference(
    )
    logger = logging.getLogger("maskrcnn_benchmark.inference")
    dataset = data_loader.dataset
-    logger.info("Start evaluation on {} images".format(len(dataset)))
+    assert hasattr(dataset, 'name'), 'Dataset must has a name to perform evaluating.'
+    dataset_name = dataset.name


I was actually thinking about passing a dataset_name as an argument to inference, instead of adding a field in the dataset itself. What do you think?

fmassa · 2018-11-13T14:54:18Z

maskrcnn_benchmark/data/datasets/voc.py

+                'boxes': [[int(obj['bndbox']['xmin']),
+                           int(obj['bndbox']['ymin']),
+                           int(obj['bndbox']['xmax']),
+                           int(obj['bndbox']['ymax'])] for obj in data['object'] if int(obj['difficult']) == 0],


I think this is not the best thing to do unconditionally.

Indeed, we might want to remove the difficult instances during training, but during testing there should be a way of knowing which one of the instances are difficult, so that they are not taken into account during evaluation.

If I understand it correctly, the way it is currently done will be penalizing the difficult instances, as if we detect a difficult instance with our model, it will be counted as a false match.

I think we should have a flag that lets us decide if we want to return difficult instances or not.

fmassa · 2018-11-13T14:54:46Z

maskrcnn_benchmark/data/datasets/coco.py

    ):
        super(COCODataset, self).__init__(root, ann_file)
-
+        self.name = name


Let's maybe remove the name attribute in favour of the approach I mentioned just before?

What are your thoughts?

I got it. This would be a better choice.

fmassa · 2018-11-13T14:56:28Z

maskrcnn_benchmark/engine/inference.py

+    return results, coco_results, predictions
+
+
+def do_coco_evaluation(predictions,


What do you think about moving the coco-specific and pascal-specific evaluation functions into their own file, and have the inference file here be very agnostic to the dataset? It is almost there, right?

fmassa · 2018-11-13T14:57:18Z

maskrcnn_benchmark/data/datasets/voc_eval.py

@@ -0,0 +1,186 @@
+# A modification version from chainercv repository.
+# (See https://github.com/chainer/chainercv/blob/master/chainercv/evaluations/eval_detection_voc.py)
+from __future__ import division


I wonder if this file should live in datasets, or in a separate folder dedicated to inference. Thoughts?

In fact, inference.py looks a little chaotic.
I think inference.py should only do the predication, then pass results and call different evaluatation methods according to the dataset_name. Like you mentioned before.

The location of 'different evaluatation methods', I think, can be:

datasets -evaluatation -coco ... ... -voc ... ... coco.py voc.py

What do you think?

I agree with you, let's keep this structure that you proposed.

I've make a new commit. Thanks for your review again.

fmassa

This is starting to look very good, thanks!

I still think there is something that should be addressed with the gt_difficult part.
My thought is that if you load the xml at every iteration (instead of pre-loading and filtering it all in the init), you could decide to change the keep_difficult flag in the dataset after it was created.

Also, I would maybe attach a field to each bounding box indicating if it is difficult or not (for testing only). Something like

boxlist = BoxList(...)
boxlist.add_field("difficult", difficult)

this would be an easy way of handling the difficult boxes in the evaluation. But that's not necessary

fmassa · 2018-11-14T16:47:41Z

maskrcnn_benchmark/data/datasets/evaluation/voc/voc_eval.py

+    assert len(gt_boxlists) == len(pred_boxlists), 'Length of gt and pred lists need to be same.'
+    prec, rec = calc_detection_voc_prec_rec(pred_boxlists=pred_boxlists,
+                                            gt_boxlists=gt_boxlists,
+                                            iou_thresh=iou_thresh)


You still need to pass gt_difficults in here if you want it to be evaluated properly

lufficc · 2018-11-15T08:01:02Z

If I load the xml at every iteration, there may be a duplicated loading(when call get_img_info). So I think a better choice is load all annotations(include difficult objects) into memory, then filter them dynamically(according to use_difficult flag).

fmassa · 2018-11-15T10:56:01Z

This is looking much better. I'd rename difficulties to difficult though, but that's minor.

I'll try using the code and training a model in the weekend / next week and once I've verified that everything is working as expected I'll merge the PR or make further comments.

Thanks!

lufficc · 2018-11-15T10:57:32Z

OK. Thanks!

henrywang1 · 2018-11-21T08:06:51Z

Hi,
Thanks for the wonderful commit and review.
I was also trying different datasets on maskrcnn-benchmark, and I have some comments.

I think we could load the notation from json files directly like Detectron, it has VOC Annotations in COCO Format.
And we could also evaluate the segmentation result on voc dataset. (The information is also in the json files.)
What do you think? Maybe I could create another issue?

Thanks!

fmassa · 2018-11-21T08:38:27Z

Hi @henrywang1 ,
Yesterday I pulled out this repo and I started doing some minor changes, I should get this ready by tomorrow.

About your comments:
1 - yes, it is possible to use COCO annotations for Pascal VOC dataset. But I think it is good to have built-in support for some evaluation code which doesn't depend on COCO, and this PR adds that
2 - indeed, it could be interesting to add. Do you know if we generally evaluate instance segmentation on Pascal as well? I know that semantic segmentation is commonly evaluated, but I'm not sure about instance segmentation.

Thanks

henrywang1 · 2018-11-21T09:49:21Z

Hi @fmassa
As we know, COCO is a more commonly used dataset for instance segmentation.
But there are still some recent papers evaluating their instance segmentation models on PASCAL.
I list some of the papers for your reference.

In my opinion, the feature could benefit people who want to try Mask-RCNN, but they don't have much computing power.

Thanks

fmassa · 2018-11-21T10:22:32Z

Sounds good, it would be great to add support for instance masks as well on Pascal at a later PR.

But that might mean also implementing the evaluation code, or just directly using the data in COCO format

fmassa · 2018-11-23T13:24:16Z

Hi @lufficc ,

I've rebased your PR and modified a few more things (like removing the need of an external lxml lib).
Can you give me rights to append to this PR, information can be seen in https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

Or else I can submit a PR, while keeping the history of commits

lufficc · 2018-11-23T14:07:25Z

I've checked "Allow edits from maintainers", do you have permission now?

fmassa · 2018-11-23T14:26:18Z

shoot, I did something wrong here, let me try to fix it
in the meantime, don't pull from your branch. Sorry!

fmassa · 2018-11-23T14:36:30Z

@lufficc I couldn't make it work as I was expecting, so I opened a new PR in #207 .
It contains all your diffs, plus a few more from me.

I'm very sorry, I need to learn how to do this properly... :-/

fmassa · 2018-11-23T14:37:56Z

I was following instructions from https://help.github.com/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork/ , but I definitely did something wrong...

lufficc · 2018-11-23T14:48:32Z

It's OK, I'm not very familiar with that either 😆 . What should I do next?

fmassa · 2018-11-23T14:53:03Z

Maybe you could have a look at #207 (or more precisely the diffs that I added) and see if you are ok with that?

fmassa · 2018-11-23T15:01:00Z

Maybe the problem happened because your diff was based on the master branch, and not on a separate branch? But I'm not 100% sure

lufficc · 2018-11-23T15:05:33Z

I'm OK with your new commits.
Or maybe I didn't merge the pr with the master's HEAD?

fmassa · 2018-11-23T15:11:58Z

I don't actually know. I did almost exactly the same thing with #102 (comment) but it worked out fine in the end.

zimenglan-sysu-512 · 2018-11-26T06:10:58Z

hi @fmassa
will u add the results of pascal voc into MODEL_ZOO.md?

fmassa · 2018-11-26T10:46:31Z

Maybe, but I'd need to train a few more models and compare against what is currently available, so this will probably not happen in the near future

narendoraiswamy · 2019-10-04T09:53:27Z

Hello @fmassa, @lufficc @henrywang1. Correct me if I am wrong but support for pascal voc dataset for instance segmentation has not been completed yet? Using the voc.py file in data/datasets only supports the detection purpose and not the instance segmentation purpose.

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 8, 2018

fmassa suggested changes Nov 10, 2018

View reviewed changes

fmassa added the awaiting response label Nov 13, 2018

fmassa suggested changes Nov 13, 2018

View reviewed changes

fmassa suggested changes Nov 14, 2018

View reviewed changes

fmassa mentioned this pull request Nov 16, 2018

Cannot overlay BBoxes and Classes on the final prediction (inference) #147

Closed

fmassa removed the awaiting response label Nov 16, 2018

fmassa mentioned this pull request Nov 20, 2018

VOCSegmentation, VOCDetection, linting passing, examples. pytorch/vision#663

Merged

fmassa closed this Nov 23, 2018

fmassa force-pushed the master branch from 981be09 to 6855f23 Compare November 23, 2018 14:25

fmassa mentioned this pull request Nov 23, 2018

add support for pascal voc dataset and evaluate #207

Merged


		from maskrcnn_benchmark.structures.bounding_box import BoxList

		VOC_BBOX_LABEL_NAMES = ('aeroplane',

		from tqdm import tqdm

		from ..data.datasets.voc import VOC_BBOX_LABEL_NAMES

		return area_i / (area_a[:, None] + area_b - area_i)


		def eval_detection_voc(

		return results, coco_results, predictions


		def do_coco_evaluation(predictions,

add support for pascal voc dataset and evaluate #131

add support for pascal voc dataset and evaluate #131

Conversation

lufficc commented Nov 8, 2018

facebook-github-bot commented Nov 8, 2018

facebook-github-bot commented Nov 8, 2018

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lufficc Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lufficc Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lufficc commented Nov 13, 2018

lufficc commented Nov 13, 2018

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lufficc commented Nov 15, 2018

fmassa commented Nov 15, 2018

lufficc commented Nov 15, 2018

henrywang1 commented Nov 21, 2018 • edited Loading

fmassa commented Nov 21, 2018

henrywang1 commented Nov 21, 2018

fmassa commented Nov 21, 2018

fmassa commented Nov 23, 2018

lufficc commented Nov 23, 2018

fmassa commented Nov 23, 2018

fmassa commented Nov 23, 2018

fmassa commented Nov 23, 2018

lufficc commented Nov 23, 2018

fmassa commented Nov 23, 2018

fmassa commented Nov 23, 2018

lufficc commented Nov 23, 2018

fmassa commented Nov 23, 2018

zimenglan-sysu-512 commented Nov 26, 2018

fmassa commented Nov 26, 2018

narendoraiswamy commented Oct 4, 2019 • edited Loading

lufficc Nov 13, 2018 •

edited

Loading

lufficc Nov 13, 2018 •

edited

Loading

henrywang1 commented Nov 21, 2018 •

edited

Loading

narendoraiswamy commented Oct 4, 2019 •

edited

Loading