FPN - Add min/max size params for pre-processing. #830

23pointsNorth · 2019-03-10T23:30:02Z

Allow for the min_size, max_size parameter to be set when creating an FPN network.
It is following the Faster-RCNN template.

yuyu2172 · 2019-03-11T03:05:08Z

Thanks. Can you set the default values for FasterRCNN so that the tests won't fail?

Hakuyume · 2019-03-12T09:33:27Z

chainercv/links/model/fpn/faster_rcnn.py

@@ -26,6 +26,9 @@ class FasterRCNN(chainer.Chain):
        head (Link): A link that has the same interface as
            :class:`~chainercv.links.model.fpn.Head`.
            Please refer to the documentation found there.
+        min_size (int): A preprocessing paramter for :meth:`prepare`. Please
+            refer to a docstring found for :meth:`prepare`.


How about this?

Please refer to :meth:`prepare`.

docstring will be confusing when it rendered by sphinx.

This was done in the same style as the faster-rcnn docs with regards to the param.

Hakuyume · 2019-03-12T09:35:41Z

chainercv/links/model/fpn/faster_rcnn_fpn_resnet.py

-       n_fg_class (int): The number of classes excluding the background.
-       pretrained_model (string): The weight file to be loaded.
+        n_fg_class (int): The number of classes excluding the background.
+        pretrained_model (string): The weight file to be loaded.


Why did you change the indentation?

The rest of the code is indented with 4 space.
These lines were with 3. I am happy to revert if comments don't follow this requirement.

This was my mistake. Thank you for fixing!

Hakuyume · 2019-03-12T09:35:48Z

chainercv/links/model/fpn/faster_rcnn_fpn_resnet.py

+        n_fg_class (int): The number of classes excluding the background.
+        pretrained_model (string): The weight file to be loaded.
+            This can take :obj:`'coco'`, `filepath` or :obj:`None`.
+            The default value is :obj:`None`.


Hakuyume · 2019-03-12T09:38:26Z

chainercv/links/model/fpn/faster_rcnn_fpn_resnet.py

