make sure observations + new_obs treatment is the same

Is it possible to remove the replay handling from DDPG, and use SyncLocalReplayOptimizer / ApexOptimizer instead? Recently we refactored DQN to have replay be handled by the optimizer classes.

Note: a copy of these classes in in ray.rllib.optimizers already

Nice. It would be good to also

• make an issue to update the docs; there are a couple new algs not documented by now
• add a DDPG example to the regression tests folder (tuned_examples/regression_tests)
• add a DDPG sanity check to multi_node_tests.sh

@alvkao58 can you take care of these comments by Eric? (see PR)

any particular sanity checks you want to see added to multi_node_tests.sh?

don't you track the actor samples in samples["actions"] or something?

yeah, but looking at https://arxiv.org/pdf/1509.02971.pdf, aren't the actions according to what the actor currently outputs?

ok yeah good point

I think update_target is actually supposed to happen very often

Maybe let's try being a bit more explicit for now, just getting a specific action_gradient.

See self.action_grads in http://pemami4911.github.io/blog/2016/08/21/ddpg-rl.html

Let's move all the tensorflow specific things out of this class.

Let's move this back to slim and preferably as a Model we can import from ModelCatalog

remind me again why we are changing terminal to dones?

I did it for convenience because LocalSyncReplayOptimizer uses "obs", "new_obs", and "dones". I can change it back if necessary.

I made this consistent across all algorithms that use this sampler.

this ends up being a little verbose; consider

self.model = DDPGModel(
    self.registry, self.env, ... )

I think that if I don't do this, the model and target_model end up sharing weights?

would you want to keep this as a running average?

Is episode_reward_mean in TrainingResult meant to give an average from the start of time?

It will give an average over the number of episodes finished since the last TrainingResult.

I think here we should just do a lot of optimizer steps and return a TrainingResult only after a while.

I think we should move all of the Tensorflow stuff out of this class; it would make it a lot easier for us to support PyTorch.

** a note throughout- can we not do this style of indentation? and change it to

net = slim.fully_connected(
     obs, 400, activation_fn=tf.nn.relu, weights_initializer=w_normal)

can we move this into the models directory?

formatting for this is also a little odd; would be great to fix this.

You can reduce verbosity by just setting tau = self.config["tau"]

see ** a note throughout

nit: we should make a comment as to why this gamma (or should not be the gamma from the config)

nit: I believe you can just do np.mean(episode_rewards) and np.sum (below)to the same effect without the if-else cases

what happens when num_workers is 0? does the algorith still output anything?

As of now, it doesn't output anything when num_workers is 0, but I could update the stats so it takes metrics from the local evaluator when there's no remote evaluators.

OK let's do that

nit: it would be great if we could just have this as one return dict

Correct me if I'm wrong, but I think that would make setting weights more annoying?

That's true but only slightly; the benefit of doing this is to that different optimizers can use the DDPG evaluator (which is one of the main points of RLlib)

OK, do I also need to put the model and target_model weights in the same dictionary?

OK let's leave this as is and fix it in a later PR if necessary.

@alvkao58 can you take care of these comments by Eric? (see PR)