Speed up mean_filter2d with depthwise_conv2d #235

WindQAQ · 2019-05-03T08:41:00Z

Closes #234. Will create another PR to clean up median_filter2d.

facaiy

Great idea to accelerate the computation. Perhaps we could refactor test cases like https://github.com/tensorflow/tensorflow/blob/b14c390fc869b63fd2b1a4e6f4477ce410b9383e/tensorflow/python/kernel_tests/conv_ops_3d_test.py#L203

tensorflow_addons/image/filters.py

facaiy · 2019-05-05T01:22:25Z

tensorflow_addons/image/filters.py

+    pad_left = (filter_width - 1) // 2
+    pad_right = filter_width - 1 - pad_left
+    paddings = [[0, 0], [pad_top, pad_bottom], [pad_left, pad_right], [0, 0]]
+    return tf.pad(image, paddings, mode=mode, constant_values=constant_values)


Can we add an unit test for even shape, say (4, 4), and make sure the result is the same with scipy?

Should we import scipy for test here?

I could add a test case to check whether the output of explicitly zero padding is the same with the one of native "SAME" padding supported by depthwise_conv2d if we really do not want to compare the result produced by other packages.

Hi, Tzu-Wei, perhaps we don't have to import scipy. Can we rename test_reflect_padding to test_reflect_padding_with_3x3_filter, and create an new test_reflect_padding_with_4x4_filter in the same way?

Hi, Facai, after a rough test, I found the padding method (or the anchor of even-sized kernel) of tensorflow seems different from the one of scipy. Here is the testing notbook. Take 4x4 filter for example. The explicit (zero) padding could yield the same result with implicit SAME padding supported by depthwise_conv2d. However, I have to adjust origin argument in scipy's implement to (-1, -1) so that it could produce the same result. In this case, should we follow the scipy's implementation or match the implicit padding in tensorflow?

BTW,
scipy's reflect padding => (c b a | a b c d | d c b)
TF's reflect padding => (d c b | a b c d | c b a)
TF's reflect padding = scipy's mirror padding
scipy's reflect padding = TF's symmetric padding

Seems that they have different meaning...
https://www.tensorflow.org/api_docs/python/tf/pad

Hi, Tzu-Wei,

If it's not easy to be compatible with scipy's implementation, we can give out all details about how we calculate it in the document.

As for padding mode, I think tf.pad keeps consistent with np.pad. And similarity, we can either keep compatible with scipy, or write a detailed document.

Both are fine for me. cc @seanpmorgan Sean, what's your opinion?

Sure :-) In either way, it should be more docs there so let me write some details in this version first.

tensorflow_addons/image/filters.py

tensorflow_addons/image/filters_test.py

facaiy · 2019-05-05T01:31:14Z

@Mainak431 Mainak, as the author, would you like to take a look? Thanks.

Mainak431 · 2019-05-05T06:25:25Z

@WindQAQ. I think we should only take 4D tensor. If anybody wants to process a single image. He/she should reshape the tensor to (1, image-shape). This pattern is followed in many tf operations that supports both single and batch processing. I think that trend should be followed here too.

WindQAQ · 2019-05-05T06:28:47Z

@Mainak431 But most image tf.image functions support both 3-D and 4-D input
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L259
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L281
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L351
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L423
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L523
https://github.com/tensorflow/tensorflow/blob/r1.13/tensorflow/python/ops/image_ops_impl.py#L553
etc

Mainak431 · 2019-05-05T06:35:45Z

Yes. Image operations support this. But, many other operations i have come through does not support this. It seems there is no rule for the same. Ok.

Mainak431 · 2019-05-05T06:38:39Z

Can you please try your function with conv2d layer, activation layer, dropout and other basic layers by creating a small tf architecture. And ensure that it is working as expected even in the training of nn. A colab notebook will be helpful. As, the batch processing operation aims at direct usage of this in the nn architecture. Thanks.

WindQAQ · 2019-05-05T06:52:25Z

@WindQAQ. I think we should only take 4D tensor. If anybody wants to process a single image. He/she should reshape the tensor to (1, image-shape). This pattern is followed in many tf operations that supports both single and batch processing. I think that trend should be followed here too.

Ok, @facaiy What's your thought on this. I'd vote for that the input can be either 3-D or 4-D.

