Add padding layer. #979

hzxie · 2018-08-17T13:16:40Z

I created padding layer because TinyDNN hasn't support add padding in convolutional layers.
The padding layer is similar to tf.pad in Tensorflow.

hzxie · 2018-08-17T13:18:13Z

Here's the code for testing.

#include <cstdlib>
#include <iostream>
#include <vector>

#include "tiny_dnn/tiny_dnn.h"

class TestNet : public tiny_dnn::network<tiny_dnn::sequential> {
 public:
  explicit TestNet(const std::string &name = "")
    : tiny_dnn::network<tiny_dnn::sequential>(name) {
    using pad     = tiny_dnn::layers::pad;
    *this << pad(5, 5, 2, 1, 2);
  }
};

int main() {
  TestNet nn;
  
  int init_value = 0;
  tiny_dnn::vec_t in(5 * 5 * 2);
  for ( auto itr = in.begin(); itr != in.end(); ++ itr ) {
    *itr = init_value ++;
  }

  int cnt = 0;
  auto res = nn.predict(in);
  for ( auto itr = res.begin(); itr != res.end(); ++ itr ) {
    std::cout << *itr << "\t";
    if ( ++ cnt % 7 == 0 ) {
      std::cout << std::endl;
    }
    if ( cnt % 63 == 0 ) {
      std::cout << std::endl;
    }
  }
  std::cout << std::endl;
}

The output is listed as follows

0	0	0	0	0	0	0	
0	0	0	0	0	0	0	
0	0	1	2	3	4	0	
0	5	6	7	8	9	0	
0	10	11	12	13	14	0	
0	15	16	17	18	19	0	
0	20	21	22	23	24	0	
0	0	0	0	0	0	0	
0	0	0	0	0	0	0	

0	0	0	0	0	0	0	
0	0	0	0	0	0	0	
0	25	26	27	28	29	0	
0	30	31	32	33	34	0	
0	35	36	37	38	39	0	
0	40	41	42	43	44	0	
0	45	46	47	48	49	0	
0	0	0	0	0	0	0	
0	0	0	0	0	0	0

beru · 2018-08-19T07:44:22Z

@hzxie

Here's the code for testing.

Adding unit tests would be better. In that way, future changes can be tested.

travis-ci jobs failed.
https://travis-ci.org/tiny-dnn/tiny-dnn/builds/417261964?utm_source=github_status&utm_medium=notification

Running clang-format with scripts/clang-format.sh is required. But as I mentioned in #978 (comment) , this requirement is cumbersome and I personally think this task should be removed for contributors.

hzxie · 2018-08-22T09:03:30Z

@beru @edgarriba
All unit tests for padding layer is created and passed.
BTW, CI often failed due to the failure of unit testing cases named train2.

Maybe we should enlarge the threshold, 1e-4 is too small?
See: https://travis-ci.org/tiny-dnn/tiny-dnn/builds/419060831

edgarriba

the PR looks good, however I would like to see more specific names since your implementation is ZeroPadding2d (in pytorch terminology). We can leave it prepared to handle more padding modes by now

UPDATE: https://pytorch.org/docs/stable/nn.html?highlight=padding#zeropad2d

hzxie · 2018-08-22T13:25:08Z

@edgarriba
The layer name has changed to zero_pad_layer.
But Travis CI failed because of failure of train2 test case (The reason was mentioned above).
The coverage decreased for an unknown reason. (Because in the previous commit the coverage increased.)

hzxie · 2018-08-24T12:07:38Z

@edgarriba @beru
Please come here to merge my PR.
Thank you.

edgarriba · 2018-08-24T12:47:00Z

@hzxie can you investigate this issue with train2 ? I remember @prlz77 mentioned at some point that this was due to the gradient check he implemented.

hzxie · 2018-08-24T16:07:21Z

Actually, #978 also failed on train2 sometimes.
In this PR, there’s nothing related to train2.
I think I can fix this issue in another PR.

hzxie · 2018-08-24T16:12:58Z

@edgarriba
Can you give me some hints to fix this problem? Because train2 only failed in some platforms.
What did @prlz77 said?

hzxie · 2018-08-24T18:41:04Z

@edgarriba
It seems that CI errored on all platforms.

Here's the error log:

CMake Error in CMakeLists.txt:
  export called with target "gmock" which requires target "gtest" that is not
  in the export set.
  If the required target is not easy to reference in this call, consider
  using the APPEND option with multiple separate calls.
-- Generating done
-- Build files have been written to: /home/travis/build/tiny-dnn/tiny-dnn
The command "if [ "$TRAVIS_OS_NAME" == "linux" ] && [ "$CXX" == "g++-4.9" ]; then lcov --directory . --zerocounters; cmake -DUSE_TBB=$USE_TBB -DUSE_SSE=$USE_SSE -DUSE_AVX=$USE_AVX -DUSE_DOUBLE=$USE_DOUBLE -DBUILD_TESTS=$BUILD_TESTS -DCOVERALLS=$COVERALLS -DUSE_ASAN=ON -DBUILD_EXAMPLES=$BUILD_EXAMPLES .; fi" failed and exited with 1 during .
Your build has been stopped.

