MscocoObjectDetection can set optional classes and instantiate. #525

takecore · 2019-10-15T10:46:03Z

Motivation and Context

Description

in _gt_boxes_from_image_id, only to use self.classes related annotation data.

I add lmnet/test/fixtures/datasets/MSCOCO. please check this out.
I add blank 4 images to MSCOCO/train2014, MSCOCO/val2014.
like this image 10x10 black image.

How has this been tested?

# on blueoil/lmnet
$ PYTHONPATH=. pytest tests/lmnet_tests/datasets_tests/test_mscoco.py

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature / Optimization (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py

takecore · 2019-10-16T10:37:41Z

I fixed buildkite test. please check again if you have time.

Joeper214

@takecore Thanks for your contribution! I added some comments. Also, please specify if either this is a bugfix or enhancement. 😄

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py

takecore · 2019-10-17T08:38:22Z

@tsawada I changed to blank images from mscoco images.
please check this out🐥

Joeper214

LGTM Thanks!

lmnet/tests/fixtures/datasets/MSCOCO/annotations/instances_train2014.json

lmnet/tests/fixtures/datasets/MSCOCO/annotations/instances_val2014.json

tsawada · 2019-10-18T07:50:07Z

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py

+
+
+# TODO
+def test_mscoco_segmentation():


This gives a false sense that we test segmentation (when somebody looks at list of tests)
So just have a todo comment without a function.
# TODO(takecore): test segmentation

tsawada · 2019-10-18T07:51:26Z

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py

+
+def test_mscoco_object_detection_optional_classes_instantiate():
+    MscocoObjectDetection.classes = ['person']
+    try:


Why a try block? I think the test fails when the function throws exception anyway (I might be wrong)

@tsawada

This try block mean instantiate test.

I think the test fails when the function throws exception anyway

I think so. but test fails problem is how class designed is. (I think it is out of scope.)
This PR just fix override classes and can instantiate.

# ok pattern MscocoObjectDetection() # error occured pattern, but this PR will can run MscocoObjectDetection.classes = ['person'] MscocoObjectDetection()

lmnet/lmnet/datasets/mscoco.py

takecore

@tsawada thank I commented.

lmnet/tests/fixtures/datasets/MSCOCO/annotations/instances_train2014.json

lmnet/lmnet/datasets/mscoco.py

takecore · 2019-10-18T08:43:29Z

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py

+
+def test_mscoco_object_detection_optional_classes_instantiate():
+    MscocoObjectDetection.classes = ['person']
+    try:


@tsawada

This try block mean instantiate test.

I think the test fails when the function throws exception anyway

I think so. but test fails problem is how class designed is. (I think it is out of scope.)
This PR just fix override classes and can instantiate.

# ok pattern MscocoObjectDetection() # error occured pattern, but this PR will can run MscocoObjectDetection.classes = ['person'] MscocoObjectDetection()

takecore · 2019-10-18T09:02:04Z

@tsawada I fixed some reviews🦀 please check again!

lmnet/lmnet/datasets/mscoco.py

lmnet/tests/fixtures/datasets/MSCOCO/annotations/instances_val2014.json

takecore · 2019-10-25T07:09:51Z

@tsawada I fixed it! please check again.

tsawada · 2019-10-25T07:54:19Z

lmnet/lmnet/datasets/mscoco.py

@@ -225,10 +225,16 @@ def coco_category_id_to_lmnet_class_id(self, cat_id):
    @functools.lru_cache(maxsize=None)
    def _gt_boxes_from_image_id(self, image_id):
        """Return gt boxes list ([[x, y, w, h, class_id]]) of a image."""
+        classes = set(class_name for class_name in self.classes if class_name is not "__background__")


class_ids should be a set BUTclasses should be a list.

sorry, you right. I fixed this.

…e. change annotation values

tsawada · 2019-10-25T08:33:36Z

lmnet/lmnet/datasets/mscoco.py

@@ -198,7 +198,7 @@ def coco(self):
    def _image_ids(self):
        """Return all files and gt_boxes list."""
        classes = [class_name for class_name in self.classes if class_name is not "__background__"]
-        target_class_ids = self.coco.getCatIds(catNms=classes)
+        target_class_ids = set(self.coco.getCatIds(catNms=classes))


why do you use set() here?

@tsawada I have 2 reasons.

When another person looks at the code in the future, it can be recognized that the same description is written.
If they can be recognized as the same, it is easy to fixed or abstracted.
That's why I used set.

It's hard to tell if you're using self.classes in this class or if you're using looped classes.
I want to distinguish each one.
But, since the scope of the fixes is getting wider and wider, I have only described set.

I think we don't need set() here. Please leave it as a list.

thanks I fixed it as a list.

tsawada

LGTM, Approval

Thanks for all the fixes!

blueoil-butler bot added the CI: auto-run Run CI automatically label Oct 15, 2019

iizukak added the CI: test-all Run all tests once label Oct 16, 2019

blueoil-butler bot removed the CI: test-all Run all tests once label Oct 16, 2019

iizukak reviewed Oct 16, 2019

View reviewed changes

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py Outdated Show resolved Hide resolved

takecore force-pushed the fix/mscoco-dataset-can-use-optional-classes branch from 3408bad to 44d2f81 Compare October 16, 2019 09:29

Joeper214 reviewed Oct 17, 2019

View reviewed changes

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py Outdated Show resolved Hide resolved

lmnet/tests/lmnet_tests/datasets_tests/test_mscoco.py Outdated Show resolved Hide resolved

iizukak requested a review from tsawada October 17, 2019 04:05

Joeper214 reviewed Oct 17, 2019

View reviewed changes

tsawada reviewed Oct 18, 2019

View reviewed changes

takecore commented Oct 18, 2019

View reviewed changes

tsawada reviewed Oct 21, 2019

View reviewed changes

lmnet/lmnet/datasets/mscoco.py Outdated Show resolved Hide resolved

lmnet/tests/fixtures/datasets/MSCOCO/annotations/instances_val2014.json Outdated Show resolved Hide resolved

tsawada reviewed Oct 25, 2019

View reviewed changes

takecore added 12 commits October 25, 2019 17:07

MscocoObjectDetection can set optional classes, instantiate.

7b61562

change try except on test

1269467

fix test

a047bfa

add pytest set_test_environment fixture

1da33c1

fix test

2946fdf

add MSCOCO train val json, and images.

968c08a

add test, change test image to blank image, because of respect licens…

c396143

…e. change annotation values

fixed json.info to dummy, fixed some review

af9e864

add endof file newline

bc1fc47

use set()

38c7d68

fix test dont use try

b50eea3

fix separate use list, set

96cc905

takecore force-pushed the fix/mscoco-dataset-can-use-optional-classes branch from bd765e9 to 96cc905 Compare October 25, 2019 08:08

tsawada reviewed Oct 25, 2019

View reviewed changes

revert to use list

e5cd2ed

tsawada approved these changes Oct 25, 2019

View reviewed changes

Joeper214 and others added 2 commits October 29, 2019 10:23

Merge branch 'master' into fix/mscoco-dataset-can-use-optional-classes

f1af5cd

Merge branch 'master' into fix/mscoco-dataset-can-use-optional-classes

b548e4e

iizukak approved these changes Oct 29, 2019

View reviewed changes

iizukak merged commit a189ba4 into blue-oil:master Oct 29, 2019

takecore deleted the fix/mscoco-dataset-can-use-optional-classes branch October 30, 2019 09:58

iizukak mentioned this pull request Oct 31, 2019

If set optional classes on MscocoObjectDetection then fail to instantiate. #523

Closed

iizukak added this to the v0.14.0 milestone Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MscocoObjectDetection can set optional classes and instantiate. #525

MscocoObjectDetection can set optional classes and instantiate. #525

takecore commented Oct 15, 2019 •

edited

Loading

takecore commented Oct 16, 2019

Joeper214 left a comment

takecore commented Oct 17, 2019

Joeper214 left a comment

tsawada Oct 18, 2019

tsawada Oct 18, 2019

takecore Oct 18, 2019

takecore left a comment

takecore Oct 18, 2019

takecore commented Oct 18, 2019

takecore commented Oct 25, 2019

tsawada Oct 25, 2019

takecore Oct 25, 2019

tsawada Oct 25, 2019

takecore Oct 25, 2019

tsawada Oct 25, 2019

takecore Oct 25, 2019 •

edited

Loading

tsawada left a comment

MscocoObjectDetection can set optional classes and instantiate. #525

MscocoObjectDetection can set optional classes and instantiate. #525

Conversation

takecore commented Oct 15, 2019 • edited Loading

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

takecore commented Oct 16, 2019

Joeper214 left a comment

Choose a reason for hiding this comment

takecore commented Oct 17, 2019

Joeper214 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

takecore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takecore commented Oct 18, 2019

takecore commented Oct 25, 2019

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

takecore Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

tsawada left a comment

Choose a reason for hiding this comment

takecore commented Oct 15, 2019 •

edited

Loading

takecore Oct 25, 2019 •

edited

Loading