-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add correlation cost layer #207
Conversation
For reference: |
Welcome, the GPU support is still in process: #118 |
I did a rebase to the master branch and fixed some path+lines in my code to adapt it to this addons-repo. I will not write the tf-eager-execution-crab. |
Lol thanks you're entitled to your opinion. As part of addons it has to be TF2 eager compatible. I would like to work on this, but will probably wait until dynamic kernels are finalized. Once it's done maybe we can benchmark autograph and graph mode performance of the same layer. I'll reach back out to you if/when I have questions if thats alright. |
Just glanced at the code it's all the op call it seems so probably will not need to benchmark. I'll see if we can adapt it to eager in the short term. GPU support might wait for dynamic kernels so we don't package 2 addons Thanks for the contribution! |
The GPU support is already in the BUILD for BAZEL (in the comments). I can do the adjustments for the GPU stuff when dynamic kernels with Cuda is described somewhere. Honestly, I tried the eager part for the tests, but I had no time to find a good way to compare CPU and GPU output. Btw: The workflow from the contribution.md in this repo with the docker image is nice and helpful! |
Any news on this? |
This got pushed for some other work, but GPU testing is now setup so I'll try to have this done by the end of the week |
@tensorflow/sig-addons-maintainers @olesalscheider @PatWie Ready for review. Tests are run in eager and graph, exposed as a Keras Layer and op if needed. GPU compiling / testing works. There is a fail on py2-gpu that seems unrelated to this as it fails on |
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.
I had a look at the changes in the following diff. Most of them are changes I would make. Otherwise, it looks good.
pad=pad, | ||
data_format=data_format) | ||
|
||
theoretical, numerical = tf.test.compute_gradient( |
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.
I would rename theoretical
to gradient_from_implementation
and numerical
to numerical_gradient
.
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.
I do like that re-name, but going to leave it so its consistent with TF Core:
https://github.com/tensorflow/tensorflow/search?utf8=%E2%9C%93&q=theoretical%2C+numerical&type=
@PatWie Hi, Patrick. Could you add some docstring for this class when time allows? I find its API document lacks necessary information: It seems that we should hide correlation_cost function, and only expose CorrelationCost in public. Please correct me if I'm wrong, thank you :-) |
@facaiy . The code is documented (just the c++ part)! Does addons/tensorflow_addons/custom_ops/layers/cc/ops/correlation_cost_op.cc Lines 92 to 109 in 7624954
help? Like I already stated, I'm not a big friend of Keras, TF2 and co. I create my graphs as I did since the first TF has been release. |
Unfortunately, I'm afraid that we only provide API document in python side for users. No worries, I filed an issue to seek help from community #509 |
Hi @PatWie , Since the documentation has already been done in C++ part. I am just copying and pasting it. @seanpmorgan , Please let me know if any other changes are there. I am new to open source and will appreciate your inputs. Thanks |
Hi @PyExtreme, many thanks for the help! What you mentioned above are the points for this issue. Just go ahead so that we could directly give you some feedback during review! Thanks again 😃 |
I just copied the files from my PR in tensorflow and slightly (+ blindly) adjusted the
BUILD
. Hence, this is really WIP.References:
I will rebase to master when I can run those ci test locally + adding a comprehensive commit message.
The docker command seems to not contain any way to run CUDA.