Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

easadler
Copy link
Contributor

@easadler easadler commented Apr 25, 2018

This PR adds tests to make sure that the gradients fortf.convo1d match tensorflow. I believe this has to do with tensorflow/tfjs#108.


This change is Reviewable

@dsmilkov
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


src/ops/conv.ts, line 111 at r1 (raw file):

                   .as3D(filter.shape[0], filter.shape[1], filter.shape[2])
      };
    };

Since conv1d calls conv2d, both are high-level ops, and conv2d has already gradients, we don't need to explicitly implement gradients for conv1d - it will happen automatically. What is veru useful, and you already have started on, is to write bunch of unit tests to make sure the gradient works and compare it with TF in python (http://colab.research.google.com is very useful for testing TF Python) Thanks!


Comments from Reviewable

@easadler
Copy link
Contributor Author

easadler commented May 2, 2018

I added a 2nd test and found that the gradient for x returned all zeros instead of what was expected from tensorflow. google colab notebook.

Error: Arrays differ: actual[0] = 0, expected[0] = 9.
Actual:   0,0,0,0,0,0,0,0,0,0,0,0,0,0.
Expected: 9,12,10,4,10,12,10,4,10,12,10,4,1,0.

@easadler easadler changed the title add tf.conv1d gradient add tests for tf.conv1d gradients May 2, 2018
dsmilkov added a commit that referenced this pull request May 3, 2018
BUG

Tests are coming in #992 from an external contributor, who found the bug.
@dsmilkov
Copy link
Contributor

dsmilkov commented May 3, 2018

Your tests found a bug in our CPU backend, which we just fixed in #1014. I will sync this PR to master, and your tests should pass, at which point I'll merge this in. Thank you!

@tensorflow tensorflow deleted a comment from googlebot May 3, 2018
@dsmilkov
Copy link
Contributor

dsmilkov commented May 3, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from Reviewable

@tensorflow tensorflow deleted a comment from googlebot May 3, 2018
@dsmilkov dsmilkov merged commit b062378 into tensorflow:master May 3, 2018
@easadler easadler deleted the convo1d-grad branch May 3, 2018 15:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants