🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/1819

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures, 19 Unrelated Failures

As of commit 90eef75 with merge base 57139bd ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@ahmed-touati suggested we use a sampler for this rather than a transform. I'm not strongly opinionated on the matter, mostly because I need more context on what we're trying to achieve here. Can you elaborate a bit more on what this transform does, maybe with a bunch of examples?

So HER is mainly used in goal-conditioned RL with sparse reward signals where the agent has to reach/achieve a goal state and only gets a reward (+1) when the goal state is achieved, otherwise no reward. The observation consists of three elements: the observation the agent sees, the state the agent had (could be x,y,z position), and the goal state the agent should reach (x,y,z). A typical task could be a robot that has to reach a goal position. The observation will include the agent position but its mostly added as additional information also helps here for understanding.

Now as we have a sparse reward function most of the trajectories will have no learning signal for the agent as it might not be possible for the agent to reach the goal position randomly or by pure luck.
What HER now does is that for each step in a trajectory that you want to add to the buffer, you sample a new goal state and pretend that this was the actual goal the agent had to reach.

So lets say you have a real transition (obs, action, reward, done, next obs, achieved_position, goal_position) for this tuple you now want to sample a new goal_position and then calculate the reward based on this new goal_position and the real achieved_position. So you then add the real transition (obs, action, reward, done, next obs, achieved_position, goal_position) but also the HER augmented transition (obs, action, new_reward, done, new next obs, achieved_position, new_goal_position). The sampling can happen in different ways but is not important for now. However, I think important will be that we need the reward function, Im not sure if we can pass it to the writer/sampler for the buffer, that's why my first thought was a transform. Most of the time the reward function might just be Euclidean distance but maybe for other tasks the user needs to provide a more sophisticated reward function.

However, I think important will be that we need the reward function, Im not sure if we can pass it to the writer/sampler for the buffer, that's why my first thought was a transform.

Why not? I would guess that even if it's a complex nn.Module you can still do pretty much everything with a well tailored function (at least nothing less than with a transform).

Thanks for the context btw!

Why not? I would guess that even if it's a complex nn.Module you can still do pretty much everything with a well tailored function (at least nothing less than with a transform).

Revisiting this I think it would make much more sense to do it with a writer. We want to augment current incoming data with new sampled goal states and store them all together in the buffer. I think this would be generally a good way to add other data augmentation strategies with writer instead of transforms. Having a closer look right now on the writer classes and will update the code here

But this would not allow us to stack multiple augmentations on top of each other... so maybe not that ideal for augmentations

You could still transform your data before passing it to the writer, but not after

Not sure about that one.
We've had that request already 3 times but for 3 different purposes so if there's a way to make it a modular component of the lib I'd like to consider that above a script that is harder to reuse (and more error prone on the user side)

Are you planning to continue working on this PR? If not, I'd be happy to help out and look into finishing it myself.

Additionally, @vmoens, do you now have a clearer sense of whether this should be implemented as a transform, writer, or sampler? It would be great to make sure we're aligned on the best approach before moving forward.

Looking forward to your thoughts!

Are you planning to continue working on this PR? If not, I'd be happy to help out and look into finishing it myself.

Additionally, @vmoens, do you now have a clearer sense of whether this should be implemented as a transform, writer, or sampler? It would be great to make sure we're aligned on the best approach before moving forward.

Looking forward to your thoughts!

I have an already complete version of it in a gist but I have not made a PR for it yet. Maybe you can have a look and provide feedback. I had talked with Alexandre on discord long ago over the implementation and he liked it. Any feedback? @vmoens https://gist.github.com/dtsaras/f321aed253a64e4849ce95bd232d1635

I really like the modular approach you've taken! That said, I have a few questions about some details of the implementation.

From what I understand, the reward function would be implemented as a HERRewardTransform. I wonder if this might be more complex than necessary compared to an alternative approach, such as passing a compute_reward_fn: Callable[[torch.Tensor, torch.Tensor], torch.Tensor] directly to the HERTransform class.

Could you share your thoughts on the reasoning behind including the HERSubGoalSampler? If we implement the common sampling methods (random, final, future), it seems like the additional complexity might not be strictly necessary – but I'd love to hear your perspective!

I really like the modular approach you've taken! That said, I have a few questions about some details of the implementation.

From what I understand, the reward function would be implemented as a HERRewardTransform. I wonder if this might be more complex than necessary compared to an alternative approach, such as passing a compute_reward_fn: Callable[[torch.Tensor, torch.Tensor], torch.Tensor] directly to the HERTransform class.

Could you share your thoughts on the reasoning behind including the HERSubGoalSampler? If we implement the common sampling methods (random, final, future), it seems like the additional complexity might not be strictly necessary – but I'd love to hear your perspective!

You are correct, the HERRewardTransform would be responsible to assign the rewards for all the intermediate states. While it's not necessary that it has to be its own special transform, it has to be "something" that reassigns the rewards to intermediate states of a trajectory. The reason I choose for it to be a Transform rather than a callable is to utilize the torchRL API. For example, the user does not need to reimplement the discounted reward function as Reward2GoTransform exists and multiple transforms can also be nicely composed into one.

The HERSubgoalSampler I have implemented only contains [final (marked as last in the code), future]. I did not implement random since it did not produce good results in the original paper if I recall correctly. The HERSubgoalSampler is only responsible for sampling states i.e. identifying the indices to be used for HERSubgoalAssigner. Do you mind elaborating more on your concerns about the complexity?

Maybe I should create a PR and work on it together where you can make changes.

P.S. You can join the Discord server as well and maybe more can provide some feedback there.

You are correct, the HERRewardTransform would be responsible to assign the rewards for all the intermediate states. While it's not necessary that it has to be its own special transform, it has to be "something" that reassigns the rewards to intermediate states of a trajectory. The reason I choose for it to be a Transform rather than a callable is to utilize the torchRL API. For example, the user does not need to reimplement the discounted reward function as Reward2GoTransform exists and multiple transforms can also be nicely composed into one.

I'm sorry, but I'm having a bit of trouble following your reasoning here. Could you elaborate on why it can't be a Callable? Wouldn't that still allow us to use Reward2GoTransform and compose it as needed?

The HERSubgoalSampler I have implemented only contains [final (marked as last in the code), future]. I did not implement random since it did not produce good results in the original paper if I recall correctly. The HERSubgoalSampler is only responsible for sampling states i.e. identifying the indices to be used for HERSubgoalAssigner. Do you mind elaborating more on your concerns about the complexity?

Ah, I see – sorry for the confusion! What I meant is that, as long as we implement all the sampling methods outlined in the paper, it seems unlikely that we'd need another one. This makes me think we could move the subgoal sampling logic directly into the HERTransform and potentially remove the HERSubgoalSampler class altogether.

I understand that random sampling didn't yield good results, but I thought it might be valuable to include it for the sake of completeness, since it was mentioned in the original paper.

Maybe I should create a PR and work on it together where you can make changes.

That sounds like a great idea.

P.S. You can join the Discord server as well and maybe more can provide some feedback there.

Thanks for letting me know! I didn't realize there was a Discord server; I've just joined.