[WIP] Fix Issue #561 - Convolution Support NCHW

[2wins]

Checklist

• I've tested that my changes are compatible with the latest version of TensorFlow.
• I've read the Contribution Guidelines
• I've updated the documentation if necessary.

Motivation and Context

Issue #561

Description

• Conv1dLayer
• Conv2dLayer
• DeConv2dLayer
• Conv3dLayer
• DeConv3dLayer
• UpSampling2dLayer
• DownSampling2dLayer
• DeformableConv2d
• AtrousConv1dLayer
• AtrousConv2dLayer
• deconv2d_bilinear_upsampling_initializer
• Conv1d
• Conv2d
• DeConv2d
• DeConv3d
• DepthwiseConv2d
• SeparableConv1d
• SeparableConv2d
• GroupConv2d

[DEKHTIARJonathan]

@2wins It is a good thing that you thought about creating a unittest file.
I was wondering, don't you think that the most efficient and simplest way is to duplicate the file test_layers_convolution.py and create:

• test_layers_convolution_channel_first.py
• test_layers_convolution_channel_last.py

The exact same code just switching the position of the channnel dimension and adapting the tests.

[2wins]

@DEKHTIARJonathan It was not appropriate to duplicate test_layers_convolution.py directly because here data_format problem does not require output for testing (does not include any assertion requiring output). Anyway, as you said, I think dividing the file into two files are better.
Meanwhile, I found that Conv*dLayer using tf.nn.conv*d had problems. I think tf.nn.conv*d must not be affected by shape parameter but different data_format has an impact on shape.

[DEKHTIARJonathan]

@2wins I'm not sure to understand your point ;)

The objective of having the "same" file, modified accordingly to the position of the channel layer is not really about testing the output shape. The output shape can be seen as a sanity check ;)

No the rpincipal objective of the convolution test file is to be sure that no error is generated during the graph definition.

Thus, up to me, it is perfectly relevant to have the exact same file modified to use a the channel first data format.

Could you please explain a bit more why you think it's not appropriate, I may have missed smthg.

Thanks buddy

[2wins]

@DEKHTIARJonathan I agree with you. After no error on graph definition, sanity check is essential. I missed the important point. Thanks.

[DEKHTIARJonathan]

May you add me as a contributor to your fork ?
I would like to be able to help you directly. You started some quite heavy changes, you may want some help ;)

I will re-origanize the tests for you, I think some refactoring may help ;)

[DEKHTIARJonathan]

I'm not sure to understand what you want to do

[2wins]

@DEKHTIARJonathan Actually, if we want to provide two kinds of data_formats, then we allow users use consistent arguments e.g. only channel_last/first instead of NCHW/NHCW, or both of them. At that time, string mapping techniques are needed. (Some convs in TF take channel_last/first while others do NCHW/NHCW)
If we only support the argument channel_last (NHWC), we don't need such processing because a user don't put an argument into it and we just handle data_format internally.

[DEKHTIARJonathan]

@zsdonghao can I let you handle this PR ?

[2wins]

@DEKHTIARJonathan As seen in the @zsdonghao 's last comment, we can handle this issue by removing data_format and just using the argument set to channels_last internally.

[DEKHTIARJonathan]

@zsdonghao @2wins shall I close this PR ?

[2wins]

@DEKHTIARJonathan Actually, if we want to provide two kinds of data_formats, then we allow users use consistent arguments e.g. only channel_last/first instead of NCHW/NHCW, or both of them. At that time, string mapping techniques are needed. (Some convs in TF take channel_last/first while others do NCHW/NHCW)
If we only support the argument channel_last (NHWC), we don't need such processing because a user don't put an argument into it and we just handle data_format internally.

@DEKHTIARJonathan As seen in the @zsdonghao 's last comment, we can handle this issue by removing data_format and just using the argument set to channels_last internally.

@DEKHTIARJonathan, @zsdonghao As I said before, many conv layers are inconsistent in terms of argument data_format. Some of them take NCHW/NHWC and others do channels_first/channels_last. Meanwhile, some do not have data_format and process features (tensors) in a NHWC/channels_last way internally. We have to discuss it.

[zsdonghao]

It seen TF will use channel_last/first to replace all NCHW/NHCW, we can follow them.

[DEKHTIARJonathan]

@zsdonghao @2wins All right. Let's do this. I close this PR, we will follow this trend in PR #755 ;)