Add Adadelta optimizer

[Mistobaan]

This is my attempt at #516 . Looking for some early feedback as is my first attempt.

[wchan]

missing the GPU impl, did you forget to git add the file? cause the code references it but the file is missing ; )

also, I think a few lines are over 80 chars. but otherwise, LGTM.

[vrv]

@zffchen78: when you get a chance can you take a look at this?

[tensorflow-jenkins]

Can one of the admins verify this patch?

[zffchen78]

LGTM

[Mistobaan]

@vrv ok I will add the cuda part and do the edits !

[osdf]

Two suggestions:

• could you add a learning rate parameter lr? The original publication does not have it, but flexibility here is a good thing.
• could you rename decay_rate to rho, following the paper and the rmsprop impl in the same file?

I think there is a mistake:
https://github.com/Mistobaan/tensorflow/blob/7a262ee6467c909cae723e0de5fb87a2a7e9a664/tensorflow/core/kernels/training_ops.cc#L53
This line either should have accum = ... (instead of +=) or follow the respective line in the rmsprop implementation further down (https://github.com/Mistobaan/tensorflow/blob/7a262ee6467c909cae723e0de5fb87a2a7e9a664/tensorflow/core/kernels/training_ops.cc#L133).

[wchan]

FYI, there's a couple of bugs, namely the += shoulda been =

my impl is here (GPU included + separate lr included + sparse):
https://github.com/wchan/tensorflow/blob/master/tensorflow/core/kernels/training_ops.cc

[zffchen78]

Hi, mistobaan, please let us know how you plan to proceed on this? I can see the following work items:

1. address the accum issue raised by osdf and wchan. I do not have background to judge whether it's a bug or its your design choice. I can only review the implementation matches the specification, where your specification says accum +=; whether that matches the intended algorithm. It's up to you three to judge;
2. whether to add lr option. It's also up to you and others to discuss and agree on something.

1 & 2 must be settled before we can add this op since it affects the op's interface / specification and will be hard to fix later.

3. gpu support and test;
4. sparse support and test;

You can get 1&2 done and merged first; and collaborate w/ others to get 3&4 done or you can incrementally get them done later.

[Mistobaan]

Hi!
I am working on a patch that has all the above suggestions and the gpu code
from william chen

On Saturday, January 16, 2016, zffchen78 notifications@github.com wrote:

Hi, mistobaan, please let us know how you plan to proceed on this? I can
see the following work items:

1. address the accum issue raised by osdf and wchan. I do not have
   background to judge whether it's a bug or its your design choice. I can
   only review the implementation matches the specification, where your
   specification says accum +=; whether that matches the intended algorithm.
   It's up to you three to judge;
2. whether to add lr option. It's also up to you and others to discuss and
   agree on something.

1 & 2 must be settled before we can add this op since it affects the op's
interface / specification and will be hard to fix later.

1. gpu support and test;
2. sparse support and test;

You can get 1&2 done and merged first; and collaborate w/ others to get
3&4 done or you can incrementally get them done later.

—
Reply to this email directly or view it on GitHub
#644 (comment)
.

LinkedIn: http://linkedin.com/in/fmilo
Twitter: @fabmilo

Github: http://github.com/Mistobaan/

Simplicity, consistency, and repetition - that's how you get through. (Jack
Welch)
Perfection must be reached by degrees; she requires the slow hand of time
(Voltaire)
The best way to predict the future is to invent it (Alan Kay)

[vrv]

@Mistobaan: any updates?

[Mistobaan]

@vrv I think the operation themselves are up to date and correct. I was not able to create a solid testing for GPU as I don't have a supported GPU environment at the moment. I just rebased the patch.

[vrv]

Ok, we'll probably soon have GPU testing, right @martinwicke ? :). Then we can try pulling this in.

[vrv]

@tensorflow-jenkins: test this please

[tensorflow-jenkins]

Can one of the admins verify this patch?

[vrv]

tensorflow/core/kernels/training_ops.cc:338:8: error: template-id 'operator()<>' for 'void tensorflow::functor::ApplyAdadelta<Eigen::GpuDevice, double>::operator()(const GPUDevice&, tensorflow::TTypes<double, 1, long int>::Flat, tensorflow::TTypes<double, 1, long int>::Flat, tensorflow::TTypes<double, 1, long int>::Flat, tensorflow::TTypes<double, 1, long int>::ConstScalar, tensorflow::TTypes<double, 1, long int>::ConstScalar, tensorflow::TTypes<double, 1, long int>::ConstScalar, tensorflow::TTypes<double, 1, long int>::ConstFlat)' does not match any template declaration
void ApplyAdadelta<GPUDevice, T>::operator()(
^
tensorflow/core/kernels/training_ops.cc:348:1: note: in expansion of macro 'DECLARE_GPU_SPEC'
DECLARE_GPU_SPEC(double);
^
tensorflow/core/kernels/training_ops.cc:345:41: note: saw 1 'template<>', need 2 for specializing a member function template
typename TTypes::ConstFlat grad);
^
tensorflow/core/kernels/training_ops.cc:348:1: note: in expansion of macro 'DECLARE_GPU_SPEC'
DECLARE_GPU_SPEC(double);
^
tensorflow/core/kernels/training_ops.cc:352:17: error: expected constructor, destructor, or type conversion before '(' token
REGISTER_KERNELS(GPU, float);
^
tensorflow/core/kernels/training_ops.cc:353:17: error: expected constructor, destructor, or type conversion before '(' token
REGISTER_KERNELS(GPU, double);

[vrv]

(ping this thread when this is ready -- it looks like you added a commit but don't know if it's ready)

[bernardopires]

@Mistobaan, any update on this? I'm also really interested in trying AdaDelta. Thanks!

[madisonmay]

Docstring references Momentum instead of AdaDelta.

[vrv]

@tensorflow-jenkins: test this please

[vrv]

@Mistobaan: I think this looks good pending the GPU tests finishing -- the only other thing I think we need is for you to run

bazel-bin/tensorflow/core/ops/compat/update_ops tensorflow/core/ops

and add that updated file to your commit for tracking backwards compatibility.

[Mistobaan]

@vrv I run and added the modified files as you requested.

[vrv]

@tensorflow-jenkins: test this please

[vrv]

Looks like nothing built -- can you double check and verify that your commit compiles and passes in at least one config?

[Mistobaan]

@vrv let's try again I ran the updated command on an old branch and force pushed

[vrv]

@tensorflow-jenkins: test this please