Skip to content
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

Add observation term history support to Observation Manager #1439

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jtigue-bdai
Copy link
Collaborator

@jtigue-bdai jtigue-bdai commented Nov 19, 2024

Description

This PR adds observation history by adding configuration parameters to ObservationTerms and having the ObservationManager handling the collection and storage of the histories via CircularBuffers.

Fixes #1208

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

* add history cfg option to observation manager

* Revert "add history cfg option to observation manager"

This reverts commit 4b00994b8c9ce6e58779150dc42a707b0bc6701f.

* add flatten history option to observation manager

* run formatter

* fix docstring
@jtigue-bdai jtigue-bdai self-assigned this Nov 19, 2024
@jtigue-bdai jtigue-bdai added enhancement New feature or request dev team Issue or pull request created by the dev team labels Nov 19, 2024
@KyleM73
Copy link
Contributor

KyleM73 commented Nov 19, 2024

From the discussion in #1208, this PR introduces observation history tracking per-term. It was discussed that per-group tracking should also be implemented. In theory, the same effect can be had by giving all obs terms the same history length with the config provided in this PR. However, I want to reiterate what I think are the two best arguments for implementing per-group obs histories as well, and would be happy to hear feedback:

  1. When writing deployment code, it is often more straightforward and natural to save the entire observation to a 2D buffer of size (H,D) for H history and D observation dimension. In the current implementation, separate buffers would need to be kept for each observation term as defined by Isaac Lab. However, it may not be trivial to do so, whereas to evaluate RL policies on hardware it is guaranteed that the observations will be stacked into the full length-D array at some point, making saving them as a per-group history easier to replicate on hardware.
  2. Providing an implementation at the group level is a cleaner interface for developers than having to change the history length for each obs term. Especially when subclassing observation group configs from parent classes, it would be much nicer to define the history in the observation group post init (the interface currently used for enable_corruption and concatenate_terms, etc.) as opposed to having to change it for every term. Having used only my own version of the code in this PR for the last ~6 months, I have inadvertently missed changing the history length for one or two obs terms many times, requiring restarting the run. This bug can be avoided by optionally choosing to only change the history length in a single location, namely in the obs group post init.

The PR as-is resolves per-term observation history tracking. If it is decided that Isaac Lab should only support per-term observation histories then I am happy to close my comment/proposal with this PR. Otherwise, I'm happy to help implement per-group history tracking as well.

Thanks for putting this together James!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev team Issue or pull request created by the dev team enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Observation Histories
3 participants