-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Transform for Recording History Observations #1763
base: main
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/1763
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (16 Unrelated Failures)As of commit 6c1109e with merge base eec9f9e (): FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff!
Is this the stack
version of CatFrames
? Do we want to have two separate transforms for that? If what we want is for CatFrames
to have the option to discard the last observation or not, or use stack instead of cat, these could be passed as arguments (though I can see why one would rather have two separate classes). I can give some advice if we want to go down that route.
If we have two separate transforms, maybe we could name this one StackFrames
for consistency? Is there any piece of code from CatFrames that we should recycle?
We should add the transform to the doc too.
self, | ||
in_keys: Sequence[NestedKey], | ||
out_keys: Sequence[NestedKey] = None, | ||
steps: int = 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we use 16?
Thanks for replying! Yes, I think renaming it to I will complete the tests and doc soon. |
It's good to have it stateless I agree. Let's keep it as it is! |
Co-authored-by: Vincent Moens <[email protected]>
Co-authored-by: Vincent Moens <[email protected]>
@btx0424 Anything I can help you with here? I can find some time to help if needed |
Hi there. I had some trouble figuring out the no-env usage while trying to align it with |
Just seeing this now sorry |
Description
This draft PR proposes a
Transform
for recording histories of observations, which is a common practice in robotic tasks. It should also address #1676: to record observation-action history, append the previous action to observation, and then transform the environment withHistory
.The naming, description (docstring), and tests are for the initial commit. Opinions and suggestions are wanted.
Types of changes
What types of changes does your code introduce? Remove all that do not apply:
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!