-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Improves recorder performance and add additional recording capability #3302
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
4515169
Add recorder and env patches/fixes from LWLab
peterd-NV d8a8abd
make annotate demos compatabile with episode_data list format
peterd-NV 7d63110
update changelog
peterd-NV dcc4a42
run formatter
peterd-NV 1f50a55
Merge branch 'main' into peterd/lw_patches
kellyguo11 3465464
Merge branch 'main' into peterd/lw_patches
peterd-NV 19fff12
fix recorder manager and hdf5 file handler tests
peterd-NV ac765b8
run formatter
peterd-NV 871e58d
Merge branch 'main' into peterd/lw_patches
peterd-NV 48d03f3
Merge branch 'main' into peterd/lw_patches
peterd-NV e92205c
Merge remote-tracking branch 'origin/main' into peterd/lw_patches
peterd-NV 0ae949e
bump vesion number
peterd-NV c5c393f
fix episode_data test
peterd-NV 492bdea
Merge branch 'main' into peterd/lw_patches
peterd-NV 014bd6e
update record_pre_physics_step name to record_post_physics_decimation…
peterd-NV 5b561a2
remove LW specific ep meta
peterd-NV 9ca442d
Update source/isaaclab/isaaclab/managers/recorder_manager.py
kellyguo11 85f54d7
Update source/isaaclab/isaaclab/managers/recorder_manager.py
kellyguo11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Isn't this happening now after the physics step?
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.
The recording right now records the states after a physics step. Here, it's adding functionality to record the states prior to every physics step in the decimation loop which provides more fine grain recording.
Uh oh!
There was an error while loading. Please reload this page.
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.
The motivation here is that full determinism during replay is not possible when running parallel envs as the physX buffers cannot be flushed on every IsaacLab reset. This can cause significant divergence when replaying data collected with parallel envs by just using env.step as it has to run through the entire action space which may contain controllers that are sensitive to this nondeterminism. Recording at the decimation level every sim dt can help alleviate this by giving users the option of more fine grained control.
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.
Note that recording pre physics steps is fully optional and will not enabled by our existing default recorder workflows to limit dataset size. Users can take advantage of it via their own recorder terms if needed (like how Lightwheel is currently doing).
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.
but should this be called record_post_physics_step() or record_post_physics_decimated_step because it is after ?
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.
Sorry I misunderstood Mayank's original comment. Yes I agree the naming here is confusing. I have updated to be called "record_post_physics_decimation_step."