New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug with _causal_dilated_conv? #29

Closed
JesseYang opened this Issue Sep 18, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@JesseYang

JesseYang commented Sep 18, 2016

I test _causal_dilated_conv in wavenet.py with the following toy example:
batch_size: 1
height: 1
width: 20
in_channel: 1
out_channel: 1
dilation: 4

The “value” tensor is set with shape [1, 1, 20, 1], and the values are from 1 to 20. The “filter” is set with shape [1, 2, 1, 1], and both values are 1.

The ”out“ tensor is:
[[[[ 3.], [ 5.], [ 7.], [ 9.], [ 13.], [ 15.], [ 17.], [ 19.], [ 23.], [ 25.], [ 27.], [ 29.], [ 33.], [ 35.], [ 37.], [ 39.]]]]

I suppose the correct ”out“ should be:
[[[[ 6.], [ 8.], [ 10.], [ 12.], [ 14.], [ 16.], [ 18.], [ 20.], [ 22.], [ 24.], [ 26.], [ 28.], [ 30.], [ 32.], [ 34.], [ 36.]]]]

In this example, the "reshaped" tensor is:
[[[[ 1.], [ 2.], [ 3.], [ 4.], [ 5.]]],
[[[ 6.], [ 7.], [ 8.], [ 9.], [ 10.]]],
[[[ 11.],[ 12.],[ 13.],[ 14.],[ 15.]]],
[[[ 16.],[ 17.],[ 18.],[ 19.],[ 20.]]]]

I guess it should be
[[[[ 1.], [ 5.], [ 9.], [ 13.], [ 17.]]],
[[[ 2.], [ 6.], [ 10.], [ 14.], [ 18.]]],
[[[ 3.], [ 7.], [ 11.], [ 15.], [ 19.]]],
[[[ 4.], [ 8.], [ 12.], [ 16.], [ 20.]]]]
and then the result should be correct.

@JesseYang JesseYang changed the title from but with _causal_dilated_conv? to Bug with _causal_dilated_conv? Sep 18, 2016

@ibab ibab added the bug label Sep 18, 2016

@ibab

This comment has been minimized.

Show comment
Hide comment
@ibab

ibab Sep 18, 2016

Owner

Thanks for spotting this!
The problem is that the function is missing transpose operations.
I'll work on fixing this.

Owner

ibab commented Sep 18, 2016

Thanks for spotting this!
The problem is that the function is missing transpose operations.
I'll work on fixing this.

ibab added a commit that referenced this issue Sep 18, 2016

Fix causal dilated convolution
The dilated convolution was missing transpose steps.
The problem was pointed out in #29.
@ibab

This comment has been minimized.

Show comment
Hide comment
@ibab

ibab Sep 18, 2016

Owner

I've carefully rewritten the causal convolution in 5e1c323, and made sure to test it heavily.
@JesseYang: Could you have a look to check whether it works as you would expect? I've moved the functions to wavenet_ops.py.

This demonstrates that we definitely need unit tests :(
I'll add a few for the causal convolution.

Owner

ibab commented Sep 18, 2016

I've carefully rewritten the causal convolution in 5e1c323, and made sure to test it heavily.
@JesseYang: Could you have a look to check whether it works as you would expect? I've moved the functions to wavenet_ops.py.

This demonstrates that we definitely need unit tests :(
I'll add a few for the causal convolution.

@jyegerlehner

This comment has been minimized.

Show comment
Hide comment
@jyegerlehner

jyegerlehner Sep 18, 2016

Contributor

Outstanding. By the time I figured out what JesseYang was talking about, ibab already committed a fix and unit test. I can't keep up with you bright young people.

Contributor

jyegerlehner commented Sep 18, 2016

Outstanding. By the time I figured out what JesseYang was talking about, ibab already committed a fix and unit test. I can't keep up with you bright young people.

@ibab

This comment has been minimized.

Show comment
Hide comment
@ibab

ibab Sep 18, 2016

Owner

I'll close this issue now, as the causal conv seems to be working correctly.

Owner

ibab commented Sep 18, 2016

I'll close this issue now, as the causal conv seems to be working correctly.

@ibab ibab closed this Sep 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment