fix bug: wrong output dimension when "keep_dims" is false in pooling layer. by Crayon-new · Pull Request #20904 · opencv/opencv

Crayon-new · 2021-10-19T09:45:28Z

Merge with extra: opencv/opencv_extra#932

Pull Request Readiness Checklist

Fixed bug in #20896：When parsing a pooling layer in dnn/tensorflow. If keep_dims is false, the additional "nhwc" layer and "squeeze" layer will be lost. Because the final "squeeze" layer name is not the original layer name.

Solution:
Change the final output layer's name to the orignal pooling layer's name.

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

opencv_extra=tf_pooling

Crayon-new · 2021-10-19T11:35:34Z

I checked the build results. "Test_TensorFlow_layers.reduce_sum/0" failed because I connect a permute layer after "squeeze". So the output dimension from "nwc" change to "ncw".

But I think it is necessary to add it. Because we may do some other operations (like "concat" in this model) after pooling.

alalek

Thank you for contribution!

alalek · 2021-10-27T19:46:55Z

modules/dnn/src/tensorflow/tf_importer.cpp

-            connect(layer_id, dstNet, parsePin(layer.input(0)), id, 0);
-
-            if (!keepDims)
+            if (keepDims) {


Test data sum_pool_by_axis_out.npy is generated by this code through original framework.

So changing the "output" only looks very strange (sum_pool_by_axis_out.npy in opencv_extra PR) and would conflict with framework results (we must avoid such conflicts).

Is there any information about processing/behavior changes between TF 1.x / TF 2.x?
If so we need to properly handle min_consumer TF versions here (but sum_pool_by_axis_net.pb file doesn't contain versions information).

Thanks for your reply!
I think TF 1.x/ TF 2.x both produce the same output data.

Crayon-new · 2021-11-05T07:07:41Z

@asmorkalov Hello, anything else do I need to do?

rogday

Thank you for your contribution! IIRC, bugfixes should go to 3.4 branch and not 4.x, so, please, rebase.

I think, there is more to it. Could you check other cases as well? I used +1 instead of ExpandDims to avoid more issues with dimensions.

I used the following code for tests, it shows errors with axis=[0], [3](keepdims true and false), [1,2]:

axises = [[0], [1], [2], [3], [1, 2]]

for axis in axises:
    for keepdims in [False, True]:
        inp = tf.placeholder(tf.float32, [2, 3, 4, 1])
        biasadd = tf.nn.bias_add(inp, [1], data_format='NHWC')
        print(axis, keepdims)
        reduced = tf.reduce_sum(biasadd, axis=axis, keepdims=keepdims)
        save(inp, reduced + 1, f'reduce_sum_{axis}_{keepdims}')

TEST_P(Test_TensorFlow_layers, pooling_reduce_sum3)
{
    std::vector<std::vector<int>> axises = {{0}, {1}, {2}, {3}, {1, 2}};

    for (const auto& axis : axises)
    {
        for (int keepdims = 0; keepdims <= 1; ++keepdims)
        {
            std::stringstream ss;
            ss << "reduce_sum_[" << axis[0];
            if (axis.size() > 1)
            {
                ss << ", " << axis[1];
            }
            ss << "]_" << (keepdims ? "True" : "False");
            std::cout << ss.str() << std::endl;
            try
            {
                runTensorFlowNet(ss.str());
            }
            catch (const std::exception& e)
            {
                std::cout << e.what() << std::endl;
            }
        }
    }
}

rogday · 2021-11-08T10:36:52Z

modules/dnn/src/tensorflow/tf_importer.cpp

+                layerParams.set("pool", pool_type);
+                layerParams.set(axis == 2 ? "kernel_w" : "kernel_h", 1);
+                layerParams.set(axis == 2 ? "global_pooling_h" : "global_pooling_w", true);


Seems like this bit of code can be extracted from the if body.

rogday

Good work! 👍

rogday · 2021-11-09T08:49:55Z

modules/dnn/src/tensorflow/tf_importer.cpp

+            else
            {
                // To keep correct order after squeeze dims we first need to change layout from NCHW to NHWC
+                std::string poolingName = name+"/Pooling";


Please, add spaces around plus sign.

rogday · 2021-11-09T08:51:53Z

modules/dnn/src/tensorflow/tf_importer.cpp


                LayerParams squeezeLp;
-                std::string squeezeName = name + "/squeeze";
+                std::string squeezeName = name;


Please, use const std::string&, since you aren't modifying the variable.

rogday · 2021-11-09T08:52:32Z

modules/dnn/src/tensorflow/tf_importer.cpp

-            int id = dstNet.addLayer(name, "Pooling", layerParams);
-            layer_id[name] = id;
-            connect(layer_id, dstNet, inpId, id, 0);
+            std::string poolingName = name+"/Pooling";


Please, add spaces around the plus sign and a check, that the layer with that name doesn't exist yet: CV_Assert(layer_id.find(poolingName) == layer_id.end());

rogday · 2021-11-09T08:54:18Z

modules/dnn/src/tensorflow/tf_importer.cpp

                LayerParams squeezeLp;
-                std::string squeezeName = name + "/squeeze";
+                std::string squeezeName = name;
                CV_Assert(layer_id.find(squeezeName) == layer_id.end());


This check is not required anymore(we aren't giving new name to the layer).

rogday · 2021-11-09T08:58:56Z

modules/dnn/src/tensorflow/tf_importer.cpp

+        }
+        else
+        {
+            std::string poolingName = name+"/Pooling";


Please, add spaces around the plus sign and a check, that the layer with that name doesn't exist yet: CV_Assert(layer_id.find(poolingName) == layer_id.end());

rogday · 2021-11-09T09:04:12Z

modules/dnn/src/tensorflow/tf_importer.cpp

+            std::string flattenName = name;
            CV_Assert(layer_id.find(flattenName) == layer_id.end());


Consider using const std::string& and removing the assert.

rogday · 2021-11-09T09:04:56Z

modules/dnn/src/tensorflow/tf_importer.cpp

+                std::string squeezeName = name;
                CV_Assert(layer_id.find(squeezeName) == layer_id.end());


Consider changing the type to const std::string& and removing the assert.

rogday · 2021-11-09T09:06:51Z

modules/dnn/test/test_tf_importer.cpp

+TEST_P(Test_TensorFlow_layers, pooling_reduce_sum2)
+{
+    int axises[] = {0, 1, 2, 3};
+    for (int i = 0; i<sizeof(axises)/sizeof(axises[0]); i++)
+    {
+        for (int keepdims = 0; keepdims <= 1; ++keepdims)
+        {
+            std::stringstream ss;
+            ss << "reduce_sum_[" << axises[i] << "]_" << (keepdims ? "True" : "False");
+            std::cout << ss.str() << std::endl;
+            try
+            {
+                runTensorFlowNet(ss.str());
+            }
+            catch (const std::exception& e)
+            {
+                std::cout << e.what() << std::endl;
+            }
+        }
+    }
+}
+
+TEST_P(Test_TensorFlow_layers, pooling_reduce_sum3)
+{
+    int axises[][2] = {{1, 2}};  // two axises
+    for (int i = 0; i<sizeof(axises)/sizeof(axises[0]); i++)
+    {
+        for (int keepdims = 0; keepdims <= 1; ++keepdims)
+        {
+            std::stringstream ss;
+            ss << "reduce_sum_[" << axises[i][0] << ", " << axises[i][1] << "]_" << (keepdims ? "True" : "False");
+            std::cout << ss.str() << std::endl;
+            try
+            {
+                runTensorFlowNet(ss.str());
+            }
+            catch (const std::exception& e)
+            {
+                std::cout << e.what() << std::endl;
+            }
+        }
+    }
+}


My proposed solution was a draft, which helps debugging the problem.
Consider this instead:

TEST_P(Test_TensorFlow_layers, pooling_reduce_sum2) { int axises[] = {0, 1, 2, 3}; for (int keepdims = 0; keepdims <= 1; ++keepdims) { for (int i = 0; i < sizeof(axises)/sizeof(axises[0]); ++i) { runTensorFlowNet(cv::format("reduce_sum_[%d]_%s", axises[i], keepdims ? "True" : "False")); } runTensorFlowNet(cv::format("reduce_sum_[1, 2]_%s", keepdims ? "True" : "False")); } }

alalek · 2021-11-09T10:39:42Z

modules/dnn/test/test_tf_importer.cpp

+        {
+            runTensorFlowNet(cv::format("reduce_sum_[%d]_%s", axises[i], (keepdims ? "True" : "False")));
+        }
+        runTensorFlowNet(cv::format("reduce_sum_[1, 2]_%s", keepdims ? "True" : "False"));


'['
']'
','
' '

Could you please use sanitized file names? (please use _ instead of these symbols or space)

rogday

LGTM 👍

asmorkalov requested a review from rogday October 19, 2021 10:00

asmorkalov added the category: dnn label Oct 19, 2021

Crayon-new mentioned this pull request Oct 23, 2021

fix output dims of test "sum_pool_by_axis" opencv/opencv_extra#928

Closed

alalek reviewed Oct 27, 2021

View reviewed changes

Crayon-new mentioned this pull request Oct 31, 2021

add test case for "tf/reduce_sum" opencv/opencv_extra#932

Merged

rogday reviewed Nov 8, 2021

View reviewed changes

Crayon-new changed the base branch from 4.x to 3.4 November 8, 2021 11:27

Crayon-new added 4 commits November 8, 2021 20:04

fix bug in max layer

e65a0e6

code align

e9c8c97

delete permute layer and add test case

d856530

add name assert

18b8102

Crayon-new force-pushed the fix_bug_in_maxLayer branch from 75a7e93 to 18b8102 Compare November 8, 2021 12:06

Crayon-new added 2 commits November 9, 2021 01:04

check other cases

57565f8

remove c++11 features

57ef45f

rogday suggested changes Nov 9, 2021

View reviewed changes

style:add "const" remove assert

860e872

alalek reviewed Nov 9, 2021

View reviewed changes

style:sanitize file names

347cb7c

rogday approved these changes Nov 9, 2021

View reviewed changes

alalek assigned rogday Nov 9, 2021

alalek merged commit 98b6ce3 into opencv:3.4 Nov 9, 2021

alalek mentioned this pull request Nov 13, 2021

(4.x) Merge 3.4 #21051

Merged

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

		std::string flattenName = name;
		CV_Assert(layer_id.find(flattenName) == layer_id.end());

		std::string squeezeName = name;
		CV_Assert(layer_id.find(squeezeName) == layer_id.end());

Uh oh!

Conversation

Crayon-new commented Oct 19, 2021 • edited by rogday Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Crayon-new commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

alalek Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crayon-new Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crayon-new commented Nov 5, 2021

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogday Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogday left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Crayon-new commented Oct 19, 2021 •

edited by rogday

Loading

Crayon-new commented Oct 19, 2021 •

edited

Loading

alalek Oct 27, 2021 •

edited

Loading

Crayon-new Oct 28, 2021 •

edited

Loading

rogday Nov 9, 2021 •

edited

Loading