-
Notifications
You must be signed in to change notification settings - Fork 137
Conversation
@rxwei mind adding a build to this, I'll not let this be merged until the test is added, just want to confirm its stable till here. |
sorry for being AWOL, I'll take care of the rest of the PRs soon. This passes locally, If you want me to do any other changes let me know. |
@eaplatanios or @rxwei could you take a look at this? |
This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Shashi456!
It would nice to test derivatives for SeparableConv2D
, but that isn't set up yet. Filed #402 to track layer derivative tests.
@dan-zheng would it be okay, if I wrote the derivative tests in some other PR, I'm not very sure about how to do them, I'll go over your suggestions in the vjpconv2d PR. But I wouldn't want to block this PR because of that. WDYT? Moreover since separable is inherently dependent on Conv2D itself, if we test that then we might not have to test this as well. |
I think this can be tested and merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Strange that it failed, I had tested and built it locally. |
@rxwei So the test passed.
The error is this :
line no 299 in
|
The nightly toolchain used for CI is not up to date because of toolchain build failures. The team is looking into this. |
@rxwei Looks like the CI issue was fixed, Could you add a build to this? |
…o separable-conv
0a05b15
to
31e8dc2
Compare
The errors were because of being up to date with master. Fixed. |
This can be merged. |
Build Passes.
Will add a test in a while.
The last of Convolution Layer to be added.