hzxie · 2018-08-24T20:17:36Z

#986 also failed.
Maybe there's something wrong with TravisCI?

beru · 2018-08-25T09:32:01Z

@hzxie It looks like recent googletest change is the cause. polybar/polybar#1393

I'm wondering if adding

option(INSTALL_GMOCK "Install Googletest's GMock?" OFF)
option(INSTALL_GTEST "Install Googletest's GTest?" OFF)

before

tiny-dnn/test/CMakeLists.txt

Line 14 in ceb8b9f

add_subdirectory(${googletest_SOURCE_DIR} ${googletest_BINARY_DIR})

might solve the problem...

beru · 2018-08-25T09:35:04Z

Talking about unit tests.

https://github.com/catchorg/Catch2 is also famous test framework. But I guess rewriting unit tests isn't trivial.

edgarriba · 2018-08-25T10:05:35Z

@beru Catch2 looks pretty good and fits perfect with our header-only policy. If anyone wants to take this task, why not.

UPDATE: I suggest discussing it first in
https://groups.google.com/forum/#!forum/tiny-dnn-dev

hzxie · 2018-08-25T11:31:23Z

Rewriting unit testing cases is time-consuming.
I think we need to fix CI failure first.

edgarriba · 2018-08-25T17:32:53Z

@hzxie agree. This PR #968 tries to fix this, take a look at it. Besides, what makes sense now is to rethink about how to setup a proper infrastructure with travis since lately things became overcomplicated with the more third-party stuff we added. At some point we should introduce docker based testing. We internally had this discussion. We should probably start a proper discussion about it. Let me open a thread since there are more some folks interested in this as well @Ravenwater @chrberger

UPDATE: There's already an open thread where we shared very few opinions.
https://groups.google.com/forum/#!topic/tiny-dnn-dev/ZdZLAE7Go30

beru · 2018-08-25T21:19:19Z

@hzxie I added a wiki page about how to update AUTHORS file.

https://github.com/tiny-dnn/tiny-dnn/wiki/How-to-update-AUTHORS-file

Please check it out.

beru · 2018-08-26T02:43:17Z

@hzxie

Rewriting unit testing cases is time-consuming.
I think we need to fix CI failure first.

I initially thought so but couldn't figure out the cause of googletest error with cmake.
So I've decided to change test framework to Catch2.

Please review #987.

beru · 2018-08-26T02:47:01Z

It seems that CI errored on all platforms.

Actually, it's OK with VC++ (Windows platform) builds but I'm not sure why.

beru · 2018-08-26T03:42:15Z

@edgarriba

Catch2 looks pretty good and fits perfect with our header-only policy. If anyone wants to take this task, why not.

I want to remove CI job problems, so made a pull request. #987

UPDATE: I suggest discussing it first in
https://groups.google.com/forum/#!forum/tiny-dnn-dev

Maybe next time.

hzxie · 2018-08-26T05:25:56Z

@beru

The AUTHORS file has been updated.
I will push it after PR #987 is merged.

BTW, I suggest that the last line in this file should be changed to:

Generated by https://github.com/nodejs/node/blob/master/tools/update-authors.sh

What do you think?

edgarriba · 2018-08-26T09:33:45Z

@hzxie why don't we just reuse this script ?

beru · 2018-08-26T09:38:51Z

@hzxie

BTW, I suggest that the last line in this file should be changed to:
Generated by https://github.com/nodejs/node/blob/master/tools/update-authors.sh
What do you think?

I think it's fine. In that way, we can show homage to nodejs team.

It's a short script file, so adding the file to scripts folder seems OK.
In that way, we'll be able to customize the file. And we can also mention where it originally came from in a commit message.

beru · 2018-08-26T21:07:23Z

@edgarriba

why don't we just reuse this script ?

The script appends comments after authors list but those comments were manually removed.

https://github.com/tiny-dnn/tiny-dnn/commits/master/AUTHORS

It would be easier to understand if you run the script.

edgarriba · 2018-08-26T22:06:38Z

@beru reuse or adapt

beru · 2018-08-26T22:45:57Z

@edgarriba

reuse or adapt

I see. So adding the file to scripts folder seems fine.
We can leave tail comments in a natural way. It can be adjusted as well.

Ignore files generated by CMake.

Fix some bugs in .gitignore.

hzxie · 2018-08-27T09:38:42Z

@beru @edgarriba
Please review this PR.
I updated my test code and added the script for updating authors.

BTW, the new test script is not clear as before.
I don't know whether my newly added test script is run or not.
I think we should print the name of each test case before we run each test case.

edgarriba · 2018-08-27T09:43:26Z

@hzxie LGTM. However, you should split the commit one for padding and the other to update the authors script.

hzxie · 2018-08-27T10:27:04Z

@edgarriba
The script for updating authors was removed.
The AUTHORS file was reverted to the original version.

Please consider merging this PR.

hzxie · 2018-08-27T10:33:59Z

@edgarriba @beru
Please also review PR #989.

beru · 2018-08-27T12:07:06Z

.gitignore

@@ -219,4 +219,4 @@ docs/_build
 *.pb.h
 CMakeFiles*
 build*
-DartConfiguration.tcl
+DartConfiguration.tcl


No new line at end of file

beru · 2018-08-27T12:07:29Z

AUTHORS

@@ -72,4 +72,4 @@ Karan Desai <karandesai_96@live.com>
 MeiHui FAN <mhfan@ustc.edu>
 Rui Huang <vowstar@users.noreply.github.com>
 Siddhartha Rao Kamalakara <srk97c@gmail.com>
-Jaakko Rantala <jaakko.rantala@gmail.com>
+Jaakko Rantala <jaakko.rantala@gmail.com>


No new line at end of file

beru · 2018-08-27T12:09:30Z

tiny_dnn/tiny_dnn.h

@@ -102,6 +103,8 @@ using fc = tiny_dnn::fully_connected_layer;

 using dense = tiny_dnn::fully_connected_layer;

+using zreo_pad = tiny_dnn::zero_pad_layer;


I've found a typo. zreo_pad should be zero_pad.

OMG.
Thank you!

beru

LGTM.

edgarriba requested changes Aug 22, 2018

View reviewed changes

edgarriba approved these changes Aug 24, 2018

View reviewed changes

hzxie added 3 commits August 27, 2018 16:56

Add padding layer.

44dfecc

Remove iostream in pad_layer.h.

80948d4

Update .gitignore

dc7d019

Ignore files generated by CMake.

hzxie added 10 commits August 27, 2018 16:58

Add padding layer.

ab0da22

Remove iostream in pad_layer.h.

b7a2916

Create unit testing for pad_layer.

f978fb6

Meet the standard of clang-format.

501244f

Add comments for pad_layer.

b1eec90

Update .gitignore

bee880a

Fix some bugs in .gitignore.

Update README.md.

b1a3c15

Rename pad_layer to zero_pad_layer.

9befb17

Update README.md

3594dc6

Create script for updating authors (thanks for @nodejs).

407be99

edgarriba approved these changes Aug 27, 2018

View reviewed changes

Revert AUTHORS to original and delete the script for updating authors.

3aabfd5

beru reviewed Aug 27, 2018

View reviewed changes

Add a new line at the end of AUTHORS and .gitignore.

ec0bdad

beru reviewed Aug 27, 2018

View reviewed changes

Fix a typo.

be95073

beru approved these changes Aug 27, 2018

View reviewed changes

beru merged commit 1ce9b17 into tiny-dnn:master Aug 27, 2018

hzxie deleted the feat/pad_layer branch August 29, 2018 05:58

hzxie mentioned this pull request Oct 2, 2018

Create L2 Normalization Layer #994

Merged

		@@ -102,6 +103,8 @@ using fc = tiny_dnn::fully_connected_layer;

		using dense = tiny_dnn::fully_connected_layer;

		using zreo_pad = tiny_dnn::zero_pad_layer;

Add padding layer. #979

Add padding layer. #979

Conversation

hzxie commented Aug 17, 2018

hzxie commented Aug 17, 2018

beru commented Aug 19, 2018

hzxie commented Aug 22, 2018 • edited Loading

edgarriba left a comment • edited Loading

Choose a reason for hiding this comment

hzxie commented Aug 22, 2018

hzxie commented Aug 24, 2018

edgarriba commented Aug 24, 2018

hzxie commented Aug 24, 2018

hzxie commented Aug 24, 2018

hzxie commented Aug 24, 2018

hzxie commented Aug 24, 2018

beru commented Aug 25, 2018

beru commented Aug 25, 2018

edgarriba commented Aug 25, 2018 • edited Loading

hzxie commented Aug 25, 2018

edgarriba commented Aug 25, 2018 • edited Loading

beru commented Aug 25, 2018

beru commented Aug 26, 2018

beru commented Aug 26, 2018 • edited Loading

beru commented Aug 26, 2018

hzxie commented Aug 26, 2018

edgarriba commented Aug 26, 2018

beru commented Aug 26, 2018

beru commented Aug 26, 2018

edgarriba commented Aug 26, 2018

beru commented Aug 26, 2018

hzxie commented Aug 27, 2018

edgarriba commented Aug 27, 2018

hzxie commented Aug 27, 2018

hzxie commented Aug 27, 2018 • edited Loading

beru Aug 27, 2018

Choose a reason for hiding this comment

beru Aug 27, 2018

Choose a reason for hiding this comment

beru Aug 27, 2018

Choose a reason for hiding this comment

hzxie Aug 27, 2018

Choose a reason for hiding this comment

beru left a comment

Choose a reason for hiding this comment

hzxie commented Aug 22, 2018 •

edited

Loading

edgarriba left a comment •

edited

Loading

edgarriba commented Aug 25, 2018 •

edited

Loading

edgarriba commented Aug 25, 2018 •

edited

Loading

beru commented Aug 26, 2018 •

edited

Loading

hzxie commented Aug 27, 2018 •

edited

Loading