Can you please try your function with conv2d layer, activation layer, dropout and other basic layers by creating a small tf architecture. And ensure that it is working as expected even in the training of nn. A colab notebook will be helpful. As, the batch processing operation aims at direct usage of this in the nn architecture. Thanks.

I'll run a toy model on MNIST dataset when I'm back to the office.

Mainak431 · 2019-05-05T06:57:22Z

Just out of curiosity, what happens to a 2D grayscale image. Batch of this kind of images will look like a 3D image. I think either this should be explicitly mentioned in the documentation. Or we should consider taking only 4D tensors.

WindQAQ · 2019-05-05T07:04:01Z

Just out of curiosity, what happens to a 2D grayscale image. Batch of this kind of images will look like a 3D image. I think either this should be explicitly mentioned in the documentation. Or we should consider taking only 4D tensors.

I think we have already mentioned this in the documentation :-) Users have to make sure that the last axis is the channel axis as the one in the core TF.

addons/tensorflow_addons/image/filters.py

Lines 58 to 60 in 51b18f6

    
               Args: 
        
                 image: Either a 3-D `Tensor` of shape `[height, width, channels]`, 
        
                   or a 4-D `Tensor` of shape `[batch_size, height, width, channels]`.

Mainak431 · 2019-05-05T07:10:55Z

Yes. But this thing that user have to set last axis as 1 for 2d grayscale image is not explicitly mentioned. it will be useful for first time users. I remember while i was using it for the first time, i ran into similar errors and i had to stackoverflow it. Or, the other option is to take only 4D tensors. Whatever you think more suitable. Thanks.

…

On Sun 5 May, 2019, 12:34 PM Tzu-Wei Sung, ***@***.***> wrote: Just out of curiosity, what happens to a 2D grayscale image. Batch of this kind of images will look like a 3D image. I think either this should be explicitly mentioned in the documentation. Or we should consider taking only 4D tensors. I think we have already mentioned this in the documentation :-) Users have to make sure that the last axis is the channel axis as the one in the core TF. https://github.com/tensorflow/addons/blob/51b18f626adce8724acfca713268bad8218c7fda/tensorflow_addons/image/filters.py#L58-L60 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#235 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIRDMQWCERDH3FG7YJVGCSTPT2BGFANCNFSM4HKSEVWQ> .

facaiy · 2019-05-07T05:02:39Z

Sorry for the delay, @WindQAQ @Mainak431

I think we should keep consistency with tf.image: function works on either a single image (image is a 3-D Tensor), or a batch of images (image is a 4-D Tensor).

facaiy · 2019-05-23T05:11:28Z

@WindQAQ Tzu-Wei, could you update the PR when time allows :-)

WindQAQ · 2019-05-23T05:21:36Z

Thanks for reminding, facai. I'm currently workingon this :-)

…eed_up_mean_filter2d

WindQAQ · 2019-05-24T08:12:45Z

@facaiy, hi facai, I make some changes:

Avoid loss of precision
Refactor test cases according to https://github.com/tensorflow/tensorflow/blob/b14c390fc869b63fd2b1a4e6f4477ce410b9383e/tensorflow/python/kernel_tests/conv_ops_3d_test.py
Add a test case with channels of None

As for even-sized kernel test, should we use or import scipy for test here? Thanks for the suggestion!

tensorflow_addons/image/filters_test.py

facaiy

I'd like to merge the PR, and then we can fix all unsolved comments, if any, in the following PRs.

bot-of-gabrieldemarmiesse · 2020-03-09T16:05:41Z

@Mainak431

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

seanpmorgan · 2020-03-09T16:09:53Z

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@gabrieldemarmiesse this doesn't happen often but wondering if you see any reason this happens?

gabrieldemarmiesse · 2020-03-09T16:22:08Z

It's the first time I hear about it. Do you have other examples so I can see if there is a pattern?

In the yaml, the only condition starting the bot is:

on:
  pull_request:
    types: [opened]

So that makes me quite perplex that something happens after a PR is closed.

gabrieldemarmiesse · 2020-03-09T16:33:42Z

Sorry @seanpmorgan I've seriously no idea what causes this. If that happens too many time I'll take a much longer look.

WindQAQ added 2 commits May 3, 2019 11:09

speed up mean_filter2d with depthwise_conv2d

d4a10fe

cast the output back to the original dtype

51b18f6

WindQAQ requested a review from a team as a code owner May 3, 2019 08:41

googlebot added the cla: yes label May 3, 2019

facaiy reviewed May 5, 2019

