🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

✅ You can merge normally! (8 Unrelated Failures)

As of commit 4873db8 with merge base 08f0bed ():

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

$\color{#D29922}\textsf{\Large⚠\kern{0.2cm}\normalsize Warning}$ Result of CPU Benchmark Tests

Total Benchmarks: 89. Improved: $\large\color{#35bf28}22$. Worsened: $\large\color{#d91a1a}4$.

$\color{#D29922}\textsf{\Large⚠\kern{0.2cm}\normalsize Warning}$ Result of GPU Benchmark Tests

Total Benchmarks: 92. Improved: $\large\color{#35bf28}10$. Worsened: $\large\color{#d91a1a}3$.

This is great! Some thoughts -

1. I think sampling in just trajectories would be great but I can see how some API users would like to have more control of how to convert the chunks to trajectories.
2. Trajectories of insufficient chunk length should be padded up to the chunk size and saved IMHO. Another option could be to let API users pass a function to handle this case where they can decide if they want to throw an exception or just pad the insufficient chunks.

I'm not sure to understand the use case of this. Could you put in more context?

It seems useful to methods that use full episodes.

Should batch_size indicate the number of slices (ie, trajectories) sampled?

Yes, it seems a bit odd and error prone to have hald the information in the slice variable and the other half in the batch size.

Related: Should we return batches of size [M, N] where M is the number of slices and N the trajectory lengths?

This would allow to have a determinitic batch size, but would be lots of zeros. I would prefer a determinitic batch size by default, and if there's a way to save memory on the tail it'd be great.

Last one: what do we do if some trajectories have an insufficient length? Should they be considered (and possibly padded)? IMO we should just return an exception or a warning but i'm open to other suggestions.

What does insuffient lenght mean here?
It seems like sampling and giving a mask to user would be the easiest.

Should we give a device to the transform to speed up the trajectory recovering (with cuda)?

The transform?

I'm not sure to understand the use case of this. Could you put in more context?

Sure!
Currently, when people need to sample trajectories, we advise them to build RBs storages with shape [N, T], but that's clunky because it means that if I tell you "build a RB of 1M elements" you have to divide 1M by the traj length.
Also it means that you have to pad or use trajs that all have the same length. On top of that, getting sub-trajectories is hard and inefficient... Finally, because in general trajectory (episode) lengths cannot be assumed to be identical so we store them as [N x T], but people will eventually want to sample some trajectories or some parts of trajectories...

It seems useful to methods that use full episodes.

Should batch_size indicate the number of slices (ie, trajectories) sampled?

Yes, it seems a bit odd and error prone to have hald the information in the slice variable and the other half in the batch size.

On the other hand, you will get more elements than you asked for! Could be surprising too.

Related: Should we return batches of size [M, N] where M is the number of slices and N the trajectory lengths?

This would allow to have a determinitic batch size, but would be lots of zeros. I would prefer a determinitic batch size by default, and if there's a way to save memory on the tail it'd be great.

Why lots of zeroes? What I meant is: should this reshape happen within the transform?

Last one: what do we do if some trajectories have an insufficient length? Should they be considered (and possibly padded)? IMO we should just return an exception or a warning but i'm open to other suggestions.

What does insuffient lenght mean here? It seems like sampling and giving a mask to user would be the easiest.

Say some trajectories in your buffer have length 10, and you ask for a batch of total 320, across 10 splits: you will want have traj length of 32, which is greater than 10 -> should we pad or jusy throw an exception?

Should we give a device to the transform to speed up the trajectory recovering (with cuda)?

The transform?

Sorry it's Friday, I meant the sampler!

This is very cool and it is much needed to have an easy way to sample trajectories!

I am a bit confused from the decrtiption though, it seems to me that N and M are overloaded in different parts of the text

for data in collector: # data is of shape [N, T]
    replay_buffer.extend(data.view(-1)) # adds the N consecutive trajectories of length T
    samples = replay_buffer.sample(M) # M elements divided in N slices
    samples = samples.view(N, -1)

• Why is T trajectory length? in normal collectors T is just the number of collected frames divided by the batch size of collection

I think we might benefit from rewriting the description with clearer symbols and a recap of the current state of affairs prior to this change

My opinion

1. I think users should request a desired trajectory length rather than a number of slices (at construction of the sampler). this is more intuive as most people care how long the traj fragment is (leaving this as a byproduct of number_of_slices is counterintuitive to me ). Therefore I would prefer something like TrajectorySampler(cache_values=cache,traj_length=10). For this reason is also suggest naming the sampler TrajectorySampler
2. Regarding the batch size, when users request buffer.sample(n_samples) I think they should get a td with batch_size [n_slices,traj_length] where n_slices is computed as n_samples / traj_length (n_samples and traj_length are the user-defined quantities and n_slices is the byproduct)
3. If the actual episode length is different from the traj_length length provided at construction (longer for some episodes, shorter for some other). We should:
   • if shorter: pad and give a warning that some are padded
   • if longer: just cut it at traj_length and put the overflowing bit in the next slice

Good points
I think we should raise an error if the length is shorter unless users explicitly set a pad arg to True, because literally everyone in RL disable warnings, and no one ever looks at the log.

if longer: just cut it at traj_length and put the overflowing bit in the next slice

I don't get that

if longer: just cut it at traj_length and put the overflowing bit in the next slice

I don't get that

If the user requests 100 samples and my buffer has one epsiode which is 100 steps long and the user asked to sample trajectory fragments of length 70, we should return 2 fragments in a td of batch size [2,70]:

• the first fragment is the first 70 steps
• the second fragment is the last 30 steps plus 40 padded steps (supposing pagging is reuqested)

That isn't the contract of this class, which is to sample randomly subsequences (this is why it isn't a trajectory sampler but a slice sampler - it samples incomplete episodes). This is also why we add a truncated signal at the end.
The implementation you're suggesting seems more constraining to me, and surprising: consecutive samples of a batch should not be correlated.
I don't see why we should complete the trajectory in the next batch, what's the advantage? If users want full trajectories of length 100 why not asking for that?
In this implementation, I could also expect a slice of length 70 without padding because all my trajectories are longer than 70, and here I'll get some trajectories that are padded. Pretty sure that will lead to issues on the repo!

Another issue with this is that it seems to be that it assumes that all trajectories have the same length. Say this is not the case (eg, all our datasets) and you want sub trajs of length 100. One trajectory may be 101 transitions long so you will get 2 batches, one of 100 and another padded of 1. Users will never see the second batch contiguously with the first (it will appear like this one element alone isn't part of the same trajectory). Also it's not clear what's the batch size here: 200? 101? What if users wanted only 100 transitions? What if they wanted 200, should they get samples from another trajectory too, thereby having a varying output size from the buffer?

I updated the description!

Thanks for the update! The new descriptions clears a lot of things and the choice of leaving to the users if they want to give S or U is great imo!

I guess the point I got confused about is if sampling is handled with or without replacement.
Ideally, both on policy algos (like PPO) and off policy algos may want to use this, so maybe the replacement strategy of this sampler might be important.

What I understand from your answers is that this sampler samples with replacement (so the data of 2 slices sampled in the same call could be the same)

I still think though I am not getting some API details of this sampler, so here are some examples that would be helpful for me to clarify:

1. from what i understand, data from different episodes will never be present in the same slice. I.e. a slice will always contain a fragment from the same episode
2. Are we sampling from trajectories independently? For example, if i have a buffer containing 2 episodes: one of length 30 and the other of length 70, and the users asks for 100 samples in 2 slices of length 50 each, can you explain what would be given to the user?
   • Are we going to sample 2 slices of 50 from the episode with length 70?
   • Are we going to sample one slice from the episode with length 70 and one padded one from the episode with length 30?
   • Are we going to sample in a random way and pad if there are not 50 steps after the selected starting indices?

What I understand from your answers is that this sampler samples with replacement (so the data of 2 slices sampled in the same call could be the same)

yes but that can be edited

from what i understand, data from different episodes will never be present in the same slice. I.e. a slice will always contain a fragment from the same episode

yes that is the point, but we could also loosen that (RNNs and value estimators allow mixed trajectories in torchrl so that wouldn't be an issue for them)

Are we sampling from trajectories independently? For example, if i have a buffer containing 2 episodes: one of length 30 and the other of length 70, and the users asks for 100 samples in 2 slices of length 50 each, can you explain what would be given to the user?

We sample first the episodes, then within the episodes. So each element does not have the same probabilty of ending up in the batch. Not sure how to implement the other option mathematically: perhaps sample items from the buffer and then sample a slice around these samples. That will mean that some trajectories have a higher priority than others, which is as opinionated as the first.

if i have a buffer containing 2 episodes: one of length 30 and the other of length 70, and the users asks for 100 samples in 2 slices of length 50 each

Currently, you would have one sample of 50 from trajectory 1 and a sample of length 30 + 20 pads from trajectory 0.

@matteobettini What does sampling without replacement mean?
If I have a trajectory of length 101 and ask for length 100, the second time I sample from it I will have only 1 sample. This means that the samples at the end of the epoch will be degenerated. If we samples another slice that contains that element, longer trajectories will have less repetitions across samples (and it won't be truly without replacement). The only option I see would be so sample each trajectory only once (that makes more sense to me) but then we would end up with a different likelihood per transition in the buffer (which I don't think is an issue).

Remember that in some settings people allocate themselves a number of "episodes" rather than a number of "transitions", which kind of make sense: for cartpole for instance, if you solve it in 100 episodes and not 200 it's way more impressive than 1000 samples and not 2000 (since success = longer episode)

Are we sampling from trajectories independently? For example, if i have a buffer containing 2 episodes: one of length 30 and the other of length 70, and the users asks for 100 samples in 2 slices of length 50 each, can you explain what would be given to the user?

We sample first the episodes, then within the episodes. So each element does not have the same probabilty of ending up in the batch. Not sure how to implement the other option mathematically: perhaps sample items from the buffer and then sample a slice around these samples. That will mean that some trajectories have a higher priority than others, which is as opinionated as the first.

if i have a buffer containing 2 episodes: one of length 30 and the other of length 70, and the users asks for 100 samples in 2 slices of length 50 each

Currently, you would have one sample of 50 from trajectory 1 and a sample of length 30 + 20 pads from trajectory 0.

Now I got it! Thanks for clarifying!

To me it was not clear that we first sample episode indeces and then slices.
So the sampling with replacement happens at 2 levels: for episodes (first) and trajecotries (second).
Or is the first sampling of episodes without replacement?

Yes I guess this has some implications. Basically each episode will be weighted equally (so if you have an episode of length 1 where the agent committed suicide in one step) this will have a dedicated slice of length U (potentially polluting the data)

The other implementation I was referring to is that you sample S indeces at random in the buffer flattened data and then you look U steps in the future from that index (if you do not have U steps in the future, you pad the remaining ones).

This implementation removes the hierarchical sampling (of episodes first and then trajs), but (as you said) gives less weight to shorter trajectories.

@matteobettini added SliceSamplerWithoutReplacement, which simply inherits from both parent classes in a rather simple way.

Sorry for being late to the party. This is really cool and much clearer now!

Raising a broader question: the description of the PR seems outdated again, so for recent features do we want to rebuild the documentation and serve it somewhere? or rely on the PR descriptions for the temporary documentation?

Good point I can update it! But the doc should be accurate