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

[Bug Report] Observation groups using the same mdp function have strange behaviour. #356

Closed
2 of 4 tasks
vassil-atn opened this issue Apr 11, 2024 · 3 comments
Closed
2 of 4 tasks
Assignees
Labels
bug Something isn't working

Comments

@vassil-atn
Copy link

Describe the bug

If you use two observation groups (low-level and high-level control) that use the same mdp function (for example mdp.base_lin_vel) have strange behaviour, where the changes in one observation group affect the other one.

Steps to reproduce

@configclass
class ObservationsCfg:

  @configclass
  class LowLevelObsCfg(ObsGroup):
      """Observations for low-level group."""
  
      base_lin_vel = ObsTerm(func=mdp.base_lin_vel, noise=Unoise(n_min=-0.1, n_max=0.1),scale=2.0)
      
  @configclass
  class HighLevelObsCfg(ObsGroup):
      """Observations for high-level group."""

      base_lin_vel = ObsTerm(func=mdp.base_lin_vel, noise=Unoise(n_min=-0.1, n_max=0.1),scale=2.0)

    # observation groups
    highLevelPolicy: HighLevelObsCfg = HighLevelObsCfg()
    lowLevelPolicy: LowLevelObsCfg = LowLevelObsCfg()

The expected behaviour (correct me if I'm wrong) would be to have two observation groups that have the same entry for base_lin_vel. However, the actual behaviour is for the first obs group (high level in this case) to have base_lin_vel scaled by 2. (as expected), but the second group has the base_lin_vel scaled by 4. (or rather it has the base_lin_vel of highLevelPolicy scaled by an additional 2.).

Similarly, if you have two observations within the same group that use the same function, the same occurs. A dirty workaround would be to create new mdp functions for the low-level and high-level policies.

Is there anything I'm missing in the definition of the observation terms and groups?

System Info

Describe the characteristic of your environment:

  • Commit: bca680a
  • Isaac Sim Version: 2023.1.1-rc.8+2023.1.688.573e0291.tc
  • OS: Ubuntu 22.04
  • GPU: RTX 3060
  • CUDA: 12.2
  • GPU Driver: 535.161.07

Checklist

  • I have checked that there is no similar issue in the repo (required)
  • I have checked that the issue is not in running Isaac Sim itself and is related to the repo

Acceptance Criteria

Add the criteria for which this task is considered done. If not known at issue creation time, you can add this once the issue is assigned.

  • Clarification of how observation groups should work.
  • Fixed if a bug.
@vassil-atn
Copy link
Author

After some more testing it seems that the issue is due to the mdp function returning the asset data directly rather than a copy of it (which means the scaling is then applied to the internal asset data). A fix would be to return a copy instead like:

return asset.data.root_lin_vel_b.clone()

instead of

return asset.data.root_lin_vel_b

@Mayankm96
Copy link
Contributor

Mayankm96 commented Apr 11, 2024

Hi @Vassil17 ,

This is a really interesting bug there! Pretty surprised we did not notice it before. All the operations here are doing an operation on "self" to avoid creating new tensors:

https://github.com/NVIDIA-Omniverse/orbit/blob/main/source/extensions/omni.isaac.orbit/omni/isaac/orbit/managers/observation_manager.py#L170-L178

However, this indeed should yield the bug you reported above.

The fix is to add a "clone" operator here: https://github.com/NVIDIA-Omniverse/orbit/blob/main/source/extensions/omni.isaac.orbit/omni/isaac/orbit/managers/observation_manager.py#L170-L171

@Mayankm96 Mayankm96 added the bug Something isn't working label Apr 11, 2024
@Mayankm96
Copy link
Contributor

Sent an MR with the fix. It should get merged soon :)

@Mayankm96 Mayankm96 self-assigned this Apr 11, 2024
fatimaanes pushed a commit to fatimaanes/omniperf that referenced this issue Aug 8, 2024
…#493)

# Description

This MR does the following fixes:

* Adds a clone operator to the observation manager term computation to
prevent shared data between terms
* Fixes the flushing of data for imitation learning worklow

Fixes isaac-sim#356

## Type of change

- Bug fix (non-breaking change which fixes an issue)

## Checklist

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants