DDPG implementation correctness

Hi! I'm trying to implement DDPG as well based on paper [Continuous control with deep reinforcement learning](http://arxiv.org/pdf/1509.02971.pdf). Though without much success yet... So I was looking for other's implementations and found your repo :)

Looking through your code (this repo and [ipython-notebooks](https://github.com/hans/ipython-notebooks)) I found some differences with my understanding and implementation of the paper. Probably because my implementation is not working very well, your code is more correct, but I'd like to clarify several points.

Let's start with this repo and Tensorflow implementation.

[**rlcomp/dpg.py**: __make_graph_](https://github.com/hans/rlcomp/blob/master/rlcomp/dpg.py#L120)

``` python
# Build tracking models.
self.a_pred_track = policy_model(self.inputs, self.mdp_spec, self.spec,
track_scope="%s/policy" % self.name,
name="policy_track")
self.critic_on_track = critic_model(self.inputs, self.a_pred, self.mdp_spec,
self.spec, name="critic_track",
track_scope="%s/critic" % self.name)
```

If I understand correctly _track_ term here is just synonym of _target_ in the paper and _tracking_ models are just copies of _current_ models which weights are updated with some delay. These models are used to calculate new Q values (`q_targets` in your code)
So my understanding is that `critic_on_track` should depend on `a_pred_track` rather than on `a_pred`, because in formula above both Q' and mu' are **target** versions of Q and mu.

[**rlcomp/dpg.py**: __make_objectives_](https://github.com/hans/rlcomp/blob/master/rlcomp/dpg.py#L142)

``` python
# Critic objective: minimize MSE of off-policy Q-value predictions
q_errors = tf.square(self.critic_off - self.q_targets)
self.critic_objective = tf.reduce_mean(q_errors)
```

Comparing to the paper:
As seen in formula for critic loss they use  which is not the same as `critic_off` in your implementation because `critic_off` doesn't use actions _a_i_ sampled from replay buffer, but instead it depends on `a_explore` that generate new actions each time `a_i = a_explore = mu(s_i) + noise`.

[**rlcomp/tasks/cartpole.py**: _train_batch_](https://github.com/hans/rlcomp/blob/master/rlcomp/tasks/cartpole.py#L78)

``` python
# Sample a training minibatch.
b_states, b_actions, b_rewards, b_states_next = buffer.sample(FLAGS.batch_size)

# Compute targets (TD error backups) given current Q function.
a_next, q_next = sess.run([dpg.a_pred_track, dpg.critic_on_track],
{dpg.inputs: b_states_next})
b_targets = b_rewards + FLAGS.gamma * q_next.flatten()

# Policy update.
sess.run(policy_update, {dpg.inputs: b_states})

# Critic update.
cost_t, _, q_pred = sess.run([dpg.critic_objective, critic_update, dpg.critic_off],
{dpg.inputs: b_states, dpg.q_targets: b_targets})
```

This piece of code I just included to be able to see everything in one place. You can see that `b_actions` are not used. It looks suspicious to me...

Here is theirs algorithm for convenience:

Then I found your Theano-based implementation :) Let's look there.

[DPG for CartPole.ipynb](https://github.com/hans/ipython-notebooks/blob/master/DPG%20for%20CartPole.ipynb): _train_batch_

``` python
# Sample a training minibatch.
idxs = np.random.choice(len(R_states) - 1, size=batch_size, replace=False)
b_states, b_actions, b_rewards, b_states_next = \
R_states[idxs], R_actions[idxs], R_rewards[idxs], R_states_next[idxs]

# Compute targets (TD error backups) given current Q function.
next_actions = dpg.track.f_action_on(b_states_next)
b_targets = b_rewards + gamma * dpg.track.f_q(b_states_next, next_actions).reshape((-1,))

# SGD update.
cost_t, _ = dpg.f_update(b_states, b_targets, lr_actor, lr_critic)
# Update tracking model.
dpg.track.f_track_update(tau)
```

As in previous implementation `b_actions` are not used. But unlike it here you use both tracking versions of actor and critic models for predicting new Q values, that seems correct.

[DPG for CartPole.ipynb](https://github.com/hans/ipython-notebooks/blob/master/DPG%20for%20CartPole.ipynb): __make_updates_

``` python
# Actor-critic learning w/ critic 3
# NB, need to flatten all timesteps into a single batch
self.updates = OrderedDict(self.critic_exp.updates)
# Add policy gradient updates
self.updates.update(SGD(-self.critic_exp.pred.mean(),
self._vs_actor.vars.values(),
self.lr_actor))
# Target network: update w.r.t. parent
if self.parent is not None:
self.target_updates = OrderedDict()
for vs, parent_vs in [(self._vs_actor, self.parent._vs_actor),
(self._vs_critic, self.parent._vs_critic)]:
for param_name, param_var in vs.vars.iteritems():
self.target_updates[param_var] = (
self.tau * vs.vars[param_name]
+ (1 - self.tau) * parent_vs.vars[param_name])
```

Still same question: why `critic_exp` are used in cost calculation? I assume `critic_given` should be used with actions sampled from replay buffer. Please, correct me if I'm wrong.

Another issue that I see, is in updating target network. As I understand, in this case `self.parent` is _current_ model from which weights should gradually be copied to **target** which is `self` in this case. So if `self.tau` is small like **0.001** then you should take **0.999** of _previous target value_ + **0.001** of _new value_: `(1 - self.tau) * vs.vars[param_name] + self.tau * parent_vs.vars[param_name])`

That's all my findings. I'm quite new to TF and Theano and RL, so they all could be wrong :) But I really would like hear you comments on them. As I said, my implementation doesn't converge to something meaningful, so I'm wondering whether this is due to misinterpreting the paper or may be I just need to tune some hyperparams. Did you achieve good results, BTW?