VecEnc Support TD3#495
Conversation
|
Ah, it seems like I would first have to make HER work with VecEnv. @araffin Any idea where to begin with that? |
| self._save_to_file(save_path, data=data, params=params_to_save, cloudpickle=cloudpickle) | ||
|
|
||
|
|
||
| class Runner(AbstractEnvRunner): |
There was a problem hiding this comment.
Why do you need a runner? It seems that you only need to save the obs variable.
There was a problem hiding this comment.
I noticed that other implementations that uses VecEnv use a Runner. I used a Runner here as I feel like that best enables future developers to build on top of it.
There was a problem hiding this comment.
PPO and A2C use a runner because some additional computation/transformations are needed (computation of the GAE notably) which is not the case of TD3 who only need to fill a replay buffer.
However, at some point, we will need to refactor and unify SAC/DDPG/TD3 which have a lot in common.
There was a problem hiding this comment.
Ah cool. So perhaps some akin to a "runner" for all three?
There was a problem hiding this comment.
Not really, more a common method collect_rollout that would be part of the OffPolicy class, but this is not the subject of this PR.
| else: | ||
| action = self.policy_tf.step(obs[None]).flatten() | ||
| action = self.policy_tf.step(prev_obs).flatten() | ||
| action = [np.array([a]) for a in action] |
There was a problem hiding this comment.
why not removing the flatten instead?
| # Add noise to the action, as the policy | ||
| # is deterministic, this is required for exploration | ||
| if self.action_noise is not None: | ||
| action = np.clip(action + self.action_noise(), -1, 1) |
There was a problem hiding this comment.
The noise should be different for each env
| episode_rewards[-1] += reward[i] | ||
| if done[i]: | ||
| if self.action_noise is not None: | ||
| self.action_noise.reset() |
There was a problem hiding this comment.
same remark as before, I think you should have a action_noise object per env, maybe we need to create wrapper for that or modify the noise class to handle it better
| if step % self.train_freq == 0: | ||
| mb_infos_vals = [] | ||
| # Update policy, critics and target networks | ||
| for grad_step in range(self.gradient_steps): |
There was a problem hiding this comment.
By putting the update inside the for loop that is used to store new samples, it seems that you are changing the algorithm
Also be careful with step % train_freq when you don't increment step by 1
There was a problem hiding this comment.
Ok, I will look into the algorithm part. As for the step % train_freq, I actually do increment step by 1 (at the end of the inner for-loop) so that should be fine.
There was a problem hiding this comment.
ok then, but the for step in range(0, total_timesteps, self.n_envs): was misleading
There was a problem hiding this comment.
I see your point. Alright, I will clarify that for-loop expression.
2 participants
Implemented SubprocVecEnv for TD3.
Related: #452 #170