[Feature] Added linear interpolation to FrameSkipTransform

[Zekrom-7780]

Description

• Added an action_interp parameter to enable or disable the action interpolation feature.
• Added an action_interp_buffer attribute to store the previous action for interpolation.

Motivation and Context

Solves Issue #1659

Types of changes

• New feature (non-breaking change which adds core functionality)
• Documentation (update in the documentation)

Checklist

• I have read the CONTRIBUTION guide (required)
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

Reviewers

cc. @vmoens

P.S. I don't know if I have to add unit-tests for this, OR if this is a breaking change, so I haven't added them here.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⏳ 8 Pending, 3 Unrelated Failures

As of commit ce0a846 with merge base 11562c7 ():

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

• Continuous Benchmark (PR) / CPU Pytest benchmark (gh)
• Continuous Benchmark (PR) / GPU Pytest benchmark (linux.4xlarge.nvidia.gpu, 3.8) (gh)

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• Habitat Tests on Linux / tests (3.9, 11.6) / linux-job (gh)

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

[vmoens]

Thanks so much for this!
We need to add a couple of tests for this feature.
What should happen at reset time, if there is more than one env being run?

[vmoens]

why this change?

[Zekrom-7780]

Ohh, that is a mistake from my side, sorry!
(regarding the init)

[vmoens]

Suggested change

	action_interp (bool, optional): whether to perform action interpolation over frame_skip steps.
	action_interp (bool, optional): whether to perform action interpolation over frame_skip steps.
	Defaults to ``False``.

[vmoens]

Let's add an example that in the docstring

Suggested change

	action_interp (bool, optional): whether to perform action interpolation over frame_skip steps.
	Suggested change
	action_interp (bool, optional): whether to perform action interpolation over frame_skip steps.
	Defaults to ``False``.
	Examples:
	>>> from torchrl.envs import GymEnv
	>>> env = TransformedEnv(GymEnv("CartPole-v1"), FrameSkipTransform(3, action_interp=False))
	>>> print(env.rollout)
	# add print here
	>>> env = TransformedEnv(GymEnv("CartPole-v1"), FrameSkipTransform(3, action_interp=True))
	>>> print(env.rollout)
	# add print here

[Zekrom-7780]

Suree, will add it in the next commit

[vmoens]

this should be a parameter in the class constructor

[Zekrom-7780]

Got it

[vmoens]

would it make sense to store this contiguously (with a torch.stack around it)?

[Zekrom-7780]

I don't understand what you mean by this, could you like elaborate?

[vmoens]

can num_steps be smaller than 0?

[Zekrom-7780]

They cannot be, but still, I felt the need to add them, just in case the people using them don't run into any issues (like trying to enter a negative number just for fun and all)

[vmoens]

I'm not sure I see what this block is doing, can you give an example?
It'd be cool to see (eg in the tests) what you would expect to get from it

[Zekrom-7780]

Ok, so my reasoning here was for let's say num_steps = 5, we have:

At step = 1, alpha = 1/5 and interpolated_action is calculated as follows:
interpolated_action=start_action+(1/5)*(end_action - start_action)

Similarly for each step (like for the 4th step, alpha=4/5 ) and interpolated_action is calculated as follows:
interpolated_action=start_action+(4/5)*(end_action - start_action)

And finally, at step = 5, alpha = 5/5 (which is 1), and interpolated_action becomes :
interpolated_action=end_action

So like theinterpolation_steps list contains the sequence of actions that go from from start_action to end_action over the specified number of steps, which in this example is 5.
I felt that this made sense for moving from start_action to end_action , for smooth action transitions over multiple steps

[Zekrom-7780]

I don't know if this makes sense, but I felt that this is how i have to do it

[vmoens]

Ok got it, do you mind if I try to do a vectorized version of this once you have written tests? I think we can make it faster

[Zekrom-7780]

Suree! please do it!
I'll send another round of commits by the next 3ish hours
Also please could you answer what

would it make sense to store this contiguously (with a torch.stack around it)?

This means?

[Zekrom-7780]

@vmoens, added the changes that you told me to.

Ready for another round of comments

would it make sense to store this contiguously (with a torch.stack around it)?

I wasn't sure what to do here, so I just wrapped interpolated_actions around a torch.stack and returned it.

Added action_key = "_action" as a parameter in the constructor, and now, will have to change the docstrings as well
Is the action correct, or you wanted a different word, or a different way of doing it?

Thanks so much for this!
We need to add a couple of tests for this feature.
What should happen at reset time, if there is more than one env being run?

Here, Should I create a action_interp_buffer (let's call it that) for each environment, and like reset when that environment is called?
But then how do I get the envirnoment like ID so as to know which one to reset?

[vmoens]

@vmoens, added the changes that you told me to.

Ready for another round of comments

would it make sense to store this contiguously (with a torch.stack around it)?

I wasn't sure what to do here, so I just wrapped interpolated_actions around a torch.stack and returned it.

Added action_key = "_action" as a parameter in the constructor, and now, will have to change the docstrings as well Is the action correct, or you wanted a different word, or a different way of doing it?

Thanks so much for this!
We need to add a couple of tests for this feature.
What should happen at reset time, if there is more than one env being run?

Here, Should I create a action_interp_buffer (let's call it that) for each environment, and like reset when that environment is called? But then how do I get the envirnoment like ID so as to know which one to reset?

I'd make a step back and define precisely what we expect this to do.
What I understand we want from that feature is to have a stack of interpolated actions, for instance:

• if action at t=0 is act = [1] and action at t=1 with frame_skip=4 is act=[5] we want the interpolated action to be [[1], [2], [3], [4], [5]], is that right?
• Maybe people will want to have the interpolated action and the action, so we should be able to pass the interpolated action as a separate key. Something like action = [5] and interpolated_action = [[1], [2], [3], [4], [5]]? I think overwriting the "action" entry by default is dangerous because the action you will see out of the transformed environment will be incompatible with the base environment, so the default should be a separate entry for the interpolated action. If people want, they can use the "action" key name in the constructor but that will be subject to bugs (from which the user is responsible since this is not the default).
• The way we do the interpolation (in a buffer or in the tensordict) seems accessory for now, I would first code the feature and all the test, then we can change the inner mechanism (I'll be happy to help do not worry).
• Do we allow any action type? Eg, categorical or one-hot? If not we should check that the action_spec of the parent env (transform.parent.full_action_spec[transform.action_key]) has a compatible type (ie, if of dtype float or smth like that).
• To me the top-1 priority here is to write a series of tests. To do that, go in test_transforms.py and add one or more tests in TestFrameSkipTransform. Check that the resulting action has the right values by inputting two actions at two consecutive steps and check that the values stricly match what you're expecting. That way we will know that any change we make works within the bounds of what we want to do. Also test what happens at reset time. Let's work with a single environment for now (no batched envs) and we'll adapt that to batched envs later.

[Zekrom-7780]

@vmoens , I read your comments, and you mention that my first priority should be adding tests

So I looked at the test_transforms.py , and it has like 9000ish lines of code!!
Where should I like write my tests?
This file made me feel extremely overwhelmed!!

[vmoens]

Ahah I can imagine!
There's a TestFrameSkipTransform with all the frame-skip related stuff. You just need a couple of functions that test that if you create a frame stkip transform the results are what is expected

[Zekrom-7780]

Thanks a lot, will push some tests later, most probably in 24ish hours