View reviewed changes

facaiy added the image label May 5, 2019

WindQAQ added 4 commits May 24, 2019 15:23

Merge branch 'master' of https://github.com/tensorflow/addons into sp…

6e4d2d6

…eed_up_mean_filter2d

refactor test cases

ea10aac

avoid loss of precision

498d365

add test case with channels of None

39ae5a3

add doc of _tile_image

2fddf16

facaiy reviewed May 27, 2019

View reviewed changes

tensorflow_addons/image/filters_test.py Outdated Show resolved Hide resolved

WindQAQ added 3 commits May 27, 2019 20:33

use ones instead of random data

8e69083

add test case with 4x4 filter

94389b8

add doc related to padding

e1aca51

facaiy added the kokoro:force-run label May 31, 2019

facaiy self-assigned this May 31, 2019

facaiy added the awaiting testing (then merge) label May 31, 2019

kokoro-team removed the kokoro:force-run label May 31, 2019

facaiy approved these changes May 31, 2019

View reviewed changes

facaiy merged commit e49361b into tensorflow:master May 31, 2019

WindQAQ deleted the speed_up_mean_filter2d branch May 31, 2019 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up mean_filter2d with depthwise_conv2d #235

Speed up mean_filter2d with depthwise_conv2d #235

WindQAQ commented May 3, 2019

facaiy left a comment

facaiy May 5, 2019

WindQAQ May 24, 2019

WindQAQ May 24, 2019

facaiy May 27, 2019

WindQAQ May 27, 2019

WindQAQ May 27, 2019

facaiy May 28, 2019 •

edited

Loading

WindQAQ May 28, 2019

facaiy commented May 5, 2019

Mainak431 commented May 5, 2019

WindQAQ commented May 5, 2019

Mainak431 commented May 5, 2019

Mainak431 commented May 5, 2019 •

edited

Loading

WindQAQ commented May 5, 2019 •

edited

Loading

Mainak431 commented May 5, 2019

WindQAQ commented May 5, 2019

Mainak431 commented May 5, 2019 via email

facaiy commented May 7, 2019

facaiy commented May 23, 2019

WindQAQ commented May 23, 2019

WindQAQ commented May 24, 2019 •

edited

Loading

facaiy left a comment

bot-of-gabrieldemarmiesse commented Mar 9, 2020

seanpmorgan commented Mar 9, 2020

gabrieldemarmiesse commented Mar 9, 2020

gabrieldemarmiesse commented Mar 9, 2020

Speed up mean_filter2d with depthwise_conv2d #235

Speed up mean_filter2d with depthwise_conv2d #235

Conversation

WindQAQ commented May 3, 2019

facaiy left a comment

Choose a reason for hiding this comment

facaiy May 5, 2019

Choose a reason for hiding this comment

WindQAQ May 24, 2019

Choose a reason for hiding this comment

WindQAQ May 24, 2019

Choose a reason for hiding this comment

facaiy May 27, 2019

Choose a reason for hiding this comment

WindQAQ May 27, 2019

Choose a reason for hiding this comment

WindQAQ May 27, 2019

Choose a reason for hiding this comment

facaiy May 28, 2019 • edited Loading

Choose a reason for hiding this comment

WindQAQ May 28, 2019

Choose a reason for hiding this comment

facaiy commented May 5, 2019

Mainak431 commented May 5, 2019

WindQAQ commented May 5, 2019

Mainak431 commented May 5, 2019

Mainak431 commented May 5, 2019 • edited Loading

WindQAQ commented May 5, 2019 • edited Loading

Mainak431 commented May 5, 2019

WindQAQ commented May 5, 2019

Mainak431 commented May 5, 2019 via email

facaiy commented May 7, 2019

facaiy commented May 23, 2019

WindQAQ commented May 23, 2019

WindQAQ commented May 24, 2019 • edited Loading

facaiy left a comment

Choose a reason for hiding this comment

bot-of-gabrieldemarmiesse commented Mar 9, 2020

seanpmorgan commented Mar 9, 2020

gabrieldemarmiesse commented Mar 9, 2020

gabrieldemarmiesse commented Mar 9, 2020

facaiy May 28, 2019 •

edited

Loading

Mainak431 commented May 5, 2019 •

edited

Loading

WindQAQ commented May 5, 2019 •

edited

Loading

WindQAQ commented May 24, 2019 •

edited

Loading