First draft for modular Hindsight Experience Replay Transform

[dtsaras]

Description

I have added the Hindsight Experience Replay Transform specifically implementing the future and last strategy as described in the paper. The transform is a combination of 3 transforms:

• HERSubGoalSampler: It's responsible for sampling indexes for the subgoals and can be changed with another subgoal sampling method for the specific use case.
• HERSubGoalAssigner: It's the method responsible for creating new trajectories given the subgoal indices.
• HERRewardTransform: While it might not necessarily need to be a separate transform yet, it's the method responsible for reassigning the rewards to the newly generated trajectories.

Motivation and Context

It's a modular implementation for hindsight experience replay as requested #1819

• I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds core functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (update in the documentation)
• Example (update in the folder of examples)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

• I have read the CONTRIBUTION guide (required)
• My change requires a change to the documentation.
• I have updated the tests accordingly (required for a bug fix or a new feature).
• I have updated the documentation accordingly.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

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

[vmoens]

Makes sense to me! Thanks for working on this.

I'd love to see some tests to understand better how this all works.
I left some comments here and there, mostly about formatting and high-level design decisions. Happy to give it a more thorough technical look later once there's an example of how to run it and/or some tests to rely upon.

[vmoens]

Suggested change

	"""Returns a TensorDict with a key `subgoal_idx` of shape [batch_size, num_samples] represebting the subgoal index.
	Available strategies are: `last` and `future`. The `last` strategy assigns the last state as the subgoal. The `future` strategy samples up to `num_samples` subgoal from the future states.
	Args:
	num_samples (int): Number of subgoals to sample from each trajectory. Defaults to 4.
	out_keys (str): The key to store the subgoal index. Defaults to "subgoal_idx".
	"""Returns a TensorDict with a key `subgoal_idx` of shape [batch_size, num_samples] represebting the subgoal index.
	Available strategies are: `last` and `future`. The `last` strategy assigns the last state as the subgoal. The `future` strategy samples up to `num_samples` subgoal from the future states.
	Args:
	num_samples (int): Number of subgoals to sample from each trajectory. Defaults to 4.
	==> NOT PRESENT: out_keys (str): The key to store the subgoal index. Defaults to "subgoal_idx".
	subgoal_idx_key: TODO
	strategy: TODO

[vmoens]

I would add a .. seealso:: with other related classes.

[vmoens]

Suggested change

	subgoal_idx_key: str = "subgoal_idx",
	strategy: str = "future"
	subgoal_idx_key: NestedKey = "subgoal_idx",
	strategy: NestedKey = "future"

[vmoens]

maybe let's create a dedicated file for these?

[vmoens]

if 0 or greater than 2, raise an error?

[vmoens]

maybe

Suggested change

	batch_size, trajectory_len = trajectories.shape
	*batch_size, trajectory_len = trajectories.shape

to account for batch size > 2

[vmoens]

Suggested change

	SubGoalSampler: Transform = HERSubGoalSampler(),
	SubGoalAssigner: Transform = HERSubGoalAssigner(),
	RewardTransform: Transform = HERRewardTransform(),
	assign_subgoal_idxs: bool = False,
	):
	super().__init__(
	in_keys=None,
	in_keys_inv=None,
	out_keys_inv=None,
	)
	SubGoalSampler: Transform | None = None,
	SubGoalAssigner: Transform | None = None,
	RewardTransform: Transform | None= None,
	assign_subgoal_idxs: bool = False,
	):
	if SubGoalSampler is None:
	SubGoalSampler = HERSubGoalSampler()
	if SubGoalAssigner is None:
	SubGoalAssigner = HERSubGoalAssigner()
	if HERRewardTransform is None:
	HERRewardTransform = HERRewardTransform()
	super().__init__(
	in_keys=None,
	in_keys_inv=None,
	out_keys_inv=None,
	)

[vmoens]

No PascalCase but snake_case for instantiated classes

[vmoens]

ditto

[vmoens]

maybe

Suggested change

	batch_size, trajectory_length = trajectories.shape
	*batch_size, trajectory_length = trajectories.shape

[vmoens]

Suggested change

	for i in range(batch_size):
	for i in range(batch_size.numel()):

which also works with batch_size=torch.Size([])!

[vmoens]

Don't we need to modify the specs?
Does this work with replay buffer (static data) or only envs? If the latter, we should not be using forward.

If you look at Compose, there are a bunch of things that need to be implemented when nesting transforms, like clone, cache eraser etc.

Perhaps we could inherit from Compose and rewrite forward, _apply_transform, _call, _reset etc such that the logic hold but the extra features are included automatically?