Implement HER

[araffin]

EDIT: we might want to support Dict obs for VecNormalize (maybe another issue)

[ferreirajoaouerj]

Maybe worth it to implement similar to baselines2. Create an environment wrapper to return concatenated states and goals and then wrap the buffer, so that goal relabeling is applied at the end of the episode.

[araffin]

Maybe worth it to implement similar to baselines2. Create an environment wrapper to return concatenated states and goals and then wrap the buffer, so that goal relabeling is applied at the end of the episode.

In fact, I would prefer a separate algorithm that does not require a wrapper, even though it means a bit of code duplication. The SB2 solution works but is quite messy and requires a lot of data transformation.

[arjun-kg]

I'm working on something similar. Can I take a shot at this? With DDPG (and maybe SAC)

[araffin]

Yes you can, but you just have to assume the model derives from the off policy class and therefore has a replay buffer.
it should not be specific to td3/dqn/sac.

i may also have a student on that one (cf roadmap) but her code needs to be adapted a bit. i would like also to compare performances when the transitions are added at sampling time (cf openai baselines code).

[arjun-kg]

I've created a PR for a HER implementation (#42). It's basically reusing elements from the existing SB implementation (with some minor edits). It works for the existing off-policy algos. Please check if this is usable. I'll take a shot at removing wrappers and minimizing data transformations next. This is my first PR, so I apologize if some things are not proper and do let me know if there is additional work to be done.

[arjun-kg]

Regarding adding HER transitions at sampling time, wouldn't that mean that the samples are more correlated than before, since HER samples would come from the same episode as the original transitions we sample. This, as opposed to uncorrelated samples while drawing randomly from a buffer filled with original + HER

[araffin]

Thank you for the PR.
I wanted to avoid doing an implementation as in SB2... Also it seems that you used the wrong branch as base. Maybe wait for the dqn to be merged first.

[araffin]

As mentioned before, I've got a student (@megan-klaiber ) that implemented HER (minimal implementation with an old version of SB3) as a programming test a while ago. If she agrees, we could use this implementation as a base.
Unfortunately, her contract did not start yet...

[araffin]

Regarding adding HER transitions at sampling time, wouldn't that mean that the samples are more correlated than before, since HER samples would come from the same episode as the original transitions we sample. This, as opposed to uncorrelated samples while drawing randomly from a buffer filled with original + HER

Take a look at Baselines implementation. The two should be equivalent

[megan-klaiber]

As mentioned before, I've got a student (@megan-klaiber ) that implemented HER (minimal implementation with an old version of SB3) as a programming test a while ago. If she agrees, we could use this implementation as a base.
Unfortunately, her contract did not start yet...

Yes, sure you can use the code. Unfortunately, I haven't had time to work on it yet.

[arjun-kg]

Thank you for the PR.
I wanted to avoid doing an implementation as in SB2... Also it seems that you used the wrong branch as base. Maybe wait for the dqn to be merged first.

Didn't realize this. Used master as base and updated the code and PR now

[arjun-kg]

As mentioned before, I've got a student (@megan-klaiber ) that implemented HER (minimal implementation with an old version of SB3) as a programming test a while ago. If she agrees, we could use this implementation as a base.
Unfortunately, her contract did not start yet...

Yes, sure you can use the code. Unfortunately, I haven't had time to work on it yet.

Thanks. I'll work on removing wrappers and reducing data transformations in the meantime, and continue updating. Will also look at baselines implementation for sampling

[araffin]

Thanks. I'll work on removing wrappers and reducing data transformations in the meantime, and continue updating. Will also look at baselines implementation for sampling

I'll try to push @megan-klaiber implementation next week, but I won't take at look at your PR before #28 and #35 are merged.
In the meantime, make sure to read the contributing guide and that all the tests passes.

[araffin]

You can find attached @megan-klaiber implementation. What I have in mind is a mix between SB2 implementation and her implementation.
To be more precise:

• I would minimize transformations and avoid using a wrapper for the buffer, even if it means a bit of code duplication
• I would keep the enum that was present in SB2 to define the type of goals
• for the env wrapper, I would prefer a VecEnvWrapper instead of a gym.Wrapper, this would allow multiprocessing in the future. Also, once with have dict support for observation space (cf roadmap Roadmap to Stable-Baselines3 V1.0 #1 ), we will be able to remove that wrapper.

But again, please wait that #28 and #35 are merged before requesting review (that does not mean you cannot work on it).

her_megan_solution.zip

[arjun-kg]

Thank you for the code. Do you have a timeline in mind as to when you want to get HER done? I will look at improving it until then.

[araffin]

There is not fix timeline but at some point (in ~1 month and a half), we will have to use it. So, I may help you finish the implementation to get things done at that time.