@@ -19,7 +19,8 @@ class FasterRCNNFPNResNet(FasterRCNN):
    A subclass of this class should have :obj:`_base` and :obj:`_models`.
    """

-    def __init__(self, n_fg_class=None, pretrained_model=None):
+    def __init__(self, n_fg_class=None, pretrained_model=None,
+                 min_size=800, max_size=1333):


min_size and max_size are not used.

That was a versioning mistake. It will be added on L39.

Hakuyume · 2019-03-12T09:41:32Z

Thanks. Can you set the default values for FasterRCNN so that the tests won't fail?

Perhaps, we can modify the tests to pass those parameters explicitly. In that case, we can remove the default value from FasterRCNN.

Hakuyume · 2019-03-12T09:44:44Z

@23pointsNorth Can you add tests that change min_size or max_size?
https://github.com/chainer/chainercv/blob/master/tests/links_tests/model_tests/fpn_tests/test_faster_rcnn.py#L120-L126

23pointsNorth · 2019-03-12T10:04:50Z

I am not sure how you expect the testing to look like. If we inspect the other place preprare is used, i.e. https://github.com/chainer/chainercv/blob/master/tests/links_tests/model_tests/faster_rcnn_tests/test_faster_rcnn.py#L81 only the input param is checked against.

I can alter the tests to include a value for min/max, such that we can remove the default in the class (but I think it may be unnecessary, Edit based on:

chainercv/chainercv/links/model/faster_rcnn/faster_rcnn.py

Line 91 in 451c216

min_size=600,

that also has default params in the class)

Hakuyume · 2019-03-12T10:48:42Z

I am not sure how you expect the testing to look like. If we inspect the other place preprare is used, i.e. https://github.com/chainer/chainercv/blob/master/tests/links_tests/model_tests/faster_rcnn_tests/test_faster_rcnn.py#L81 only the input param is checked against.

The test in faster_rcnn_tests checks the effect of min_size/max_size because it uses values that are different from the default values ((200, 400) vs (600, 1000)). On the other hand, the test in fpn_tests uses the default value. So the test cannot detect an error even if FasterRCNN ignores those parameters. At least we should use different values for the test. It would be better to check both default params and other params (with testing.parameterize).

I can alter the tests to include a value for min/max, such that we can remove the default in the class (but I think it may be unnecessary, Edit based on:

I'm sorry for my confusing comment. It is OK to keep the default values. I just mentioned the side-effect.

23pointsNorth · 2019-03-12T11:20:42Z

@Hakuyume I attempted to add extra tests. I changed them to follow the ones in test_faster_rcnn in regards to in_shape, expected_shape.

The paramterize tag becomes a bit clunky, because in our tests we want to make the permutations of 3*n_fg_class, 2*(min_size, max_size, in_shape, expected shape). I don't know if I can give 2 dict and the testing can do the permutations of them.

Hakuyume · 2019-03-12T12:00:00Z

tests/links_tests/model_tests/fpn_tests/test_faster_rcnn.py

-    {'n_fg_class': 20},
+    {'n_fg_class': 1, 'min_size': 200, 'max_size': 400, 
+     'in_shape': (3, 200, 50), 'expected_shape': (3, 400, 100)},
+    {'n_fg_class': 1, 'min_size': 800, 'max_size': 133, 


133 is typo of 1333?

Yes, a typo.

Hakuyume · 2019-03-12T12:16:38Z

tests/links_tests/model_tests/fpn_tests/test_faster_rcnn.py

+        self.assertEqual(out.shape, self.expected_shape)
+
+    def test_prepare_cpu(self):
+        self.check_prepare()


We do not need to make _cpu/_gpu because prepare is not related cupy.

Hakuyume · 2019-03-12T12:17:39Z

You can use testing.product_dict.

@testing.parameterize(*testing.product_dict(
    [
        {'n_fg_class': 1},
        {'n_fg_class': 5},
        {'n_fg_class': 20},
    ],
    [
        {
            'in_shape': (3, 480, 640),
            'min_size': 800, 'max_size': 1333,
            'expected_shape': (3, 800, 1088),
        },
        {
            'in_shape': (3, 200, 50),
            'min_size': 200, 'max_size': 400,
            'expected_shape': (3, 400, 100),
        },
    ],
))
class TestFasterRCNN(unittest.TestCase):

Hakuyume · 2019-03-12T12:30:27Z

tests/links_tests/model_tests/fpn_tests/test_faster_rcnn.py

+    {'n_fg_class': 1, 'min_size': 200, 'max_size': 400, 
+     'in_shape': (3, 200, 50), 'expected_shape': (3, 400, 100)},
+    {'n_fg_class': 1, 'min_size': 800, 'max_size': 133, 
+     'in_shape': (3, 480, 640), 'expected_shape': (3, 800, 1088)},


prepare can accept a batch of images of different shapes.
How about 'in_sizes': [(480, 640), (320, 320)], 'expected_size': (800, 1088) ? (I omit 3 because it is not parameterized)

23pointsNorth · 2019-03-12T16:13:59Z

I think I've incorporated all of the requests. A general unexpected behaviour I found based on the tests i.e.
https://travis-ci.org/chainer/chainercv/jobs/505234966#L847
If the max_size is the parameter based on which the size is calculated, and if it is not divisible by stride, based on line
https://github.com/chainer/chainercv/blob/master/chainercv/links/model/fpn/faster_rcnn.py#L165
we'll get a resulting image size that is larger than max_size (i.e. max_size of 400 -> final image size 416).

This I think is ok in general, but should be documented somewhere.

Hakuyume · 2019-03-12T17:08:22Z

If the max_size is the parameter based on which the size is calculated, and if it is not divisible by stride, based on line
https://github.com/chainer/chainercv/blob/master/chainercv/links/model/fpn/faster_rcnn.py#L165
we'll get a resulting image size that is larger than max_size (i.e. max_size of 400 -> final image size 416).

Actually, this is an expected behavior (same as the Detecton's implementation).
But I agree with you that it is confusing because the output can exceed max_size.

This I think is ok in general, but should be documented somewhere.

How about adding a note here?

chainercv/chainercv/links/model/fpn/faster_rcnn.py

Line 31 in f1ad965

max_size (int): A preprocessing paramter for :meth:`prepare`.

For example,

max_size (int): A preprocessing paramter for :meth:`prepare`. 
        Note that the result of :meth:`prepare` can exceed this size
        because of alignment.

Hakuyume

LGTM. Thank you!

Add size params for preprocessing.

b1cca53

Update FasterRCNN with default sizes.

451c216

Hakuyume reviewed Mar 12, 2019

View reviewed changes

Add size param and fix comments.

ea816f8

Add tests for min/max size in prepare.

b27316c

Hakuyume reviewed Mar 12, 2019

View reviewed changes

23pointsNorth added 2 commits March 12, 2019 13:54

Update size tests

99a0a35

Fix size param to be a whole integer of _stride.

f1ad965

23pointsNorth added 2 commits March 12, 2019 17:31

Fix flake8 errors.

2f2d262

Add note about size.

98e5058

Hakuyume approved these changes Mar 12, 2019

View reviewed changes

Hakuyume merged commit 975964d into chainer:master Mar 12, 2019

knorth55 added the enhancement label May 14, 2019

knorth55 added this to the 0.13 milestone May 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FPN - Add min/max size params for pre-processing. #830

FPN - Add min/max size params for pre-processing. #830

23pointsNorth commented Mar 10, 2019

yuyu2172 commented Mar 11, 2019

Hakuyume Mar 12, 2019

23pointsNorth Mar 12, 2019

Hakuyume Mar 12, 2019

Hakuyume Mar 12, 2019

23pointsNorth Mar 12, 2019

Hakuyume Mar 12, 2019

Hakuyume Mar 12, 2019

Hakuyume Mar 12, 2019

23pointsNorth Mar 12, 2019

Hakuyume commented Mar 12, 2019

Hakuyume commented Mar 12, 2019

23pointsNorth commented Mar 12, 2019 •

edited

Loading

Hakuyume commented Mar 12, 2019

23pointsNorth commented Mar 12, 2019

Hakuyume Mar 12, 2019

23pointsNorth Mar 12, 2019

Hakuyume Mar 12, 2019

Hakuyume commented Mar 12, 2019

Hakuyume Mar 12, 2019

23pointsNorth commented Mar 12, 2019 •

edited

Loading

Hakuyume commented Mar 12, 2019

Hakuyume left a comment

FPN - Add min/max size params for pre-processing. #830

FPN - Add min/max size params for pre-processing. #830

Conversation

23pointsNorth commented Mar 10, 2019

yuyu2172 commented Mar 11, 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

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

Hakuyume commented Mar 12, 2019

Hakuyume commented Mar 12, 2019

23pointsNorth commented Mar 12, 2019 • edited Loading

Hakuyume commented Mar 12, 2019

23pointsNorth commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hakuyume commented Mar 12, 2019

Choose a reason for hiding this comment

23pointsNorth commented Mar 12, 2019 • edited Loading

Hakuyume commented Mar 12, 2019

Hakuyume left a comment

Choose a reason for hiding this comment

23pointsNorth commented Mar 12, 2019 •

edited

Loading

23pointsNorth commented Mar 12, 2019 •

edited

Loading